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

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

[Dmitry Baryshkov]

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(-)

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

[PATCH 2/2] drm/msm/dpu: fix DSC 1.2 enc subblock length

[Dmitry Baryshkov]

Both struct dpu_dsc_sub_blks instances declare enc subblock length to be
0x100, while the actual length is 0x9c (last register having offset 0x98).
Reduce subblock length to remove the empty register space from being
dumped.

Fixes: 0d1b10c63346 ("drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 0de507d4d7b7..dd2f89ada043 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -517,12 +517,12 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
  * DSC sub blocks config
  *************************************************************/
 static const struct dpu_dsc_sub_blks dsc_sblk_0 = {
-	.enc = {.base = 0x100, .len = 0x100},
+	.enc = {.base = 0x100, .len = 0x9c},
 	.ctl = {.base = 0xF00, .len = 0x10},
 };
 
 static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
-	.enc = {.base = 0x200, .len = 0x100},
+	.enc = {.base = 0x200, .len = 0x9c},
 	.ctl = {.base = 0xF80, .len = 0x10},
 };
 
-- 
2.39.2

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

[Abhinav Kumar]

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

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

[Marijn Suijten]

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

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

[Marijn Suijten]

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

Re: [PATCH 2/2] drm/msm/dpu: fix DSC 1.2 enc subblock length

[Marijn Suijten]

On 2023-06-23 04:37:31, Dmitry Baryshkov wrote:
> Both struct dpu_dsc_sub_blks instances declare enc subblock length to be
> 0x100, while the actual length is 0x9c (last register having offset 0x98).
> Reduce subblock length to remove the empty register space from being
> dumped.
> 
> Fixes: 0d1b10c63346 ("drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 0de507d4d7b7..dd2f89ada043 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -517,12 +517,12 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
>   * DSC sub blocks config
>   *************************************************************/
>  static const struct dpu_dsc_sub_blks dsc_sblk_0 = {
> -	.enc = {.base = 0x100, .len = 0x100},
> +	.enc = {.base = 0x100, .len = 0x9c},
>  	.ctl = {.base = 0xF00, .len = 0x10},

Hmm, these hexadecimals are still uppercase.  When do we get a
formatter/linter that automatically catches and fixes these?

- Marijn

>  };
>  
>  static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
> -	.enc = {.base = 0x200, .len = 0x100},
> +	.enc = {.base = 0x200, .len = 0x9c},
>  	.ctl = {.base = 0xF80, .len = 0x10},
>  };
>  
> -- 
> 2.39.2
> 

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

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

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

[Abhinav Kumar]

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

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

[Abhinav Kumar]

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
> 

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

[Dmitry Baryshkov]

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
>>

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

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
>>>
> 

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

[Marijn Suijten]

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

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

[Marijn Suijten]

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

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

[Abhinav Kumar]

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 ;)

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

[Marijn Suijten]

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

Re: [PATCH 2/2] drm/msm/dpu: fix DSC 1.2 enc subblock length

[Abhinav Kumar]

On 6/22/2023 6:37 PM, Dmitry Baryshkov wrote:
> Both struct dpu_dsc_sub_blks instances declare enc subblock length to be
> 0x100, while the actual length is 0x9c (last register having offset 0x98).
> Reduce subblock length to remove the empty register space from being
> dumped.
> 
> Fixes: 0d1b10c63346 ("drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>