[PATCH RFC v2 2/6] drm/msm: Add MSM-specific DSC helper methods
Jessica Zhang
quic_jesszhan at quicinc.com
Tue Apr 4 16:29:12 UTC 2023
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,
Jessica Zhang
>
>>
>>>
>>>> +
>>>> + 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
>
More information about the dri-devel
mailing list