by Abhinav Kumar

[permalink] [raw]

Subject: Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog

On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
> On 06/06/2023 20:51, Abhinav Kumar wrote:
>>
>>
>> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar
>>> <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>>> compatible string from the device tree to select the dpu
>>>>>>>>>>>> catalog
>>>>>>>>>>>> being used in the device.
>>>>>>>>>>>>
>>>>>>>>>>>> This approach works well however also necessitates adding
>>>>>>>>>>>> catalog
>>>>>>>>>>>> entries for small register level details as dpu capabilities
>>>>>>>>>>>> and/or
>>>>>>>>>>>> features bloating the catalog unnecessarily. Examples
>>>>>>>>>>>> include but
>>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>>> widebus etc.
>>>>>>>>>>>>
>>>>>>>>>>>> Introduce the dpu core revision back as an entry to the catalog
>>>>>>>>>>>> so that
>>>>>>>>>>>> we can just use dpu revision checks and enable those bits which
>>>>>>>>>>>> should be enabled unconditionally and not controlled by a
>>>>>>>>>>>> catalog
>>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>>
>>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>>
>>>>>>>>>>>> Also, add some of the useful macros back to be able to use
>>>>>>>>>>>> dpu core
>>>>>>>>>>>> revision effectively.
>>>>>>>>>>>>
>>>>>>>>>>>> [1]:
>>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Abhinav Kumar <[email protected]>
>>>>>>>>>>>> ---
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    | 1 +
>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    | 1 +
>>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>>
>>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>>     */
>>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>>
>>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /* sm6115|sm4250 */
>>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /*
>>>>>>>>>>>> qcm2290|sm4125 */
>>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>>
>>>>>>>>>>> Instead of having defines for all SoCs (which can quickly become
>>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest
>>>>>>>>>>> inlining
>>>>>>>>>>> all
>>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sure, that can be done.
>>>>>>>>>>
>>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>>> catalog. I
>>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>>> correct
>>>>>>>>>>> me if I'm wrong, different step levels are used for revisions
>>>>>>>>>>> of the
>>>>>>>>>>> same SoC. The original code that was reading the hw revision
>>>>>>>>>>> from
>>>>>>>>>>> the
>>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>>
>>>>>>>>>> Before the catalog rework, we used to handle even steps as we
>>>>>>>>>> used
>>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>>> handler. But after the rework, we dont handle steps anymore. Yes,
>>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>>> atleast am not aware of DPU differences between steps but I am
>>>>>>>>>> not
>>>>>>>>>> able to rule it out.
>>>>>>>>>>
>>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>>> macro shall only handle major and minor versions? With the
>>>>>>>>>> current
>>>>>>>>>> chipsets I see, it should not make a difference . Its just that I
>>>>>>>>>> am not sure if that will never happen.
>>>>>>>>>
>>>>>>>>> Yes. The goal of this rework would be to drop generic features and
>>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>>
>>>>>>>> Yes thats right.
>>>>>>>>
>>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>>> revision is a sign of paltform feature (or mis-feature) rather
>>>>>>>>> than
>>>>>>>>> a generic DPU bit.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
>>>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>>> cannot.
>>>>>>>>
>>>>>>>> This is what is the sticking point for me which is holding me back
>>>>>>>> against dropping step. Thoughts?
>>>>>>>
>>>>>>> We usually do not support ES versions of the chips, only the final
>>>>>>> version. So supporting the perf data for earlier revisions is also
>>>>>>> not required.
>>>>>>>
>>>>>>
>>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>>> versions.
>>>>>>
>>>>>>>>
>>>>>>>>> In fact I suppose that even handling a minor revision would be an
>>>>>>>>> overkill. Why don't we start with .dpu_major instead of .core_rev?
>>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>>
>>>>>>>>
>>>>>>>> No, unfortunately we cannot drop minor version for sure. I am
>>>>>>>> seeing
>>>>>>>> examples in downstream code where some of the features are
>>>>>>>> available
>>>>>>>> after a minor verion as well.
>>>>>>>
>>>>>>> Can you please give an example?
>>>>>>>
>>>>>>
>>>>>> Yes, watchdog timer, intf reset counter are available only after DPU
>>>>>> HW version 8.1 (not major version 8).
>>>>>
>>>>> Hmm, IIRC, wd timer was available for ages. Was it moved together with
>>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>>
>>>>
>>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>>
>>>>> But anyway, I see your point. Let's have major and minor. I'd probably
>>>>> still ask for the separate major and minor fields, if you don't mind.
>>>>>
>>>>
>>>> hmmm so something like this?
>>>>
>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>>
>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>> .......
>>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>>
>>> Just:
>>>
>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>      .dpu_major_rev = 3,
>>>      .dpu_minor_rev = 0,
>>>      /* .... */
>>> };
>>>
>>> We do not need a single enumeration of all the versions. It can easily
>>> become a source of merge conflicts.
>>>
>>
>> The issue with this approch is then the DPU_HW_VER_xxx macros become
>> redundant but we should keep them. Because in the code, its much more
>> readable to have
>>
>> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>>      then enable feature;
>>
>> But now we will have to do
>>
>> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>>      then enable feature;
>>
>> /// probably folks will question this xxx and yyy as to what it means.
>
> The first approach is less readable. It will require anybody to check,
> what is the major/minor of the mentioned XXX platform. In the second
> case we know exactly what we are looking for. E.g. we have new INTF
> interrupt addresses since 7.0. We have MDP_TOP_PERIPH0_REMOVED since 8.0
> (or 8.1?) We have PP_TE until 5.0 (not included) and INTF_TE since 5.0.
>
> Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW
> revision of sm8350, sc8280xp, sm8150, etc.
>

