Re: [Freedreno] [PATCH RFC v2 4/6] drm/msm/dpu: Fix slice_last_group_size calculation

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 4/3/2023 2:51 PM, Dmitry Baryshkov wrote:
> On 04/04/2023 00:45, Jessica Zhang wrote:
>>
>>
>> On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote:
>>> On 31/03/2023 21:49, Jessica Zhang wrote:
>>>> Correct the math for slice_last_group_size so that it matches the
>>>> calculations downstream.
>>>>
>>>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>>> index b952f7d2b7f5..9312a8d7fbd9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>>> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc 
>>>> *hw_dsc,
>>>>       if (is_cmd_mode)
>>>>           initial_lines += 1;
>>>> -    slice_last_group_size = 3 - (dsc->slice_width % 3);
>>>> +    slice_last_group_size = dsc->slice_width % 3;
>>>> +
>>>> +    if (slice_last_group_size == 0)
>>>> +        slice_last_group_size = 3;
>>>
>>> Hmm. As I went on checking this against techpack:
>>>
>>> mod = dsc->slice_width % 3
>>>
>>> mod | techpack | old | your_patch
>>> 0   | 2        | 3   | 3
>>> 1   | 0        | 2   | 1
>>> 2   | 1        | 1   | 2
>>>
>>> So, obviously neither old nor new code match the calculations of the 
>>> techpack. If we assume that sde_dsc_helper code is correct (which I 
>>> have no reasons to doubt), then the proper code should be:
>>>
>>> slice_last_group_size = (dsc->slice_width + 2) % 3;
>>>
>>> Could you please doublecheck and adjust.
>>
>> Hi Dmitry,
>>
>> The calculation should match the techpack calculation (I kept the 
>> `data |= ((slice_last_group_size - 1) << 18);` a few lines down).
> 
> And the techpack doesn't have -1.
> 
> I think the following code piece would be more convenient as it is simpler:
> 
> slice_last_group_size = (dsc->slice_width + 2) % 3;
> [...]
> data |= slice_last_group_size << 18;
> 
> If you agree, could you please switch to it?

Sure.

Thanks,

Jessica Zhang

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> +
>>>>       data = (initial_lines << 20);
>>>>       data |= ((slice_last_group_size - 1) << 18);
>>>>       /* bpp is 6.4 format, 4 LSBs bits are for fractional part */
>>>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
>>>
> 
> -- 
> With best wishes
> Dmitry
>