Continue migration to the MDSS-revision based checks and replace
DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 ++--------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
7 files changed, 5 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h
index c6bf3bca200d268912ae92cb8399a7e82b0d5ae8..14069958a71141815dc3722b00900c4659c1efab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h
@@ -100,14 +100,12 @@ static const struct dpu_pingpong_cfg msm8937_pp[] = {
{
.name = "pingpong_0", .id = PINGPONG_0,
.base = 0x70000, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
}, {
.name = "pingpong_1", .id = PINGPONG_1,
.base = 0x70800, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h
index bebdba68667aaf79399da8ba810ca10d70ac430f..0d43041e727e13e7a364c35090f65405c74cab32 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h
@@ -93,7 +93,6 @@ static const struct dpu_pingpong_cfg msm8917_pp[] = {
{
.name = "pingpong_0", .id = PINGPONG_0,
.base = 0x70000, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h
index 598113bd59f1bf33dcf0c25ecdd81057ddf1029e..d7e8fed190800324cd4cf245fd258ef8c3187a93 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h
@@ -100,14 +100,12 @@ static const struct dpu_pingpong_cfg msm8953_pp[] = {
{
.name = "pingpong_0", .id = PINGPONG_0,
.base = 0x70000, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
}, {
.name = "pingpong_1", .id = PINGPONG_1,
.base = 0x70800, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h
index 6dfbd843c3b1cb0d972baab9eb463ecbb334f075..25fa0bd574894ef4d11b14af0c0ef386539e121f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h
@@ -181,28 +181,26 @@ static const struct dpu_pingpong_cfg msm8996_pp[] = {
{
.name = "pingpong_0", .id = PINGPONG_0,
.base = 0x70000, .len = 0xd4,
- .features = PINGPONG_MSM8996_TE2_MASK,
+ .features = BIT(DPU_PINGPONG_TE2),
.sblk = &msm8996_pp_sblk_te,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
}, {
.name = "pingpong_1", .id = PINGPONG_1,
.base = 0x70800, .len = 0xd4,
- .features = PINGPONG_MSM8996_TE2_MASK,
+ .features = BIT(DPU_PINGPONG_TE2),
.sblk = &msm8996_pp_sblk_te,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13),
}, {
.name = "pingpong_2", .id = PINGPONG_2,
.base = 0x71000, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14),
}, {
.name = "pingpong_3", .id = PINGPONG_3,
.base = 0x71800, .len = 0xd4,
- .features = PINGPONG_MSM8996_MASK,
.sblk = &msm8996_pp_sblk,
.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15),
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 b18f4848f61391b527af243e6f0866ac3811b7cd..3f9e0045d8d6268304a2d85ebf8d86db373a3028 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -95,20 +95,14 @@
#define MIXER_QCM2290_MASK \
(BIT(DPU_DIM_LAYER) | BIT(DPU_MIXER_COMBINED_ALPHA))
-#define PINGPONG_MSM8996_MASK \
- (BIT(DPU_PINGPONG_DSC))
-
-#define PINGPONG_MSM8996_TE2_MASK \
- (PINGPONG_MSM8996_MASK | BIT(DPU_PINGPONG_TE2))
-
#define PINGPONG_SDM845_MASK \
- (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_DSC))
+ (BIT(DPU_PINGPONG_DITHER))
#define PINGPONG_SDM845_TE2_MASK \
(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
#define PINGPONG_SM8150_MASK \
- (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_DSC))
+ (BIT(DPU_PINGPONG_DITHER))
#define WB_SDM845_MASK (BIT(DPU_WB_LINE_MODE) | \
BIT(DPU_WB_UBWC) | \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 07349ba60c15387b0fa26b13cf6acaf69125b9f8..bef98e3471d4c8530e6a0fa35c8be207e080bd6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -119,7 +119,6 @@ enum {
* @DPU_PINGPONG_SPLIT PP block supports split fifo
* @DPU_PINGPONG_SLAVE PP block is a suitable slave for split fifo
* @DPU_PINGPONG_DITHER Dither blocks
- * @DPU_PINGPONG_DSC PP block supports DSC
* @DPU_PINGPONG_MAX
*/
enum {
@@ -127,7 +126,6 @@ enum {
DPU_PINGPONG_SPLIT,
DPU_PINGPONG_SLAVE,
DPU_PINGPONG_DITHER,
- DPU_PINGPONG_DSC,
DPU_PINGPONG_MAX
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -319,7 +319,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(struct drm_device *dev,
c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
}
- if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
+ if (mdss_rev->core_major_ver < 7) {
c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
--
2.39.5
On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
> Continue migration to the MDSS-revision based checks and replace
> DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 ++--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
> 7 files changed, 5 insertions(+), 20 deletions(-)
>
<snip>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -319,7 +319,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(struct drm_device *dev,
> c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
> }
>
> - if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
> + if (mdss_rev->core_major_ver < 7) {
> c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
> c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
> c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
>
So far in this series, we replaced the feature bits with >= checks of
core_revisions. That kind of works as usually feature bits get added
after a specific version.
With this patch and later, whenever we use < checks it gets a bit tricky
as we might also need an upper bound. Feature bits gave individual
control of chipsets but generalizing that all chipsets < 7 have PP DSC
is also not correct. I have to really cross-check but there could be
some old chipsets which do not have DSC at all.
On 1/23/2025 1:32 PM, Abhinav Kumar wrote:
>
>
> On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
>> Continue migration to the MDSS-revision based checks and replace
>> DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10
>> ++--------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>> 7 files changed, 5 insertions(+), 20 deletions(-)
>>
>
> <snip>
>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> index
>> 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>> @@ -319,7 +319,7 @@ struct dpu_hw_pingpong
>> *dpu_hw_pingpong_init(struct drm_device *dev,
>> c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
>> }
>> - if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
>> + if (mdss_rev->core_major_ver < 7) {
>> c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
>> c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
>> c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
>>
>
> So far in this series, we replaced the feature bits with >= checks of
> core_revisions. That kind of works as usually feature bits get added
> after a specific version.
>
> With this patch and later, whenever we use < checks it gets a bit tricky
> as we might also need an upper bound. Feature bits gave individual
> control of chipsets but generalizing that all chipsets < 7 have PP DSC
> is also not correct. I have to really cross-check but there could be
> some old chipsets which do not have DSC at all.
This raises another question as well.
what if some register was introduced >= X version but was then dropped
in a newer chipset.
Is it not difficult for the user to go back to the files of each of the
sub-blocks and alter these checks rather than just fixing up the catalog.
On Thu, Jan 23, 2025 at 01:41:14PM -0800, Abhinav Kumar wrote:
>
>
> On 1/23/2025 1:32 PM, Abhinav Kumar wrote:
> >
> >
> > On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
> > > Continue migration to the MDSS-revision based checks and replace
> > > DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
> > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
> > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
> > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10
> > > ++--------
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
> > > 7 files changed, 5 insertions(+), 20 deletions(-)
> > >
> >
> > <snip>
> >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0
> > > 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > @@ -319,7 +319,7 @@ struct dpu_hw_pingpong
> > > *dpu_hw_pingpong_init(struct drm_device *dev,
> > > c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
> > > }
> > > - if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
> > > + if (mdss_rev->core_major_ver < 7) {
> > > c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
> > > c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
> > > c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
> > >
> >
> > So far in this series, we replaced the feature bits with >= checks of
> > core_revisions. That kind of works as usually feature bits get added
> > after a specific version.
> >
> > With this patch and later, whenever we use < checks it gets a bit tricky
> > as we might also need an upper bound. Feature bits gave individual
> > control of chipsets but generalizing that all chipsets < 7 have PP DSC
> > is also not correct. I have to really cross-check but there could be
> > some old chipsets which do not have DSC at all.
>
> This raises another question as well.
>
> what if some register was introduced >= X version but was then dropped in a
> newer chipset.
>
> Is it not difficult for the user to go back to the files of each of the
> sub-blocks and alter these checks rather than just fixing up the catalog.
Well, the obvious example we are going to have is the CTL_LAYER_EXT4,
but if I understand correctly the whole block is going to be dropped, so
maybe it's not that relevant.
Another example might be CWB, where we are going to have 5.x-7.x and
8.x+ DPU ranges.
Basically, yes, when adding support for a new platform we have to audit
HW blocks. But this applied even beforehand, where new platforms could
be drooping existing regs (8.x+ dropping a part of the TOP region).
--
With best wishes
Dmitry
On 1/23/2025 9:19 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 23, 2025 at 01:41:14PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/23/2025 1:32 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
>>>> Continue migration to the MDSS-revision based checks and replace
>>>> DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10
>>>> ++--------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>>>> 7 files changed, 5 insertions(+), 20 deletions(-)
>>>>
>>>
>>> <snip>
>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0
>>>> 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> @@ -319,7 +319,7 @@ struct dpu_hw_pingpong
>>>> *dpu_hw_pingpong_init(struct drm_device *dev,
>>>> c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
>>>> }
>>>> - if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
>>>> + if (mdss_rev->core_major_ver < 7) {
>>>> c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
>>>> c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
>>>> c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
>>>>
>>>
>>> So far in this series, we replaced the feature bits with >= checks of
>>> core_revisions. That kind of works as usually feature bits get added
>>> after a specific version.
>>>
>>> With this patch and later, whenever we use < checks it gets a bit tricky
>>> as we might also need an upper bound. Feature bits gave individual
>>> control of chipsets but generalizing that all chipsets < 7 have PP DSC
>>> is also not correct. I have to really cross-check but there could be
>>> some old chipsets which do not have DSC at all.
>>
>> This raises another question as well.
>>
>> what if some register was introduced >= X version but was then dropped in a
>> newer chipset.
>>
>> Is it not difficult for the user to go back to the files of each of the
>> sub-blocks and alter these checks rather than just fixing up the catalog.
>
> Well, the obvious example we are going to have is the CTL_LAYER_EXT4,
> but if I understand correctly the whole block is going to be dropped, so
> maybe it's not that relevant.
>
> Another example might be CWB, where we are going to have 5.x-7.x and
> 8.x+ DPU ranges.
>
> Basically, yes, when adding support for a new platform we have to audit
> HW blocks. But this applied even beforehand, where new platforms could
> be drooping existing regs (8.x+ dropping a part of the TOP region).
>
Right, I am wondering what is the trade-off here.
1) Feature bits allow selective control for every chipset. IOW, the
specific version is already checked for. I do admit that I have seen
errors about using the correct feature mask sometimes but even with this
change, evey developer will need to go and check every sub-block file
which they might not even know about.
2) core_revision certainly can generalize but we might still need
absolute versions for some of the bits anyway. So the checks may not
always be >= or < but it could also end up with something like
if (major_rev == xxx && minor_rev == yyy)
ops = ops_a;
else if (major_rev == aaa &&& minor_rev == bbb)
ops = ops_b
So the revision check will most likely end up being more complicated
than simple range checks. With each catalog feature bit atleast we are
guaranteed that its applied only to that revision.
I do see in the cover letter, about incorrect feature bits sometimes
used but I am trying to assess the trade-offs even with this approach.
On Fri, Jan 24, 2025 at 04:08:17PM -0800, Abhinav Kumar wrote:
>
>
> On 1/23/2025 9:19 PM, Dmitry Baryshkov wrote:
> > On Thu, Jan 23, 2025 at 01:41:14PM -0800, Abhinav Kumar wrote:
> > >
> > >
> > > On 1/23/2025 1:32 PM, Abhinav Kumar wrote:
> > > >
> > > >
> > > > On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
> > > > > Continue migration to the MDSS-revision based checks and replace
> > > > > DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h | 2 --
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h | 1 -
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h | 2 --
> > > > > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h | 6 ++----
> > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10
> > > > > ++--------
> > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
> > > > > 7 files changed, 5 insertions(+), 20 deletions(-)
> > > > >
> > > >
> > > > <snip>
> > > >
> > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > > > > @@ -319,7 +319,7 @@ struct dpu_hw_pingpong
> > > > > *dpu_hw_pingpong_init(struct drm_device *dev,
> > > > > c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
> > > > > }
> > > > > - if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
> > > > > + if (mdss_rev->core_major_ver < 7) {
> > > > > c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
> > > > > c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
> > > > > c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
> > > > >
> > > >
> > > > So far in this series, we replaced the feature bits with >= checks of
> > > > core_revisions. That kind of works as usually feature bits get added
> > > > after a specific version.
> > > >
> > > > With this patch and later, whenever we use < checks it gets a bit tricky
> > > > as we might also need an upper bound. Feature bits gave individual
> > > > control of chipsets but generalizing that all chipsets < 7 have PP DSC
> > > > is also not correct. I have to really cross-check but there could be
> > > > some old chipsets which do not have DSC at all.
> > >
> > > This raises another question as well.
> > >
> > > what if some register was introduced >= X version but was then dropped in a
> > > newer chipset.
> > >
> > > Is it not difficult for the user to go back to the files of each of the
> > > sub-blocks and alter these checks rather than just fixing up the catalog.
> >
> > Well, the obvious example we are going to have is the CTL_LAYER_EXT4,
> > but if I understand correctly the whole block is going to be dropped, so
> > maybe it's not that relevant.
> >
> > Another example might be CWB, where we are going to have 5.x-7.x and
> > 8.x+ DPU ranges.
> >
> > Basically, yes, when adding support for a new platform we have to audit
> > HW blocks. But this applied even beforehand, where new platforms could
> > be drooping existing regs (8.x+ dropping a part of the TOP region).
> >
>
> Right, I am wondering what is the trade-off here.
>
> 1) Feature bits allow selective control for every chipset. IOW, the specific
> version is already checked for. I do admit that I have seen errors about
> using the correct feature mask sometimes but even with this change, evey
> developer will need to go and check every sub-block file which they might
> not even know about.
>
> 2) core_revision certainly can generalize but we might still need absolute
> versions for some of the bits anyway. So the checks may not always be >= or
> < but it could also end up with something like
>
> if (major_rev == xxx && minor_rev == yyy)
> ops = ops_a;
> else if (major_rev == aaa &&& minor_rev == bbb)
> ops = ops_b
>
> So the revision check will most likely end up being more complicated than
> simple range checks. With each catalog feature bit atleast we are guaranteed
> that its applied only to that revision.
Yes... However I'd prefer uniformity. In other words, if we started
using core revisions, let's use just it. Having core revision in some
places and feature bits in some other places sounds like an easy way to
confuse developers.
>
> I do see in the cover letter, about incorrect feature bits sometimes used
> but I am trying to assess the trade-offs even with this approach.
Sure :-)
--
With best wishes
Dmitry
© 2016 - 2025 Red Hat, Inc.