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

Tue Apr 4 00:33:33 UTC 2023

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?

> 
>>
>>> +
>>> +    return extra_eol_bytes;
>>> +}
>>> +
>>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config 
>>> *dsc, int intf_width, u32 src_bpp)
>>
>> Basing on Abhinav's description ("pclk_per_line can be only per 
>> interface") would it better be named as 
>> msm_dsc_get_uncompressed_pclk_per_intf() ? or 
>> msm_dsc_get_uncompressed_pclk_for_intf() ?
>>
>> BTW: if get_bytes_per_soft_slice() doesn't use intf_width, we can 
>> probably drop it here too.
>>
>>> +{
>>> +    s64 data_width;
>>> +
>>> +    if (!dsc->slice_width || (intf_width < dsc->slice_width))
>>> +        return -EINVAL;
>>
>> Error code is not validated at dsi_timing_setup. I'd suggest moving 
>> error checks there and dropping the error handling here. If 
>> dsc->slice_width is not set, we should stop much earlier than 
>> drm_bridge's pre_enable() callback.
> 
> Acked.
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>> +
>>> +    data_width = drm_fixp_mul(dsc->slice_count,
>>> +            get_bytes_per_soft_slice(dsc, intf_width, src_bpp));
>>> +
>>> +    return drm_fixp2int_ceil(data_width);
>>> +}
>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h 
>>> b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>> new file mode 100644
>>> index 000000000000..743cd324b7d9
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>> @@ -0,0 +1,42 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>> reserved
>>> + */
>>> +
>>> +#ifndef MSM_DSC_HELPER_H_
>>> +#define MSM_DSC_HELPER_H_
>>> +
>>> +#include <drm/display/drm_dsc_helper.h>
>>> +#include <drm/drm_modes.h>
>>> +
>>> +/*
>>> + * Helper methods for MSM specific DSC calculations that are common 
>>> between timing engine,
>>> + * DSI, and DP.
>>> + */
>>> +
>>> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
>>> +{
>>> +    WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
>>> +    return dsc->bits_per_pixel >> 4;
>>> +}
>>> +
>>> +static inline int msm_dsc_get_slice_per_intf(struct drm_dsc_config 
>>> *dsc, int intf_width)
>>> +{
>>> +    return DIV_ROUND_UP(intf_width, dsc->slice_width);
>>> +}
>>> +
>>> +static inline u32 msm_dsc_get_dce_bytes_per_line(struct 
>>> drm_dsc_config *dsc, int intf_width)
>>> +{
>>> +    return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8);
>>> +}
>>> +
>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int 
>>> intf_width, u32 src_bpp);
>>> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int 
>>> intf_width);
>>> +
>>> +/* Calculate uncompressed pclk per line. This value will then be 
>>> passed along to
>>> + * DSI and DP to calculate pclk_per_line. This is because DSI and DP 
>>> divide the
>>> + * uncompressed pclk_per_line by different values depending on if 
>>> widebus is enabled.
>>> + */
>>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc,
>>> +        int intf_width, u32 src_bpp);
>>> +#endif /* MSM_DSC_HELPER_H_ */
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry