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

[Dmitry Baryshkov]

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.

> > > +
> > >       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.

> > > +
> > > +    if (cfg->dsc)
> > >           DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> > > -    }
> > >   }
> > >   static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> > > @@ -630,6 +638,8 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops 
> > > *ops,
> > >           ops->update_pending_flush_merge_3d =
> > >               dpu_hw_ctl_update_pending_flush_merge_3d_v1;
> > >           ops->update_pending_flush_wb = 
> > > dpu_hw_ctl_update_pending_flush_wb_v1;
> > > +        ops->update_pending_flush_dsc =
> > > +            dpu_hw_ctl_update_pending_flush_dsc_v1;
> > >       } else {
> > >           ops->trigger_flush = dpu_hw_ctl_trigger_flush;
> > >           ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg;
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h 
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > index 6292002..d5f3ef8 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops {
> > >           enum dpu_dspp blk, u32 dspp_sub_blk);
> > >       /**
> > > +     * OR in the given flushbits to the cached pending_(dsc_)flush_mask
> > > +     * No effect on hardware
> > > +     * @ctx: ctl path ctx pointer
> > > +     * @blk: interface block index
> > > +     */
> > > +    void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx,
> > > +        enum dpu_dsc blk);
> > > +
> > > +    /**
> > >        * Write the value of the pending_flush_mask to hardware
> > >        * @ctx       : ctl path ctx pointer
> > >        */
> > > @@ -229,6 +238,9 @@ struct dpu_hw_ctl_ops {
> > >    * @pending_flush_mask: storage for pending ctl_flush managed via ops
> > >    * @pending_intf_flush_mask: pending INTF flush
> > >    * @pending_wb_flush_mask: pending WB flush
> > The above is all capitalized, so...:
> >
> > > + * @pending_merge_3d_flush_mask: pending merge_3d flush
> > MERGE_3D?
> >
> > > + * @pending_dspp_flush_mask: pending dspp flush
> > DSPP
> >
> > > + * @pending_dsc_flush_mask: pending dsc flush
> > DSC
> >
> > - Marijn
> >
> > >    * @ops: operation list
> > >    */
> > >   struct dpu_hw_ctl {
> > > @@ -245,6 +257,7 @@ struct dpu_hw_ctl {
> > >       u32 pending_wb_flush_mask;
> > >       u32 pending_merge_3d_flush_mask;
> > >       u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0];
> > > +    u32 pending_dsc_flush_mask;
> > >       /* ops */
> > >       struct dpu_hw_ctl_ops ops;
> > > --
> > > 2.7.4

--
With best wishes
Dmitry