| Message ID | 20230411-dpu-intf-te-v2-4-ef76c877eb97@somainline.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: Implement tearcheck support on INTF block | expand |
On 4/17/2023 1:21 PM, Marijn Suijten wrote: > SM8550 only comes with a DITHER subblock inside the PINGPONG block, > hence the name and a block length of zero. However, the PP_BLK macro > name was typo'd to DIPHER rather than DITHER. > > Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> This change itself is fine, hence Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> one comment below > --- > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16 ++++++++-------- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- > 2 files changed, 9 insertions(+), 9 deletions(-) > > 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 9e403034093f..d0ab351b6a8b 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 > @@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = { > &sm8150_dspp_sblk), > }; > static const struct dpu_pingpong_cfg sm8550_pp[] = { 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. > - 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, \ >
On 2023-04-24 13:53:13, Abhinav Kumar wrote: > > > On 4/17/2023 1:21 PM, Marijn Suijten wrote: > > SM8550 only comes with a DITHER subblock inside the PINGPONG block, > > hence the name and a block length of zero. However, the PP_BLK macro > > name was typo'd to DIPHER rather than DITHER. > > > > Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550") > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > This change itself is fine, hence > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > one comment below > > > --- > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16 ++++++++-------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- > > 2 files changed, 9 insertions(+), 9 deletions(-) > > > > 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 9e403034093f..d0ab351b6a8b 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 > > @@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = { > > &sm8150_dspp_sblk), > > }; > > static const struct dpu_pingpong_cfg sm8550_pp[] = { > > 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. 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. - 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, \ > >
On 4/24/2023 3:30 PM, Marijn Suijten wrote: > On 2023-04-24 13:53:13, Abhinav Kumar wrote: >> >> >> On 4/17/2023 1:21 PM, Marijn Suijten wrote: >>> SM8550 only comes with a DITHER subblock inside the PINGPONG block, >>> hence the name and a block length of zero. However, the PP_BLK macro >>> name was typo'd to DIPHER rather than DITHER. >>> >>> Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550") >>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >> >> This change itself is fine, hence >> >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >> one comment below >> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16 ++++++++-------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- >>> 2 files changed, 9 insertions(+), 9 deletions(-) >>> >>> 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 9e403034093f..d0ab351b6a8b 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 >>> @@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = { >>> &sm8150_dspp_sblk), >>> }; >>> static const struct dpu_pingpong_cfg sm8550_pp[] = { >> >> 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. > 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. > - 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, \ >>>
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. > > 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, \ > >>>
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. >>> 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, \ >>>>>
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, \ > >>>>>
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, \ >>>>>>>
On 2023-04-25 09:47:30, Abhinav Kumar wrote: > > > 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. Thanks, that'd be great. I wasn't sure at this point what you wanted to be changed here, after commenting on a typo fix rather than i.e. patch 2 that deals with the TE2 sub-block of PP :) > 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. Thanks for checking, so then sm8450 is wrong on multiple occasions. Let's check all other SoCs that use sdm845_pp_sblk whether they should have used sc7280_pp_sblk instead. - Marijn
On 4/25/2023 1:43 PM, Marijn Suijten wrote: > On 2023-04-25 09:47:30, Abhinav Kumar wrote: >> >> >> 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. > > Thanks, that'd be great. I wasn't sure at this point what you wanted to > be changed here, after commenting on a typo fix rather than i.e. patch 2 > that deals with the TE2 sub-block of PP :) > The reason I commented on this patch is because all the discussion so far was surrounding the PP_BLK_DITHER macro which was being touched in this patch. So even now, we found out about sm8450 and sm8550 because of the question that why sm8550 alone should use PP_BLK_DITHER and not sm8450. This patch led to all the discussion about PP_BLK_DITHER. Even though it was just a typo fix patch, it uncovered deeper issues in catalog about why PP_BLK_DITHER wasnt used more often. >> 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. > > Thanks for checking, so then sm8450 is wrong on multiple occasions. > Let's check all other SoCs that use sdm845_pp_sblk whether they should > have used sc7280_pp_sblk instead. > > - Marijn
On 2023-04-25 14:37:21, Abhinav Kumar wrote: > > > On 4/25/2023 1:43 PM, Marijn Suijten wrote: > > On 2023-04-25 09:47:30, Abhinav Kumar wrote: > >> > >> > >> 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. > > > > Thanks, that'd be great. I wasn't sure at this point what you wanted to > > be changed here, after commenting on a typo fix rather than i.e. patch 2 > > that deals with the TE2 sub-block of PP :) > > > > The reason I commented on this patch is because all the discussion so > far was surrounding the PP_BLK_DITHER macro which was being touched in > this patch. > > So even now, we found out about sm8450 and sm8550 because of the > question that why sm8550 alone should use PP_BLK_DITHER and not sm8450. > > This patch led to all the discussion about PP_BLK_DITHER. > > Even though it was just a typo fix patch, it uncovered deeper issues in > catalog about why PP_BLK_DITHER wasnt used more often. Indeed: the initial question was for the dither _block_ which is enabled on every other platform, just through the original macros which do more than exclusively enabling the dither block. > >> But atleast now, someone will remember to do it. I'll see whether I can include these fixes before sending v3 (got all the other changes in and am all-ready to send it): is there any other SoC you're seeing this issue on? - Marijn > >>> 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. > > > > Thanks for checking, so then sm8450 is wrong on multiple occasions. > > Let's check all other SoCs that use sdm845_pp_sblk whether they should > > have used sc7280_pp_sblk instead. > > > > - Marijn
On 4/25/2023 2:53 PM, Marijn Suijten wrote: > On 2023-04-25 14:37:21, Abhinav Kumar wrote: >> >> >> On 4/25/2023 1:43 PM, Marijn Suijten wrote: >>> On 2023-04-25 09:47:30, Abhinav Kumar wrote: >>>> >>>> >>>> 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. >>> >>> Thanks, that'd be great. I wasn't sure at this point what you wanted to >>> be changed here, after commenting on a typo fix rather than i.e. patch 2 >>> that deals with the TE2 sub-block of PP :) >>> >> >> The reason I commented on this patch is because all the discussion so >> far was surrounding the PP_BLK_DITHER macro which was being touched in >> this patch. >> >> So even now, we found out about sm8450 and sm8550 because of the >> question that why sm8550 alone should use PP_BLK_DITHER and not sm8450. >> >> This patch led to all the discussion about PP_BLK_DITHER. >> >> Even though it was just a typo fix patch, it uncovered deeper issues in >> catalog about why PP_BLK_DITHER wasnt used more often. > > Indeed: the initial question was for the dither _block_ which is enabled > on every other platform, just through the original macros which do more > than exclusively enabling the dither block. > >>>> But atleast now, someone will remember to do it. > > I'll see whether I can include these fixes before sending v3 (got all > the other changes in and am all-ready to send it): is there any other > SoC you're seeing this issue on? > Thats alright, you can have it in a separate series not v3 of this one. I am picking up the fixes from this one now. I will update the other SOCs on IRC or even better i will take up this cleanup. > - Marijn > >>>>> 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. >>> >>> Thanks for checking, so then sm8450 is wrong on multiple occasions. >>> Let's check all other SoCs that use sdm845_pp_sblk whether they should >>> have used sc7280_pp_sblk instead. >>> >>> - Marijn
On 2023-04-25 14:55:56, Abhinav Kumar wrote: <snip> > > I'll see whether I can include these fixes before sending v3 (got all > > the other changes in and am all-ready to send it): is there any other > > SoC you're seeing this issue on? > > > > Thats alright, you can have it in a separate series not v3 of this one. > > I am picking up the fixes from this one now. > > I will update the other SOCs on IRC or even better i will take up this > cleanup. I already have the fix patch in my tree that is compatible with the other patches, and will send those in a minute. All DPU >= 7.0.0 seems to be affected, both SM8350 and SM8450 need to use the SC7280 sblk with DITHER V2 at 0xe0 (SM8250 is still V1). I believe SC8280XP should also be updated but do not have access to DTS: where can I find that (what is its codename again?) or can you otherwise confirm this for me? - Marijn
On 4/25/2023 3:15 PM, Marijn Suijten wrote: > On 2023-04-25 14:55:56, Abhinav Kumar wrote: > <snip> >>> I'll see whether I can include these fixes before sending v3 (got all >>> the other changes in and am all-ready to send it): is there any other >>> SoC you're seeing this issue on? >>> >> >> Thats alright, you can have it in a separate series not v3 of this one. >> >> I am picking up the fixes from this one now. >> >> I will update the other SOCs on IRC or even better i will take up this >> cleanup. > > I already have the fix patch in my tree that is compatible with the > other patches, and will send those in a minute. All DPU >= 7.0.0 seems > to be affected, both SM8350 and SM8450 need to use the SC7280 sblk with > DITHER V2 at 0xe0 (SM8250 is still V1). I believe SC8280XP should also > be updated but do not have access to DTS: where can I find that (what is > its codename again?) or can you otherwise confirm this for me? > Sure, I can wait another day too. Dont want to rush you too much for this. 8280xp still has dither at 0xe0 and yes its version is V2. 8280xp's DTS is not located in the techpack. Its a different tree. > - Marijn
On 2023-04-25 15:37:44, Abhinav Kumar wrote: > > > On 4/25/2023 3:15 PM, Marijn Suijten wrote: > > On 2023-04-25 14:55:56, Abhinav Kumar wrote: > > <snip> > >>> I'll see whether I can include these fixes before sending v3 (got all > >>> the other changes in and am all-ready to send it): is there any other > >>> SoC you're seeing this issue on? > >>> > >> > >> Thats alright, you can have it in a separate series not v3 of this one. > >> > >> I am picking up the fixes from this one now. > >> > >> I will update the other SOCs on IRC or even better i will take up this > >> cleanup. > > > > I already have the fix patch in my tree that is compatible with the > > other patches, and will send those in a minute. All DPU >= 7.0.0 seems > > to be affected, both SM8350 and SM8450 need to use the SC7280 sblk with > > DITHER V2 at 0xe0 (SM8250 is still V1). I believe SC8280XP should also > > be updated but do not have access to DTS: where can I find that (what is > > its codename again?) or can you otherwise confirm this for me? > > > > Sure, I can wait another day too. Dont want to rush you too much for this. Thank you. There are even more Fixes: patches now as well as some small wording cleanups. > 8280xp still has dither at 0xe0 and yes its version is V2. Thanks! Added that info now. Is the PP blk still length 0xd4? > 8280xp's DTS is not located in the techpack. Its a different tree. A tree that's... not public? - Marijn
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 9e403034093f..d0ab351b6a8b 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 @@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = { &sm8150_dspp_sblk), }; static const struct dpu_pingpong_cfg sm8550_pp[] = { - 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, \
SM8550 only comes with a DITHER subblock inside the PINGPONG block, hence the name and a block length of zero. However, the PP_BLK macro name was typo'd to DIPHER rather than DITHER. Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550") Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16 ++++++++-------- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-)