Re: [Freedreno] [PATCH v3] drm/msm/dpu: always program DSC active bits

[Abhinav Kumar]

Hi Dmitry / Marijn

On 4/20/2023 7:03 AM, Dmitry Baryshkov wrote:
> On 15/04/2023 02:02, Marijn Suijten wrote:
> > On 2023-04-14 09:46:17, Kuogee Hsieh wrote:
> > > In current code, the dsc active bits are set only if the cfg->dsc is 
> > > set.
> >
> > This is the old sentence from v1 again, did you accidentally send the
> > wrong patch as the improvements from v2 are missing?
> >
> > > However, for displays which are hot-pluggable, there can be a use-case
> > > of disconnecting a DSC supported sink and connecting a non-DSC sink.
> > >
> > > For those cases we need to clear DSC active bits during teardown.
> >
> > At least teardown is one word again, v2 had "tear down" which is wrong.
> >
> > > As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()
> >
> > discussed* as pointed out by Dmitry, and make it clear that this is
> > about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically.  Once
> > that is moved to reset_intf_cfg(), this patch should be reverted as
> > there is no need to write the registers once again when cfg->dsc equals
> > 0.
>
> Kuogee, can we please get a proper v4? With all the relevant changes 
> from v2, with the changelog, etc.
>
> Otherwise the present Reviewed-by tags are just incorrect.

After looking into the DPU DSC changes internally which will be posted 
today/tomm, that piece of code is again touching this block, so I am now 
also not convinced this change should be made right now because it was 
again touching flush programming, so that again leaves only the active 
bits which as Marijn mentioned will be applicable for a use-case only 
with hot-pluggable display which we dont have till we land DP DSC.

I think this is what we will do:

-> post DPU DSC changes
-> post the patch to improve reset_intf_cfg() to handle DSC
-> post rest of DP DSC changes
-> post rest of DSI DSC changes

I think that way, we all are aligned.

Apologies for posting this patch a bit ahead of time but if i had 
foreseen that DPU DSC changes will again touch the flush code, i would 
have held this patch back too.

> > - Marijn
> >
> > > Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> > > Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
> > > Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> > >
> > > [1] 
> > > https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@xxxxxxxxxxx/ 
> > >
> > > ---
> > >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c 
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > index bbdc95c..88e4efe 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > @@ -541,10 +541,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);
> > > -        DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> > > -    }
> > > +
> > > +    DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> > > +    DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
> > >   }
> > >   static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> > > Forum,
> > > a Linux Foundation Collaborative Project