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

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

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?

-- 
With best wishes
Dmitry