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.
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.
- 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, \
> >>>>>
