Re: [Freedreno] [PATCH v2 04/17] drm/msm/dpu: Fix PP_BLK_DIPHER -> DITHER typo

From: Abhinav Kumar
Date: Tue Apr 25 2023 - 12:48:43 EST



On 4/25/2023 9:33 AM, Marijn Suijten wrote:
On 2023-04-25 09:18:58, Abhinav Kumar wrote:


On 4/24/2023 11:54 PM, Marijn Suijten wrote:
On 2023-04-24 16:09:45, Abhinav Kumar wrote:
<snip>
dither block should be present on many other chipsets too but looks like
on sm8550 was enabling it. Not sure how it was validated there. But we
are enabling dither, even other chipsets have this block.

Correct, they all seem to have it starting at sdm845. My patch message
seems to lack the word "exclusively" as the PP on sm8550 appears to
exclusively contain a DITHER subblock (unless other blocks are available
that simply aren't supported within this driver yet) and no other
registers. Hence this aptly named macro exist to emit just the feature
bitflag for that and a .len of zero.


I think after the TE blocks were moved to INTF, dither is the only
sub-block for all Ping-Pongs not just in sm8550.

So you are asking / leaving context to make all >= 5.0.0 pingpong blocks
use this macro with only a single DITHER sblk in PP?

As far as I recall SM8550 is the first SoC to use zero registers in PP,
which is specifically what this macro takes care of too. Then, there
are only a few SoCs downstream still (erroneously?) referencing TE2 as
the only other sub-blk, those SoCs still use sdm845_pp_sblk_te.


So, what I didnt follow is why should sm8450 use PP_BLK_TE Vs sm8550
should use PP_BLK_DIPHER?

Atleast for those two, both should be using PP_BLK_DIPHER.

Thats what I was trying to note here.

This isnt even right as there is no PP_BLK_TE in sm8450.

SM8450 doesn't use PP_BLK_TE (TE2) anymore since the second patch in
this series. If you think it should use the DITHER (not DIPHER!) macro
instead of the regular PP_BLK with a size of 0xd4, we can do that in
another patch as that's not strictly related to this series.


Yes, thanks for pointing the TE2 was removed in the prev patch of this series for sm8450. I was just focusing too much on this patch.

And Yes, I think we should use the DIPHER ..... oh sorry .... DITHER ;)

Yes, it can go as a different series, like I already wrote many times in this.

But atleast now, someone will remember to do it.

Note that that's the only difference between these macros. The size
becomes 0 but the .features mask is the same (SM8450 uses
PINGPONG_SM8150_MASK).

These patches are anyway already distracting from my series, but were
easier to do in one go as I was touching the PP and INTF catalog blocks
regardless.

While at it, perhaps we should check if the version and offset for the
DITHER block are correct? SM8450 uses SDM845 sblk definitions.


Yes I already checked. the version and offset of dither are same between sm8450 and sm8550.

- Marijn

Now, whether we should have the features contain subblock flags rather
than just scanning for their id's or presence in the subblocks is a
different discussion / cleanup we should have.


Yes, separate patch and hence I gave R-b on this one. But had to leave
this comment to not lose context.

Fwiw this is a different suggestion: we already have these flags in the
sub-block `.id` field so there seems to be no reason to duplicate info
in the top-level `.features` field, deduplicating some info and
simplifying some defines.

- Marijn

- Marijn

- PP_BLK_DIPHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0, sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
-1),
- PP_BLK_DIPHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0, sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
-1),
- PP_BLK_DIPHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1, sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
-1),
- PP_BLK_DIPHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1, sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
-1),
- PP_BLK_DIPHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2, sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30),
-1),
- PP_BLK_DIPHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2, sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31),
-1),
- PP_BLK_DIPHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3, sc7280_pp_sblk,
-1,
-1),
- PP_BLK_DIPHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3, sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3, sc7280_pp_sblk,
-1,
-1),
};
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 03f162af1a50..ca8a02debda9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -491,7 +491,7 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
.len = 0x20, .version = 0x20000},
};
-#define PP_BLK_DIPHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
+#define PP_BLK_DITHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
{\
.name = _name, .id = _id, \
.base = _base, .len = 0, \