[PATCH v4 1/6] drm/msm: Add MSM-specific DSC helper methods

[Abhinav Kumar]

On 4/5/2023 12:26 PM, Dmitry Baryshkov wrote:
> On 05/04/2023 03:41, 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()
>>
>> Changes in v3:
>> - Dropped src_bpp parameter from all methods -- src_bpp can be
>>    calculated as dsc->bits_per_component * 3
>> - Dropped intf_width parameter from get_bytes_per_soft_slice()
>> - Moved dsc->bits_per_component to numerator calculation in
>>    get_bytes_per_soft_slice()
>> - Renamed msm_dsc_get_uncompressed_pclk_per_line to
>>    *_get_uncompressed_pclk_per_intf()
>> - Removed dsc->slice_width check from
>>    msm_dsc_get_uncompressed_pclk_per_intf()
>> - Made get_bytes_per_soft_slice() a public method (this will be called
>>    later to help calculate DP pclk params)
>> - Added documentation in comments
>> - Moved extra_eol_bytes math out of msm_dsc_get_eol_byte_num() and
>>    renamed msm_dsc_get_eol_byte_num to *_get_bytes_per_intf.
>>
>> Changes in v4:
>> - Changed msm_dsc_get_uncompressed_pclk_per_intf to
>>    msm_dsc_get_pclk_per_intf
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/Makefile         |  1 +
>>   drivers/gpu/drm/msm/msm_dsc_helper.c | 47 ++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/msm_dsc_helper.h | 70 
>> ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 118 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..0539221eb09d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
>> @@ -0,0 +1,47 @@
>> +// 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"
>> +
>> +s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
>> +{
>> +    int bpp = msm_dsc_get_bpp_int(dsc);
>> +    s64 numerator_fp, denominator_fp;
>> +    s64 comp_ratio_fp = 
>> drm_fixp_from_fraction(dsc->bits_per_component * 3, bpp);
>> +
>> +    numerator_fp = drm_int2fixp(dsc->slice_width * 3 * 
>> dsc->bits_per_component);
>> +    denominator_fp = drm_fixp_mul(comp_ratio_fp, drm_int2fixp(8));
>> +
>> +    return drm_fixp_div(numerator_fp, denominator_fp);
> 
> If we remove 3 * dsc->bits_per_components from both numerator and 
> denominator, this whole function seems to be as simple as 
> DIV_ROUND_UP(dsc->slice_width * bpp, 8)
> 
> Or, if you prefer FP math, drm_fixp_from_fraction(dsc->slice_width * 
> bpp, 8).
> 

True and thats not really surprising because bytes_per_soft_slice is 
eventually (dsc->slice_width * bpp, 8)

I also thought about it after you mentioned that they will cancel out 
then why was downstream code and our programming guide doing this way.

So i thought a bit more and the reason from what I can see is that its 
showing that compression ratio was factored into the math while 
calculating it. If we just go with (dsc->slice_width * bpp, 8), it does 
not really show that this was actually compressed bytes per slice 
leading to the earlier confusion that it was uncompressed pclk while it 
actually was.

>> +}
>> +
>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int 
>> intf_width)
>> +{
>> +    u32 bytes_per_soft_slice, 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);
>> +    bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
>> +
>> +    bytes_per_intf = bytes_per_soft_slice * slice_per_intf;
>> +
>> +    return bytes_per_intf;
>> +}
>> +
>> +int msm_dsc_get_pclk_per_intf(struct drm_dsc_config *dsc)
>> +{
>> +    s64 data_width;
>> +
>> +    data_width = drm_fixp_mul(drm_int2fixp(dsc->slice_count),
>> +            get_bytes_per_soft_slice(dsc));
> 
> And this is then DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * bpp, 8)
> 
>> +
>> +    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..31116a31090f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
>> @@ -0,0 +1,70 @@
>> +/* 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.
>> + */
>> +
>> +/**
>> + * msm_dsc_get_bpp_int - get bits per pixel integer value
>> + * @dsc: Pointer to drm dsc config struct
>> + */
>> +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;
>> +}
>> +
>> +/**
>> + * msm_dsc_get_slice_per_intf - get number of slices per interface
>> + * @dsc: Pointer to drm dsc config struct
>> + * @intf_width: interface width
>> + */
>> +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);
>> +}
>> +
>> +/**
>> + * msm_dsc_get_dce_bytes_per_line - get bytes per line to help 
>> calculate data width
>> + *    when configuring the timing engine
>> + * @dsc: Pointer to drm dsc config struct
>> + * @intf_width: interface 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);
>> +}
>> +
>> +/**
>> + * get_bytes_per_soft_slice - get size of each soft slice for dsc
>> + * @dsc: Pointer to drm dsc config struct
>> + */
>> +s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
>> +
>> +/**
>> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
>> + * @dsc: Pointer to drm dsc config struct
>> + * @intf_width: interface width
>> + */
>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int 
>> intf_width);
>> +
>> +/**
>> + * msm_dsc_get_pclk_per_intf - Calculate pclk per interface.
>> + * @dsc: Pointer to drm dsc config struct
>> + *
>> + * Note: This value will then be passed along to DSI and DP for some 
>> more
>> + * calculations. This is because DSI and DP divide the pclk_per_intf 
>> value
>> + * by different values depending on if widebus is enabled.
>> + */
>> +int msm_dsc_get_pclk_per_intf(struct drm_dsc_config *dsc);
> 
> empty line, please
> 
>> +#endif /* MSM_DSC_HELPER_H_ */
>>
>