Agreed, it will avoid one extra lookup but are you comfortable with a
hard-coded number in the code? So for example.

if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
enable feature;

Is this acceptable more than having to lookup?

Thats my only concern.

2023-06-06 21:35:29

by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog

On 06/06/2023 22:28, Abhinav Kumar wrote:
>
>
> On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
>> On 06/06/2023 20:51, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
>>>> On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar
>>>> <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
>>>>>> On 06/06/2023 03:55, Abhinav Kumar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
>>>>>>>> On 31/05/2023 21:25, Abhinav Kumar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On 31/05/2023 06:05, Abhinav Kumar wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> On Wed, 31 May 2023 at 03:54, Abhinav Kumar
>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> With [1] dpu core revision was dropped in favor of using the
>>>>>>>>>>>>> compatible string from the device tree to select the dpu
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> being used in the device.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This approach works well however also necessitates adding
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> entries for small register level details as dpu
>>>>>>>>>>>>> capabilities and/or
>>>>>>>>>>>>> features bloating the catalog unnecessarily. Examples
>>>>>>>>>>>>> include but
>>>>>>>>>>>>> are not limited to data_compress, interrupt register set,
>>>>>>>>>>>>> widebus etc.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Introduce the dpu core revision back as an entry to the
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> so that
>>>>>>>>>>>>> we can just use dpu revision checks and enable those bits
>>>>>>>>>>>>> which
>>>>>>>>>>>>> should be enabled unconditionally and not controlled by a
>>>>>>>>>>>>> catalog
>>>>>>>>>>>>> and also simplify the changes to do something like:
>>>>>>>>>>>>>
>>>>>>>>>>>>> if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
>>>>>>>>>>>>>           enable the bit;
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also, add some of the useful macros back to be able to use
>>>>>>>>>>>>> dpu core
>>>>>>>>>>>>> revision effectively.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]:
>>>>>>>>>>>>> https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Abhinav Kumar <[email protected]>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    | 1 +
>>>>>>>>>>>>>    .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    | 1 +
>>>>>>>>>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 31
>>>>>>>>>>>>> ++++++++++++++++++-
>>>>>>>>>>>>>    14 files changed, 43 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> [skipped catalog changes]
>>>>>>>>>>>>
>>>>>>>>>>>>> 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 677048cc3b7d..cc4aa75a1219 100644
>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>>>>>>>>>> @@ -19,6 +19,33 @@
>>>>>>>>>>>>>     */
>>>>>>>>>>>>>    #define MAX_BLOCKS    12
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define DPU_HW_VER(MAJOR, MINOR, STEP)\
>>>>>>>>>>>>> +                 ((((unsigned int)MAJOR & 0xF) << 28) |\
>>>>>>>>>>>>> +                 ((MINOR & 0xFFF) << 16) |\
>>>>>>>>>>>>> +                 (STEP & 0xFFFF))
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define DPU_HW_MAJOR(rev)((rev) >> 28)
>>>>>>>>>>>>> +#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
>>>>>>>>>>>>> +#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
>>>>>>>>>>>>> +#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2)   \
>>>>>>>>>>>>> +(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
>>>>>>>>>>>>> +#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
>>>>>>>>>>>>> +#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180 v1.0 */
>>>>>>>>>>>>> +#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /*
>>>>>>>>>>>>> sm6115|sm4250 */
>>>>>>>>>>>>> +#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /*
>>>>>>>>>>>>> qcm2290|sm4125 */
>>>>>>>>>>>>> +#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
>>>>>>>>>>>>> +#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
>>>>>>>>>>>>> +#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
>>>>>>>>>>>>> +#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
>>>>>>>>>>>>> +#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
>>>>>>>>>>>>
>>>>>>>>>>>> Instead of having defines for all SoCs (which can quickly
>>>>>>>>>>>> become
>>>>>>>>>>>> unmanageable) and can cause merge conflicts, I'd suggest
>>>>>>>>>>>> inlining
>>>>>>>>>>>> all
>>>>>>>>>>>> the defines into respective catalog files.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Sure, that can be done.
>>>>>>>>>>>
>>>>>>>>>>>> Also, I'm not sure that the "step" should be a part of the
>>>>>>>>>>>> catalog. I
>>>>>>>>>>>> know that this follows the hardware revision. However, please
>>>>>>>>>>>> correct
>>>>>>>>>>>> me if I'm wrong, different step levels are used for
>>>>>>>>>>>> revisions of the
>>>>>>>>>>>> same SoC. The original code that was reading the hw revision
>>>>>>>>>>>> from
>>>>>>>>>>>> the
>>>>>>>>>>>> hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This is one of the things i noticed while making this change.
>>>>>>>>>>>
>>>>>>>>>>> Before the catalog rework, we used to handle even steps as we
>>>>>>>>>>> used
>>>>>>>>>>> to read that from the register and match it with the mdss_cfg
>>>>>>>>>>> handler. But after the rework, we dont handle steps anymore.
>>>>>>>>>>> Yes,
>>>>>>>>>>> you are right that different step levels are used for the
>>>>>>>>>>> revisions of the same SOC and so with that, i dont expect or
>>>>>>>>>>> atleast am not aware of DPU differences between steps but I
>>>>>>>>>>> am not
>>>>>>>>>>> able to rule it out.
>>>>>>>>>>>
>>>>>>>>>>> So are you suggesting we drop step altogether and DPU_HW_VER()
>>>>>>>>>>> macro shall only handle major and minor versions? With the
>>>>>>>>>>> current
>>>>>>>>>>> chipsets I see, it should not make a difference . Its just
>>>>>>>>>>> that I
>>>>>>>>>>> am not sure if that will never happen.
>>>>>>>>>>
>>>>>>>>>> Yes. The goal of this rework would be to drop generic features
>>>>>>>>>> and
>>>>>>>>>> to replace those checks with DPU-revision lookups. Correct?
>>>>>>>>>
>>>>>>>>> Yes thats right.
>>>>>>>>>
>>>>>>>>>> I think that from this perspective having to handle toe step
>>>>>>>>>> revision is a sign of an overkill. Having to handle the step
>>>>>>>>>> revision is a sign of paltform feature (or mis-feature) rather
>>>>>>>>>> than
>>>>>>>>>> a generic DPU bit.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Not entirely. Lets not forget that at the moment even dpu_perf_cfg
>>>>>>>>> is part of the catalog. Even if in terms of major HW blocks steps
>>>>>>>>> shouldnt change, there is absolutely no guarantee that perf data
>>>>>>>>> cannot.
>>>>>>>>>
>>>>>>>>> This is what is the sticking point for me which is holding me back
>>>>>>>>> against dropping step. Thoughts?
>>>>>>>>
>>>>>>>> We usually do not support ES versions of the chips, only the final
>>>>>>>> version. So supporting the perf data for earlier revisions is also
>>>>>>>> not required.
>>>>>>>>
>>>>>>>
>>>>>>> ack, we will drop step in that case. and good to know about the ES
>>>>>>> versions.
>>>>>>>
>>>>>>>>>
>>>>>>>>>> In fact I suppose that even handling a minor revision would be an
>>>>>>>>>> overkill. Why don't we start with .dpu_major instead of
>>>>>>>>>> .core_rev?
>>>>>>>>>> We can add .dpu_minor if/when required.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, unfortunately we cannot drop minor version for sure. I am
>>>>>>>>> seeing
>>>>>>>>> examples in downstream code where some of the features are
>>>>>>>>> available
>>>>>>>>> after a minor verion as well.
>>>>>>>>
>>>>>>>> Can you please give an example?
>>>>>>>>
>>>>>>>
>>>>>>> Yes, watchdog timer, intf reset counter are available only after DPU
>>>>>>> HW version 8.1 (not major version 8).
>>>>>>
>>>>>> Hmm, IIRC, wd timer was available for ages. Was it moved together
>>>>>> with
>>>>>> the introduction of MDSS_PERIPH_0_REMOVED?
>>>>>>
>>>>>
>>>>> I am not sure of the timeline but its certainly tied to 8.1.
>>>>>
>>>>>> But anyway, I see your point. Let's have major and minor. I'd
>>>>>> probably
>>>>>> still ask for the separate major and minor fields, if you don't mind.
>>>>>>
>>>>>
>>>>> hmmm so something like this?
>>>>>
>>>>> +#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
>>>>>
>>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>> .......
>>>>> .dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
>>>>> .dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
>>>>
>>>> Just:
>>>>
>>>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>>>>      .dpu_major_rev = 3,
>>>>      .dpu_minor_rev = 0,
>>>>      /* .... */
>>>> };
>>>>
>>>> We do not need a single enumeration of all the versions. It can easily
>>>> become a source of merge conflicts.
>>>>
>>>
>>> The issue with this approch is then the DPU_HW_VER_xxx macros become
>>> redundant but we should keep them. Because in the code, its much more
>>> readable to have
>>>
>>> if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
>>>      then enable feature;
>>>
>>> But now we will have to do
>>>
>>> if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
>>>      then enable feature;
>>>
>>> /// probably folks will question this xxx and yyy as to what it means.
>>
>> The first approach is less readable. It will require anybody to check,
>> what is the major/minor of the mentioned XXX platform. In the second
>> case we know exactly what we are looking for. E.g. we have new INTF
>> interrupt addresses since 7.0. We have MDP_TOP_PERIPH0_REMOVED since
>> 8.0 (or 8.1?) We have PP_TE until 5.0 (not included) and INTF_TE since
>> 5.0.
>>
>> Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW
>> revision of sm8350, sc8280xp, sm8150, etc.
>>
>
> Agreed, it will avoid one extra lookup but are you comfortable with a
> hard-coded number in the code? So for example.
>
> if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
>     enable feature;
>
> Is this acceptable more than having to lookup?

Yes, definitely.

Note that this is not a correct condition for what you have meant.

>
> Thats my only concern.

--
With best wishes
Dmitry