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

[Dmitry Baryshkov]

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 at quicinc.com>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at 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?

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