Messages in this thread | | | Date | Sat, 2 Dec 2023 21:54:51 +0200 | Subject | Re: [PATCH 2/2] drm/msm/dpu: Set input_sel bit for INTF | From | Dmitry Baryshkov <> |
| |
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 >>> >> >>
-- With best wishes Dmitry
| |