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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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