Re: [PATCH 1/2] drm/msm/dpu: fix DSC 1.2 block lengths

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 23/06/2023 08:47, Abhinav Kumar wrote:
> On 6/22/2023 6:37 PM, Dmitry Baryshkov wrote:
> > All DSC_BLK_1_2 declarations incorrectly pass 0x29c as the block length.
> > This includes the common block itself, enc subblocks and some empty
> > space around. Change that to pass 0x4 instead, the length of common
> > register block itself.
> >
> > Fixes: 0d1b10c63346 ("drm/msm/dpu: add DSC 1.2 hw blocks for relevant 
> > chipsets")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> There is no need of a fixes tag for this.
>
> This is not a bug but was intentional.

We have other subblocks which are not dumped withoyt Ryan's patchset. So 
this declaration should be corrected.

> Till we added sub-block parsing support we had to dump the whole block.
>
> And hence I would suggest this change should be merged after the 
> sub-block parsing change otherwise we wont have full register dumps for 
> DSC.

No, the order should be opposite: this is merged first, then subblocks 
dumping can use block->len in all the cases.

> Also, please add :
>
> Suggested-by: Ryan McCann <quic_rmccann@xxxxxxxxxxx>

More likely:

Reported-by: Ryan McCann <quic_rmccann@xxxxxxxxxxx>

> > ---
> >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h   |  8 ++++----
> >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h   |  2 +-
> >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 12 ++++++------
> >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h   |  8 ++++----
> >   .../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h   |  8 ++++----
> >   5 files changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> > index 8da424eaee6a..6edf323f381f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> > @@ -159,10 +159,10 @@ static const struct dpu_merge_3d_cfg 
> > sm8350_merge_3d[] = {
> >    * its own different sub block address.
> >    */
> >   static const struct dpu_dsc_cfg sm8350_dsc[] = {
> > -    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x29c, 0, dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x29c, 0, dsc_sblk_1),
> > -    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x4, 0, dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x4, 0, dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> >   };
> >   static const struct dpu_intf_cfg sm8350_intf[] = {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> > index 900fee410e11..5354003aa8be 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> > @@ -104,7 +104,7 @@ static const struct dpu_pingpong_cfg sc7280_pp[] = {
> >   /* NOTE: sc7280 only has one DSC hard slice encoder */
> >   static const struct dpu_dsc_cfg sc7280_dsc[] = {
> > -    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> >   };
> >   static const struct dpu_wb_cfg sc7280_wb[] = {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> > index f6ce6b090f71..1d374abec1fd 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> > @@ -148,12 +148,12 @@ static const struct dpu_merge_3d_cfg 
> > sc8280xp_merge_3d[] = {
> >    * its own different sub block address.
> >    */
> >   static const struct dpu_dsc_cfg sc8280xp_dsc[] = {
> > -    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x29c, 0, dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x29c, 0, dsc_sblk_1),
> > -    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> > -    DSC_BLK_1_2("dce_2_0", DSC_4, 0x82000, 0x29c, 0, dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_2_1", DSC_5, 0x82000, 0x29c, 0, dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x4, 0, dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x4, 0, dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_2_0", DSC_4, 0x82000, 0x4, 0, dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_2_1", DSC_5, 0x82000, 0x4, 0, dsc_sblk_1),
> >   };
> >   /* TODO: INTF 3, 8 and 7 are used for MST, marked as INTF_NONE for 
> > now */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> > index 8d13c369213c..79447d8cab05 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> > @@ -167,10 +167,10 @@ static const struct dpu_merge_3d_cfg 
> > sm8450_merge_3d[] = {
> >    * its own different sub block address.
> >    */
> >   static const struct dpu_dsc_cfg sm8450_dsc[] = {
> > -    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x29c, 0, dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x29c, 0, dsc_sblk_1),
> > -    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x4, 0, dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x4, 0, dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> >   };
> >   static const struct dpu_intf_cfg sm8450_intf[] = {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > index f17b9a7fee85..26e3c28003f7 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > @@ -171,10 +171,10 @@ static const struct dpu_merge_3d_cfg 
> > sm8550_merge_3d[] = {
> >    * its own different sub block address.
> >    */
> >   static const struct dpu_dsc_cfg sm8550_dsc[] = {
> > -    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x29c, 0, dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x29c, 0, dsc_sblk_1),
> > -    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > -    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x29c, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_0_0", DSC_0, 0x80000, 0x4, 0, dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_0_1", DSC_1, 0x80000, 0x4, 0, dsc_sblk_1),
> > +    DSC_BLK_1_2("dce_1_0", DSC_2, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_0),
> > +    DSC_BLK_1_2("dce_1_1", DSC_3, 0x81000, 0x4, 
> > BIT(DPU_DSC_NATIVE_42x_EN), dsc_sblk_1),
> >   };
> >   static const struct dpu_intf_cfg sm8550_intf[] = {

--
With best wishes
Dmitry