Re: [PATCH v10 6/8] drm/msm/dpu: separate DSC flush update out of interface

[Marijn Suijten]

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>