Re: [PATCH v5 3/7] drm/msm/dpu: add DPU_PINGPONG_DSC bits into PP_BLK and PP_BLK_TE marcos

[Abhinav Kumar]

On 5/4/2023 11:23 AM, Marijn Suijten wrote:
> On 2023-05-04 20:53:33, Dmitry Baryshkov wrote:
> > On Thu, 4 May 2023 at 20:49, Marijn Suijten
> > <marijn.suijten@xxxxxxxxxxxxxx> wrote:
> > > PP_BLK_TE is no longer there.
> > >
> > > marcos -> macros.
> > >
> > > On 2023-05-04 09:46:41, Kuogee Hsieh wrote:
> > > > At legacy chipsets, it required DPU_PINGPONG_DSC bit be set to indicate
> > >
> > > I may have not made this clear, but the comments on patch 2/7
> > > (introducing the DPU_PINGPONG_DSC bit) also apply to this patch: clarify
> > > DPU 7.0.0 exactly in favour of "legacy", which has no definition at all
> > > and changes over time.
> > >
> > > > pingpong ops functions are required to complete DSC data path setup if
> > > > this chipset has DSC hardware block presented. This patch add
> > > > DPU_PINGPONG_DSC bit to both PP_BLK and PP_BLK_TE marcos if it has DSC
> > > > hardware block presented.
> > >
> > > Strictly speaking this patch together with 2/7 is not bisectable, as 2/7
> > > first disables the callbacks for _all_ hardware and then this patch adds
> > > it back by adding the flag to all DPU < 7.0.0 catalog descriptions.
> >
> > I asked to split these into two patches, but I see your point and
> > partially agree with it. However if we mix the catalog changes with
> > functional changes, it is too easy to overlook or misjudge the
> > functional changes.
>
> I did the same in the INTF TE series for patches that have very little
> and/or very obvious functional changes: exactly this combination of
> guarding a few callbacks behind a feature bit, and setting that feature
> bit on a few specific catalog entries.
>
> > As you are correct about bisectability, I'd probably suggest either
> > having three patches (define flag, update catalog, handle flag in the
> > driver) or squashing first two patches to have two patches (add flag +
> > catalog, separate functional changes).
>
> Sure, if you really prefer a split I'd go for two patches:
> 1. Add the flag to the enum and catalog;
> 2. Add the ops guard (functional change).
>
> Then don't forget to reword the commit message, following the guidelines
> below and the suggestion for 2/7.
>
> - Marijn

Plan sounds good to me.

Marijn, we will wait for a couple of days to post the next rev but would 
be hard more than that as we need to pick up other things which are 
pending on top of this. Hence would appreciate if you can finish reviews 
by then.

> > > To solve that, as we do in other DPU patch-series, just squash this
> > > patch into 2/7.  That way you also don't have to spend extra time
> > > rewording this commit message either to match the improvements we made
> > > in 2/7 (for example, you mention that "ops functions are required to
> > > complete DSC data path setup", but those were already available before
> > > 2/7, despite sounding as if this is a new thing that was previously
> > > missing entirely).
> > >
> > > But please wait at least a couple days before sending v6.  I only have a
> > > few hours every day/week but would appreciate to review and test all the
> > > other patches.
> > >
> > > > Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> > > > ---
> > > >   .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h    | 16 +++++++--------
> > > >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 16 +++++++--------
> > > >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 24 +++++++++++-----------
> > > >   .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h    | 24 +++++++++++-----------
> > > >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 24 +++++++++++-----------
> > > >   5 files changed, 52 insertions(+), 52 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> > > > index 521cfd5..ef92545 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> > > > @@ -112,17 +112,17 @@ static const struct dpu_lm_cfg msm8998_lm[] = {
> > > >   };
> > > >
> > > >   static const struct dpu_pingpong_cfg msm8998_pp[] = {
> > > > -     PP_BLK("pingpong_0", PINGPONG_0, 0x70000, PINGPONG_SDM845_TE2_MASK, 0, sdm845_pp_sblk_te,
> > > > -                     DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
> > > > +     PP_BLK("pingpong_0", PINGPONG_0, 0x70000, PINGPONG_SDM845_TE2_MASK|BIT(DPU_PINGPONG_DSC),
> > >
> > > This should be added to the MASK (add new #define's where necessary).
> > >
> > > - Marijn
>
> <snip>