Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote:
> On 27/07/2023 18:24, Abhinav Kumar wrote:
>>
>>
>> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
>>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar 
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
>>>>> Switch to using data from MDSS driver to program the SSPP fetch and 
>>>>> UBWC
>>>>> configuration.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
>>>>>    6 files changed, 36 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>> index cf70a9bd1034..bfd82c2921af 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>> @@ -8,6 +8,8 @@
>>>>>    #include "dpu_hw_sspp.h"
>>>>>    #include "dpu_kms.h"
>>>>>
>>>>> +#include "msm_mdss.h"
>>>>> +
>>>>>    #include <drm/drm_file.h>
>>>>>
>>>>>    #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct 
>>>>> dpu_sw_pipe *pipe,
>>>>>                DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
>>>>>                        DPU_FETCH_CONFIG_RESET_VALUE |
>>>>>                        ctx->ubwc->highest_bank_bit << 18);
>>>>
>>>> Does this needs to be protected with if ctx->ubwc check?
>>>
>>> Yes... And it should have been already.
>>>
>>>>
>>>>> -             switch (ctx->ubwc->ubwc_version) {
>>>>> -             case DPU_HW_UBWC_VER_10:
>>>>> +             switch (ctx->ubwc->ubwc_enc_version) {
>>>>> +             case UBWC_1_0:
>>>>
>>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
>>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in 
>>>> the
>>>> catalog for the encoder version in the first place? Because looking at
>>>> the registers UBWC_x_x is the correct value.
>>>>
>>>> If we cannot find the reason why, it should be noted in the commit text
>>>> that the values we are using did change.
>>>
>>> Huh? This is just an enum. It isn't a part of uABI, nor it is written
>>> to the hardware.
>>>
>>
>> The reason is that, this switch case is moving from comparing one set 
>> of values to totally different ones. So atleast that should be noted.
>>
>> First thing that struck me was this point because the enums UBWC_x_x 
>> and DPU_HW_UBWC_VER_xx dont match.
>>
> 
> Do you have any proposed text in mind?
> 

I was doing some checking about this. The issue was that when this enum 
was made, it missed using the SDE_HW_UBWC_VER macro


75 enum {
76 	DPU_HW_UBWC_VER_10 = 0x100,
77 	DPU_HW_UBWC_VER_20 = 0x200,
78 	DPU_HW_UBWC_VER_30 = 0x300,
79 	DPU_HW_UBWC_VER_40 = 0x400,
80 };
81

so something like this:

183  */
184 enum {
185 	SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100),
186 	SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200),
187 	SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300),
188 	SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400),
189 	SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431),
190 };

This macro handles that conversion under the hood.

So I would write something like this

"This also corrects the usage of UBWC version which was incorrect from 
the beginning because of the enum storing the DPU_HW_UBWC_*** missed out 
the conversion to the full UBWC version"