| Message ID | 20230623013731.1088007-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] drm/msm/dpu: fix DSC 1.2 block lengths | expand |
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@linaro.org> There is no need of a fixes tag for this. This is not a bug but was intentional. 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. Also, please add : Suggested-by: Ryan McCann <quic_rmccann@quicinc.com> > --- > .../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[] = {
On 2023-06-22 22:47:04, 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@linaro.org> > > There is no need of a fixes tag for this. > > This is not a bug but was intentional. > > 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. This was indeed intentional, we discussed it in [1]. In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover the CTL registers, but that change didn't make it through. 0x29c is an arbitrary number that I have no clue what it was based on. [1]: https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ - Marijn
On 2023-06-23 08:54:39, Marijn Suijten wrote: > On 2023-06-22 22:47:04, 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@linaro.org> > > > > There is no need of a fixes tag for this. > > > > This is not a bug but was intentional. > > > > 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. > > This was indeed intentional, we discussed it in [1]. > > In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover > the CTL registers, but that change didn't make it through. 0x29c is an > arbitrary number that I have no clue what it was based on. Ah, as expected Dmitry's second commit clarifies that the last register in the enc block is at 0x98, and the base of the enc + length of the enc then is 0x200 + 0x9c. That still excludes the ctl sblk. > [1]: https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ > > - Marijn
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@linaro.org> > > 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@quicinc.com> More likely: Reported-by: Ryan McCann <quic_rmccann@quicinc.com> > > >> --- >> .../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[] = {
On 23/06/2023 09:54, Marijn Suijten wrote: > On 2023-06-22 22:47:04, 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@linaro.org> >> >> There is no need of a fixes tag for this. >> >> This is not a bug but was intentional. >> >> 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. > > This was indeed intentional, we discussed it in [1]. > > In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover > the CTL registers, but that change didn't make it through. 0x29c is an > arbitrary number that I have no clue what it was based on. This should have been NAKed. or at least TODOed. > > [1]: https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ > > - Marijn
On 6/23/2023 4:25 AM, Dmitry Baryshkov wrote: > 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@linaro.org> >> >> 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. > As registers were not contiguous, some of them had to be missed but the goal was to cover as much as possible with the len of the main blk. Some registers had to take a hit till we dumped sub-blocks. >> >> 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. > Please stop pushing changes in the middle of an ongoing series. If you really wanted this, we could have expanded the sub-block series to fix this too or you could have discussed with the authors that you were going to push this in parallel. Instead of helping developers, it sometimes offends them to receive patches in the middle of an ongoing series. >> >> Also, please add : >> >> Suggested-by: Ryan McCann <quic_rmccann@quicinc.com> > + /* For now, pass in a length of 0 because the DSC_BLK register space + * overlaps with the sblks' register space. + * + * TODO: Pass in a length of 0 t0 DSC_BLK_1_2 in the HW catalog where + * applicable. The comment reports and tells what to do. I thought of suggesting to add both first. > More likely: > > Reported-by: Ryan McCann <quic_rmccann@quicinc.com> > >> >> >>> --- >>> .../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[] = { >
On 6/22/2023 11:57 PM, Marijn Suijten wrote: > On 2023-06-23 08:54:39, Marijn Suijten wrote: >> On 2023-06-22 22:47:04, 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@linaro.org> >>> >>> There is no need of a fixes tag for this. >>> >>> This is not a bug but was intentional. >>> >>> 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. >> >> This was indeed intentional, we discussed it in [1]. >> >> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover >> the CTL registers, but that change didn't make it through. 0x29c is an >> arbitrary number that I have no clue what it was based on. > > Ah, as expected Dmitry's second commit clarifies that the last register > in the enc block is at 0x98, and the base of the enc + length of the enc > then is 0x200 + 0x9c. > > That still excludes the ctl sblk. 0x29c is not an arbitrary number. The last encoder offset is 0x298 so we add 4 more to that. Yes it will still exclude ctl blk as that space is not contiguous and we dont want to increase len all the way to 0xf00. > >> [1]: https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ >> >> - Marijn
On 6/23/2023 4:37 AM, Dmitry Baryshkov wrote: > On 23/06/2023 09:54, Marijn Suijten wrote: >> On 2023-06-22 22:47:04, 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@linaro.org> >>> >>> There is no need of a fixes tag for this. >>> >>> This is not a bug but was intentional. >>> >>> 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. >> >> This was indeed intentional, we discussed it in [1]. >> >> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover >> the CTL registers, but that change didn't make it through. 0x29c is an >> arbitrary number that I have no clue what it was based on. > > This should have been NAKed. or at least TODOed. > Its is not an arbitrary number. Thats an incorrect comment. Its 4 more than the encoder's last offset which is 0x298 + 0x4 = 0x29c. There was nothing to NAK or TODO here. >> >> [1]: >> https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ >> >> - Marijn >
On 23/06/2023 22:37, Abhinav Kumar wrote: > > > On 6/23/2023 4:37 AM, Dmitry Baryshkov wrote: >> On 23/06/2023 09:54, Marijn Suijten wrote: >>> On 2023-06-22 22:47:04, 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@linaro.org> >>>> >>>> There is no need of a fixes tag for this. >>>> >>>> This is not a bug but was intentional. >>>> >>>> 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. >>> >>> This was indeed intentional, we discussed it in [1]. >>> >>> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover >>> the CTL registers, but that change didn't make it through. 0x29c is an >>> arbitrary number that I have no clue what it was based on. >> >> This should have been NAKed. or at least TODOed. >> > > Its is not an arbitrary number. Thats an incorrect comment. > > Its 4 more than the encoder's last offset which is 0x298 + 0x4 = 0x29c. > > There was nothing to NAK or TODO here. We do not include sub-blocks in the main block area. The SSPP's SRC blocks were cleaned up for this reason. The ENC registers are definitely a sub-block (and are described this way). There should have been a "TODO: reduce block length to 0x4 after adding sub-blocks to dump" comment. > >>> >>> [1]: >>> https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ >>> >>> - Marijn >>
On 6/23/2023 12:40 PM, Dmitry Baryshkov wrote: > On 23/06/2023 22:37, Abhinav Kumar wrote: >> >> >> On 6/23/2023 4:37 AM, Dmitry Baryshkov wrote: >>> On 23/06/2023 09:54, Marijn Suijten wrote: >>>> On 2023-06-22 22:47:04, 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@linaro.org> >>>>> >>>>> There is no need of a fixes tag for this. >>>>> >>>>> This is not a bug but was intentional. >>>>> >>>>> 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. >>>> >>>> This was indeed intentional, we discussed it in [1]. >>>> >>>> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover >>>> the CTL registers, but that change didn't make it through. 0x29c is an >>>> arbitrary number that I have no clue what it was based on. >>> >>> This should have been NAKed. or at least TODOed. >>> >> >> Its is not an arbitrary number. Thats an incorrect comment. >> >> Its 4 more than the encoder's last offset which is 0x298 + 0x4 = 0x29c. >> >> There was nothing to NAK or TODO here. > > We do not include sub-blocks in the main block area. The SSPP's SRC > blocks were cleaned up for this reason. The ENC registers are definitely > a sub-block (and are described this way). There should have been a > "TODO: reduce block length to 0x4 after adding sub-blocks to dump" comment. > iirc the sub-block dump idea came to me in some other patchset not this one. But ack that a comment could have been left. >> >>>> >>>> [1]: >>>> https://lore.kernel.org/linux-arm-msm/y2whfntyo2rbrg3taazjdw5sijle6k6swzl4uutcxm6tmuayh4@uxdur74uasua/ >>>> >>>> - Marijn >>> >
On 2023-06-23 12:36:06, Abhinav Kumar wrote: > > > On 6/22/2023 11:57 PM, Marijn Suijten wrote: > > On 2023-06-23 08:54:39, Marijn Suijten wrote: > >> On 2023-06-22 22:47:04, 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@linaro.org> > >>> > >>> There is no need of a fixes tag for this. > >>> > >>> This is not a bug but was intentional. > >>> > >>> 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. > >> > >> This was indeed intentional, we discussed it in [1]. > >> > >> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover > >> the CTL registers, but that change didn't make it through. 0x29c is an > >> arbitrary number that I have no clue what it was based on. > > > > Ah, as expected Dmitry's second commit clarifies that the last register > > in the enc block is at 0x98, and the base of the enc + length of the enc > > then is 0x200 + 0x9c. > > > > That still excludes the ctl sblk. > > 0x29c is not an arbitrary number. The last encoder offset is 0x298 so we > add 4 more to that. That is literally what I said in this correction followup ;) > Yes it will still exclude ctl blk as that space is not contiguous and we > dont want to increase len all the way to 0xf00. Sure, it's quite a lot of "dead space" to have in-between. Looking forward to having better dumpers. - Marijn
On 2023-06-23 14:37:12, Dmitry Baryshkov wrote: <snip> > > In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover > > the CTL registers, but that change didn't make it through. 0x29c is an > > arbitrary number that I have no clue what it was based on. > > This should have been NAKed. or at least TODOed. As usual ;) - add new features first, fix the fundamentals... later? - Marijn
On 6/23/2023 1:28 PM, Marijn Suijten wrote: > On 2023-06-23 14:37:12, Dmitry Baryshkov wrote: > <snip> >>> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover >>> the CTL registers, but that change didn't make it through. 0x29c is an >>> arbitrary number that I have no clue what it was based on. >> >> This should have been NAKed. or at least TODOed. > > As usual ;) - add new features first, fix the fundamentals... later? > > - Marijn I think you yourself found out that this was not an arbitary number but we atleast wanted to cover the full encoder set. So fundamentals are always sound sometimes understanding is not ;)
On 2023-06-23 15:12:06, Abhinav Kumar wrote: > > > On 6/23/2023 1:28 PM, Marijn Suijten wrote: > > On 2023-06-23 14:37:12, Dmitry Baryshkov wrote: > > <snip> > >>> In fact I asked to make it 0xf00 + 0x10 or 0xf80 + 0x10 to also cover > >>> the CTL registers, but that change didn't make it through. 0x29c is an > >>> arbitrary number that I have no clue what it was based on. > >> > >> This should have been NAKed. or at least TODOed. > > > > As usual ;) - add new features first, fix the fundamentals... later? > > > > - Marijn > > I think you yourself found out that this was not an arbitary number but > we atleast wanted to cover the full encoder set. > > So fundamentals are always sound sometimes understanding is not ;) The fundamentals are not sound until the CTL block/register can be dumped ;) - Marijn
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[] = {
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@linaro.org> --- .../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(-)