[Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Jun 6 21:16:38 UTC 2023
On 6/6/2023 2:08 PM, Dmitry Baryshkov wrote:
> 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 at 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 at 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 at 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))
>
Ah okay, I really didnt focus too much on the condition itself but more
on the fact that we will now use hard-coded revision numbers in the code.
Will spin a v2 with the things that were agreed on. Thanks.
More information about the dri-devel
mailing list