On 2023-05-19 01:40:19, Dmitry Baryshkov wrote:
>
> On 19/05/2023 01:09, Kuogee Hsieh wrote:
> >
> > On 5/17/2023 3:31 PM, Marijn Suijten wrote:
> >>
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> >>> @@ -139,6 +139,11 @@ static inline void
> >>> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
> >>> CTL_DSPP_n_FLUSH(dspp - DSPP_0),
> >>> ctx->pending_dspp_flush_mask[dspp - DSPP_0]);
> >>> }
> >>> +
> >>> + if (ctx->pending_flush_mask & BIT(DSC_IDX))
> >>> + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH,
> >>> + ctx->pending_dsc_flush_mask);
> >> Again, when do we reset this mask to 0? (v8 review)
> >
> > can not find it.
> >
> > let me add a separate patch to fix this.
>
> The pending_dsc_flush_mask was added in this patch, so the reset should
> be a part of this patch too.
Yes, same patch.
Related question I asked in v8: only the global pending_flush_mask and
pending_dspp_flush_mask are reset in dpu_hw_ctl_clear_pending_flush().
Shall I send a patch to clear the other missing ones (e.g. merge_3d etc)
as well?
> >>> +
> >>> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
> >>> }
> >>> @@ -285,6 +290,13 @@ static void
> >>> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
> >>> ctx->pending_flush_mask |= BIT(MERGE_3D_IDX);
> >>> }
> >>> +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl
> >>> *ctx,
> >>> + enum dpu_dsc dsc_num)
> >>> +{
> >>> + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0);
> >>> + ctx->pending_flush_mask |= BIT(DSC_IDX);
> >>> +}
> >>> +
> >>> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl
> >>> *ctx,
> >>> enum dpu_dspp dspp, u32 dspp_sub_blk)
> >>> {
> >>> @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
> >>> dpu_hw_ctl *ctx,
> >>> if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features)))
> >>> mode_sel = CTL_DEFAULT_GROUP_ID << 28;
> >>> - if (cfg->dsc)
> >>> - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc);
> >>> -
> >>> if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
> >>> mode_sel |= BIT(17);
> >>> @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
> >>> dpu_hw_ctl *ctx,
> >>> if (cfg->merge_3d)
> >>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
> >>> BIT(cfg->merge_3d - MERGE_3D_0));
> >>> - if (cfg->dsc) {
> >>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> >> Again, this bugfix of now wrapping DSC_IDX in BIT() should go in a
> >> separate Fixes: patch to have this semantic change documented. (v8
> >> review)
> > That will be this patch. let me add Fixes at this patch
>
> _separate_ patch.
Separate patch, and documenting clearly what happens and why. Kuogee, I
can send this as well if it makes things more clear, since it doesn't
seem (from the patch description) that anyone noticed the
implication/bugfix in this change as a drive-by effect of porting
sde_hw_ctl_update_bitmask_dsc_v1() from downstream.
- Marijn
<snip>
