Date:   Tue, 6 Jun 2023 22:09:01 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc:     Sean Paul <sean@...rly.run>, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        quic_khsieh@...cinc.com,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        quic_jesszhan@...cinc.com, freedreno@...ts.freedesktop.org
Subject: Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision
 to the catalog

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 <quic_abhinavk@...cinc.com> 
>> 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
>>>>>>>>>> <quic_abhinavk@...cinc.com> 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 <quic_abhinavk@...cinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>>    .../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.

> 
> Can you please explain the single enumeration a bit more?
> 
> I thought we are going to move each revision to its separate file to 
> avoid the conflicts as you suggested earlier in the comments. Isnt that 
> enough?

This way you wouldn't be able to check against them in other modules.

> 
>>> .......
>>> }
>>>
>>> But may I ask why? Since the manor/minor version macros handle this
>>> nicely for us.
>>
>>
>>

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists