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

Fri Jun 23 17:41:38 UTC 2023

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 at 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 at 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 at 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[] = {
> 