Message-ID: <98e4bda7-19e9-09b6-f008-383adada97cb@linaro.org>
Date:   Wed, 31 May 2023 13:07:42 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc:     freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        quic_khsieh@...cinc.com, Rob Clark <robdclark@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, quic_jesszhan@...cinc.com,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Sean Paul <sean@...rly.run>
Subject: Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision
 to the catalog

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?
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.

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.

> 
>>> +
>>>   #define DPU_HW_BLK_NAME_LEN    16
>>>
>>>   #define MAX_IMG_WIDTH 0x3fff
>>> @@ -769,7 +796,7 @@ struct dpu_perf_cfg {
>>>   /**
>>>    * struct dpu_mdss_cfg - information of MDSS HW
>>>    * This is the main catalog data structure representing
>>> - * this HW version. Contains number of instances,
>>> + * this HW version. Contains dpu core revision, number of instances,
>>>    * register offsets, capabilities of the all MDSS HW sub-blocks.
>>>    *
>>>    * @dma_formats        Supported formats for dma pipe
>>> @@ -778,6 +805,8 @@ struct dpu_perf_cfg {
>>>    * @mdss_irqs:         Bitmap with the irqs supported by the target
>>>    */
>>>   struct dpu_mdss_cfg {
>>> +       u32 core_rev;
>>> +
>>>          const struct dpu_caps *caps;
>>>
>>>          const struct dpu_ubwc_cfg *ubwc;
>>> -- 
>>> 2.40.1
>>>
>>
>>

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists