[PATCH RFC v2 2/6] drm/msm: Add MSM-specific DSC helper methods

[Dmitry Baryshkov]

On 04/04/2023 19:29, Jessica Zhang wrote:
> 
> 
> On 4/3/2023 5:33 PM, Dmitry Baryshkov wrote:
>> On 04/04/2023 00:38, Jessica Zhang wrote:
>>>
>>>
>>> On 4/2/2023 4:21 AM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 21:49, Jessica Zhang wrote:
>>>>> Introduce MSM-specific DSC helper methods, as some calculations are
>>>>> common between DP and DSC.
>>>>>
>>>>> Changes in v2:
>>>>> - Moved files up to msm/ directory
>>>>> - Dropped get_comp_ratio() helper
>>>>> - Used drm_int2fixp() to convert to integers to fp
>>>>> - Style changes to improve readability
>>>>> - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line()
>>>>> - Changed msm_dsc_get_slice_per_intf() to a static inline method
>>>>> - Dropped last division step of msm_dsc_get_pclk_per_line() and 
>>>>> changed
>>>>>    method name accordingly
>>>>> - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method
>>>>> - Fixed some math issues caused by passing in incorrect types to
>>>>>    drm_fixed methods in get_bytes_per_soft_slice()
>>>>>
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/Makefile         |  1 +
>>>>>   drivers/gpu/drm/msm/msm_dsc_helper.c | 53 
>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>   drivers/gpu/drm/msm/msm_dsc_helper.h | 42 
>>>>> ++++++++++++++++++++++++++++
>>>>>   3 files changed, 96 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/Makefile 
>>>>> b/drivers/gpu/drm/msm/Makefile
>>>>> index 7274c41228ed..b814fc80e2d5 100644
>>>>> --- a/drivers/gpu/drm/msm/Makefile
>>>>> +++ b/drivers/gpu/drm/msm/Makefile
>>>>> @@ -94,6 +94,7 @@ msm-y += \
>>>>>       msm_atomic_tracepoints.o \
>>>>>       msm_debugfs.o \
>>>>>       msm_drv.o \
>>>>> +    msm_dsc_helper.o \
>>>>>       msm_fb.o \
>>>>>       msm_fence.o \
>>>>>       msm_gem.o \
>>>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c 
>>>>> b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>>>> new file mode 100644
>>>>> index 000000000000..60b73e17e6eb
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>>>> @@ -0,0 +1,53 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +/*
>>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>>>> reserved
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/errno.h>
>>>>> +#include <drm/drm_fixed.h>
>>>>> +
>>>>> +#include "msm_drv.h"
>>>>> +#include "msm_dsc_helper.h"
>>>>> +
>>>>> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, 
>>>>> int intf_width, u32 src_bpp)
>>>>
>>>> intf_width is unused
>>>
>>> Hi Dmitry,
>>>
>>> Acked.
>>>
>>>>
>>>>> +{
>>>>> +    int bpp = msm_dsc_get_bpp_int(dsc);
>>>>> +    s64 numerator_fp, denominator_fp;
>>>>> +    s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp);
>>>>> +
>>>>> +    numerator_fp = drm_int2fixp(dsc->slice_width * 3);
>>>>
>>>> You have lost dsc->bits_per_component here.
>>>
>>> This was moved to the denominator calculation, but I'll move it back 
>>> to this line to avoid confusion.
>>
>> Maybe you occasionally mixed bpp and bpc, because there is no 
>> bits_per_component usage in denominator. Could you please recheck the 
>> calculations.
>>
>>>
>>>>
>>>>> +    denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, 
>>>>> drm_int2fixp(bpp));
>>>>
>>>> denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp);
>>>
>>> Acked.
>>>
>>>>
>>>>> +
>>>>> +    return drm_fixp_div(numerator_fp, denominator_fp);
>>>>> +}
>>>>> +
>>>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int 
>>>>> intf_width, u32 src_bpp)
>>>>> +{
>>>>> +    u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf;
>>>>> +    s64 bytes_per_soft_slice_fp;
>>>>> +    int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
>>>>> +
>>>>> +    bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, 
>>>>> intf_width, src_bpp);
>>>>> +    bytes_per_soft_slice = 
>>>>> drm_fixp2int_ceil(bytes_per_soft_slice_fp);
>>>>> +
>>>>> +    bytes_per_intf = bytes_per_soft_slice * slice_per_intf;
>>>>> +    extra_eol_bytes = bytes_per_intf % 3;
>>>>> +    if (extra_eol_bytes != 0)
>>>>> +        extra_eol_bytes = 3 - extra_eol_bytes;
>>>>
>>>> I become confused here when I checked eol_bytes in the display 
>>>> techpack.
>>>>
>>>> I see that for DP the dp_panel_dsc_pclk_param_calc() calculates 
>>>> dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * 
>>>> slice_per_intf to 3 bytes.
>>>>
>>>> However, for DSI this is a simple as total_bytes_per_intf % 3 , so 
>>>> it is not a padding, but a length of the last chunk.
>>>>
>>>> Could you please clarify? If the techpack code is correct, I'd 
>>>> prefer if we return last chunk size here and calculate the padding 
>>>> length in the DP driver.
>>>
>>> I've double checked the calculations between DP and DSI, and I think 
>>> you're right. Will move the `if (extra_eol_bytes != 0)` block out to 
>>> DP code.
>>
>> Ack. Could you please check with HW team that our understanding is 
>> correct?
> 
> Hey Dmitry,
> 
> I've checked with the HW team and looks like the math for eol_byte_nums 
> differs between DP and DSI.
> 
> For DSI, eol_byte_num = bytes_per_intf % 3
> 
> But for DP, eol_byte_num = (bytes_per_intf % 3 == 0) ? 0 : 3 - 
> bytes_per_intf % 3 *only* for non-widebus.
> 
> For DP && widebus enabled, eol_byte_num = (bytes_per_intf % 6 == 0) ? 0 
> : 6 - bytes_per_intf % 6
> 
> In that case, we should move even the bytes_per_intf % 3 out and change 
> this method to msm_dsc_get_bytes_per_intf() instead.

Thanks for the note. I'm looking forward to seeing the v3 then.

> 
> Thanks,
> 
> Jessica Zhang

-- 
With best wishes
Dmitry