| Message ID | 20231130-encoder-fixup-v1-2-585c54cd046e@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: INTF CRC configuration cleanups and fix | expand |
On Fri, 1 Dec 2023 at 03:31, Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > Set the input_sel bit for encoders as it was missed in the initial > implementation. > > Reported-by: Rob Clark <robdclark@gmail.com> > Fixes: 91143873a05d ("drm/msm/dpu: Add MISR register support for interface") > Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/39 > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 7 ++++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +++- > 4 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > index 3442cf65b86f..d0884997ecb7 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > @@ -320,7 +320,7 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) > > static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf) > { > - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL); > + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, true); > } > > static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > index f38473e68f79..77b14107c84a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, > > static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx) > { > - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL); > + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, false); > } > > static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > index a8a0a4e76b94..f441df47fdde 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > @@ -481,7 +481,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > cfg->danger_safe_en ? QOS_QOS_CTRL_DANGER_SAFE_EN : 0); > } > > -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) > +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, > + bool set_input_sel) > { > u32 config = 0; > > @@ -491,6 +492,10 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) > wmb(); > > config = MISR_FRAME_COUNT | MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; > + > + if (set_input_sel) > + config |= MISR_CTRL_INPUT_SEL; > + > DPU_REG_WRITE(c, misr_ctrl_offset, config); > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > index bb496ebe283b..793670d62414 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > @@ -17,6 +17,7 @@ > #define MISR_CTRL_ENABLE BIT(8) > #define MISR_CTRL_STATUS BIT(9) > #define MISR_CTRL_STATUS_CLEAR BIT(10) > +#define MISR_CTRL_INPUT_SEL BIT(24) The public apq8916 TRM documents this as a 4-bit field. I think this was followed into the later generations. Can we please document it correctly and use an uint instead of just bool for set_input_sel? > #define MISR_CTRL_FREE_RUN_MASK BIT(31) > > /* > @@ -357,7 +358,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > bool qos_8lvl, > const struct dpu_hw_qos_cfg *cfg); > > -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset); > +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, > + bool set_input_sel); > > int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, > u32 misr_ctrl_offset, > > -- > 2.43.0 >
On 11/30/2023 11:36 PM, Dmitry Baryshkov wrote: > On Fri, 1 Dec 2023 at 03:31, Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >> >> Set the input_sel bit for encoders as it was missed in the initial >> implementation. >> >> Reported-by: Rob Clark <robdclark@gmail.com> >> Fixes: 91143873a05d ("drm/msm/dpu: Add MISR register support for interface") >> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/39 >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 7 ++++++- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +++- >> 4 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >> index 3442cf65b86f..d0884997ecb7 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >> @@ -320,7 +320,7 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) >> >> static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf) >> { >> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL); >> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, true); >> } >> >> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >> index f38473e68f79..77b14107c84a 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, >> >> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx) >> { >> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL); >> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, false); >> } >> >> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >> index a8a0a4e76b94..f441df47fdde 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >> @@ -481,7 +481,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, >> cfg->danger_safe_en ? QOS_QOS_CTRL_DANGER_SAFE_EN : 0); >> } >> >> -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) >> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, >> + bool set_input_sel) >> { >> u32 config = 0; >> >> @@ -491,6 +492,10 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) >> wmb(); >> >> config = MISR_FRAME_COUNT | MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; >> + >> + if (set_input_sel) >> + config |= MISR_CTRL_INPUT_SEL; >> + >> DPU_REG_WRITE(c, misr_ctrl_offset, config); >> } >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >> index bb496ebe283b..793670d62414 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >> @@ -17,6 +17,7 @@ >> #define MISR_CTRL_ENABLE BIT(8) >> #define MISR_CTRL_STATUS BIT(9) >> #define MISR_CTRL_STATUS_CLEAR BIT(10) >> +#define MISR_CTRL_INPUT_SEL BIT(24) > > The public apq8916 TRM documents this as a 4-bit field. I think this > was followed into the later generations. Can we please document it > correctly and use an uint instead of just bool for set_input_sel? > Can you pls point us to this document you are referring? I was not aware that bit level details are revealed in external documents :) Even though its a 4-bit field, it only takes a 0 or 1 as others are undefined. Exposing all the bits will only cause more confusion like it did for others thinking that input select is actually configurable when its not. I think what we should do is just pass "misr_type" to this API to tell whether its lm misr or intf misr and set BIT(24) based on that. >> #define MISR_CTRL_FREE_RUN_MASK BIT(31) >> >> /* >> @@ -357,7 +358,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, >> bool qos_8lvl, >> const struct dpu_hw_qos_cfg *cfg); >> >> -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset); >> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, >> + bool set_input_sel); >> >> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, >> u32 misr_ctrl_offset, >> >> -- >> 2.43.0 >> > >
On 01/12/2023 23:29, Abhinav Kumar wrote: > > > On 11/30/2023 11:36 PM, Dmitry Baryshkov wrote: >> On Fri, 1 Dec 2023 at 03:31, Jessica Zhang <quic_jesszhan@quicinc.com> >> wrote: >>> >>> Set the input_sel bit for encoders as it was missed in the initial >>> implementation. >>> >>> Reported-by: Rob Clark <robdclark@gmail.com> >>> Fixes: 91143873a05d ("drm/msm/dpu: Add MISR register support for >>> interface") >>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/39 >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 7 ++++++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +++- >>> 4 files changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>> index 3442cf65b86f..d0884997ecb7 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>> @@ -320,7 +320,7 @@ static u32 dpu_hw_intf_get_line_count(struct >>> dpu_hw_intf *intf) >>> >>> static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf) >>> { >>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL); >>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, true); >>> } >>> >>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 >>> *misr_value) >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>> index f38473e68f79..77b14107c84a 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct >>> dpu_hw_mixer *ctx, >>> >>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx) >>> { >>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL); >>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, false); >>> } >>> >>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 >>> *misr_value) >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>> index a8a0a4e76b94..f441df47fdde 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>> @@ -481,7 +481,8 @@ void _dpu_hw_setup_qos_lut(struct >>> dpu_hw_blk_reg_map *c, u32 offset, >>> cfg->danger_safe_en ? >>> QOS_QOS_CTRL_DANGER_SAFE_EN : 0); >>> } >>> >>> -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>> misr_ctrl_offset) >>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>> misr_ctrl_offset, >>> + bool set_input_sel) >>> { >>> u32 config = 0; >>> >>> @@ -491,6 +492,10 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map >>> *c, u32 misr_ctrl_offset) >>> wmb(); >>> >>> config = MISR_FRAME_COUNT | MISR_CTRL_ENABLE | >>> MISR_CTRL_FREE_RUN_MASK; >>> + >>> + if (set_input_sel) >>> + config |= MISR_CTRL_INPUT_SEL; >>> + >>> DPU_REG_WRITE(c, misr_ctrl_offset, config); >>> } >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>> index bb496ebe283b..793670d62414 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>> @@ -17,6 +17,7 @@ >>> #define MISR_CTRL_ENABLE BIT(8) >>> #define MISR_CTRL_STATUS BIT(9) >>> #define MISR_CTRL_STATUS_CLEAR BIT(10) >>> +#define MISR_CTRL_INPUT_SEL BIT(24) >> >> The public apq8916 TRM documents this as a 4-bit field. I think this >> was followed into the later generations. Can we please document it >> correctly and use an uint instead of just bool for set_input_sel? >> > > Can you pls point us to this document you are referring? I have this link in my bookmarks, which doesn't seem to work no longer: https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf?referrer=node/29241 96boards forum has several links and mentions of this doc. > > I was not aware that bit level details are revealed in external > documents :) > > Even though its a 4-bit field, it only takes a 0 or 1 as others are > undefined. > > Exposing all the bits will only cause more confusion like it did for > others thinking that input select is actually configurable when its not. > > I think what we should do is just pass "misr_type" to this API to tell > whether its lm misr or intf misr and set BIT(24) based on that. This would be another simplification. Can we instead just use values 0 and 1 instead and maybe document that by default everybody should use 0. > > >>> #define MISR_CTRL_FREE_RUN_MASK BIT(31) >>> >>> /* >>> @@ -357,7 +358,8 @@ void _dpu_hw_setup_qos_lut(struct >>> dpu_hw_blk_reg_map *c, u32 offset, >>> bool qos_8lvl, >>> const struct dpu_hw_qos_cfg *cfg); >>> >>> -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>> misr_ctrl_offset); >>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>> misr_ctrl_offset, >>> + bool set_input_sel); >>> >>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, >>> u32 misr_ctrl_offset, >>> >>> -- >>> 2.43.0 >>> >> >>
On 12/2/2023 11:54 AM, Dmitry Baryshkov wrote: > On 01/12/2023 23:29, Abhinav Kumar wrote: >> >> >> On 11/30/2023 11:36 PM, Dmitry Baryshkov wrote: >>> On Fri, 1 Dec 2023 at 03:31, Jessica Zhang >>> <quic_jesszhan@quicinc.com> wrote: >>>> >>>> Set the input_sel bit for encoders as it was missed in the initial >>>> implementation. >>>> >>>> Reported-by: Rob Clark <robdclark@gmail.com> >>>> Fixes: 91143873a05d ("drm/msm/dpu: Add MISR register support for >>>> interface") >>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/39 >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 7 ++++++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +++- >>>> 4 files changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> index 3442cf65b86f..d0884997ecb7 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> @@ -320,7 +320,7 @@ static u32 dpu_hw_intf_get_line_count(struct >>>> dpu_hw_intf *intf) >>>> >>>> static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf) >>>> { >>>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL); >>>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, true); >>>> } >>>> >>>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 >>>> *misr_value) >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> index f38473e68f79..77b14107c84a 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct >>>> dpu_hw_mixer *ctx, >>>> >>>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx) >>>> { >>>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL); >>>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, false); >>>> } >>>> >>>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 >>>> *misr_value) >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> index a8a0a4e76b94..f441df47fdde 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> @@ -481,7 +481,8 @@ void _dpu_hw_setup_qos_lut(struct >>>> dpu_hw_blk_reg_map *c, u32 offset, >>>> cfg->danger_safe_en ? >>>> QOS_QOS_CTRL_DANGER_SAFE_EN : 0); >>>> } >>>> >>>> -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>>> misr_ctrl_offset) >>>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>>> misr_ctrl_offset, >>>> + bool set_input_sel) >>>> { >>>> u32 config = 0; >>>> >>>> @@ -491,6 +492,10 @@ void dpu_hw_setup_misr(struct >>>> dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) >>>> wmb(); >>>> >>>> config = MISR_FRAME_COUNT | MISR_CTRL_ENABLE | >>>> MISR_CTRL_FREE_RUN_MASK; >>>> + >>>> + if (set_input_sel) >>>> + config |= MISR_CTRL_INPUT_SEL; >>>> + >>>> DPU_REG_WRITE(c, misr_ctrl_offset, config); >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> index bb496ebe283b..793670d62414 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> @@ -17,6 +17,7 @@ >>>> #define MISR_CTRL_ENABLE BIT(8) >>>> #define MISR_CTRL_STATUS BIT(9) >>>> #define MISR_CTRL_STATUS_CLEAR BIT(10) >>>> +#define MISR_CTRL_INPUT_SEL BIT(24) >>> >>> The public apq8916 TRM documents this as a 4-bit field. I think this >>> was followed into the later generations. Can we please document it >>> correctly and use an uint instead of just bool for set_input_sel? >>> >> >> Can you pls point us to this document you are referring? > > I have this link in my bookmarks, which doesn't seem to work no longer: > > https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf?referrer=node/29241 > > 96boards forum has several links and mentions of this doc. > >> >> I was not aware that bit level details are revealed in external >> documents :) >> >> Even though its a 4-bit field, it only takes a 0 or 1 as others are >> undefined. >> >> Exposing all the bits will only cause more confusion like it did for >> others thinking that input select is actually configurable when its not. >> >> I think what we should do is just pass "misr_type" to this API to tell >> whether its lm misr or intf misr and set BIT(24) based on that. > > This would be another simplification. Can we instead just use values 0 > and 1 instead and maybe document that by default everybody should use 0. Hi Dmitry, Acked. Will change the input_sel parameter to a u8 and add a note that it should be 0x0 by default with an exception for encoders. Thanks, Jessica Zhang > >> >> >>>> #define MISR_CTRL_FREE_RUN_MASK BIT(31) >>>> >>>> /* >>>> @@ -357,7 +358,8 @@ void _dpu_hw_setup_qos_lut(struct >>>> dpu_hw_blk_reg_map *c, u32 offset, >>>> bool qos_8lvl, >>>> const struct dpu_hw_qos_cfg *cfg); >>>> >>>> -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>>> misr_ctrl_offset); >>>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 >>>> misr_ctrl_offset, >>>> + bool set_input_sel); >>>> >>>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, >>>> u32 misr_ctrl_offset, >>>> >>>> -- >>>> 2.43.0 >>>> >>> >>> > > -- > With best wishes > Dmitry >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 3442cf65b86f..d0884997ecb7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -320,7 +320,7 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf) { - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL); + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, true); } static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c index f38473e68f79..77b14107c84a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx) { - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL); + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, false); } static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c index a8a0a4e76b94..f441df47fdde 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c @@ -481,7 +481,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, cfg->danger_safe_en ? QOS_QOS_CTRL_DANGER_SAFE_EN : 0); } -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, + bool set_input_sel) { u32 config = 0; @@ -491,6 +492,10 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset) wmb(); config = MISR_FRAME_COUNT | MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; + + if (set_input_sel) + config |= MISR_CTRL_INPUT_SEL; + DPU_REG_WRITE(c, misr_ctrl_offset, config); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h index bb496ebe283b..793670d62414 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h @@ -17,6 +17,7 @@ #define MISR_CTRL_ENABLE BIT(8) #define MISR_CTRL_STATUS BIT(9) #define MISR_CTRL_STATUS_CLEAR BIT(10) +#define MISR_CTRL_INPUT_SEL BIT(24) #define MISR_CTRL_FREE_RUN_MASK BIT(31) /* @@ -357,7 +358,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, bool qos_8lvl, const struct dpu_hw_qos_cfg *cfg); -void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset); +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, + bool set_input_sel); int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset,
Set the input_sel bit for encoders as it was missed in the initial implementation. Reported-by: Rob Clark <robdclark@gmail.com> Fixes: 91143873a05d ("drm/msm/dpu: Add MISR register support for interface") Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/39 Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 7 ++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +++- 4 files changed, 11 insertions(+), 4 deletions(-)