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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 07/06/2023 00:01, Dmitry Baryshkov wrote:
> 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 
>>>>> <quic_abhinavk@quicinc.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@quicinc.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@quicinc.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.
>>>
>>
>> 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.

Please excuse me for not being explicit. I assumed we have to enable 
feature since 4.6 (major > 3, minor > 5)

Proper condition would be:

if (dpu_major_rev > 4 ||
     (dpu_major_rev == 4 && dpu_minor_rev >= 6))

-- 
With best wishes
Dmitry