diff mbox series

[RFC,2/5] drm/msm: Add MSM-specific DSC helper methods

Message ID 20230329-rfc-msm-dsc-helper-v1-2-f3e479f59b6d@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Introduce MSM-specific DSC helpers | expand

Commit Message

Jessica Zhang March 29, 2023, 11:18 p.m. UTC
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/Makefile              |  1 +
 drivers/gpu/drm/msm/disp/msm_dsc_helper.c | 74 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/disp/msm_dsc_helper.h | 28 ++++++++++++
 3 files changed, 103 insertions(+)

Comments

Dmitry Baryshkov March 30, 2023, 12:40 a.m. UTC | #1
On 30/03/2023 02:18, Jessica Zhang wrote:
> Introduce MSM-specific DSC helper methods, as some calculations are
> common between DP and DSC.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/Makefile              |  1 +
>   drivers/gpu/drm/msm/disp/msm_dsc_helper.c | 74 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/msm/disp/msm_dsc_helper.h | 28 ++++++++++++
>   3 files changed, 103 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 7274c41228ed..897a5b1c88f6 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -90,6 +90,7 @@ msm-y += \
>   	disp/mdp_kms.o \
>   	disp/msm_disp_snapshot.o \
>   	disp/msm_disp_snapshot_util.o \
> +	disp/msm_dsc_helper.o \
>   	msm_atomic.o \
>   	msm_atomic_tracepoints.o \
>   	msm_debugfs.o \
> diff --git a/drivers/gpu/drm/msm/disp/msm_dsc_helper.c b/drivers/gpu/drm/msm/disp/msm_dsc_helper.c
> new file mode 100644
> index 000000000000..ec15c0d829e8
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/msm_dsc_helper.c
> @@ -0,0 +1,74 @@
> +// 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 int get_comp_ratio(struct drm_dsc_config *dsc, u32 src_bpp)
> +{
> +	return mult_frac(100, src_bpp, DSC_BPP(*dsc));
> +}
> +
> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int intf_width, int comp_ratio)
> +{
> +	s64 comp_ratio_fp, num_bits_fp;
> +	s64 numerator_fp, denominator_fp;
> +
> +	comp_ratio_fp = drm_fixp_from_fraction(comp_ratio, 100);

Please inline comp_ration calculation here. Don't use mult_frac().

> +	num_bits_fp = drm_fixp_from_fraction(8, 1);

drm_int2fixp

> +
> +	numerator_fp = drm_fixp_from_fraction(dsc->slice_width * dsc->bits_per_component * 3, 1);

And here too.

> +	denominator_fp = drm_fixp_mul(comp_ratio_fp, num_bits_fp);

And num_bits_fp can be inlined too.
denominator_fp = drm_fixp_from_fraction(src_bpp * 8, DSC_BPP)

> +
> +	return drm_fixp_div(numerator_fp, denominator_fp);

dsc->slice_width * bpc * 3 / (8 * src_bpp / DSC_BPP), thus:

drm_fixp_from_fraction(dsc->slice_width * bpc * 3 * DSC_BPP, 8 * src_bpp)

but I will not insist on this one.

> +}
> +
> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp)
> +{
> +	u32 bytes_per_ss, extra_eol_bytes, bytes_per_intf;
> +	s64 bytes_per_ss_fp;
> +	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
> +	int comp_ratio = get_comp_ratio(dsc, src_bpp);
> +
> +	bytes_per_ss_fp = get_bytes_per_soft_slice(dsc, intf_width, comp_ratio);
> +	bytes_per_ss = drm_fixp2int_ceil(bytes_per_ss_fp);

s/_ss/_soft_slice/g

> +
> +	bytes_per_intf = bytes_per_ss * slice_per_intf;
> +	extra_eol_bytes = bytes_per_intf % 3;
> +	if (extra_eol_bytes != 0)
> +		extra_eol_bytes = 3 - extra_eol_bytes;
> +
> +	return extra_eol_bytes;
> +}
> +
> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int intf_width)
> +{
> +	u32 bpp;
> +	u32 dce_bytes_per_line;
> +
> +	bpp = DSC_BPP(*dsc);

Didn't this cause a warning on the unused-but-set variable?

> +	dce_bytes_per_line = DIV_ROUND_UP(dsc->bits_per_pixel * intf_width, 8);
> +
> +	return dce_bytes_per_line;
> +}

If you have msm_dsc_get_slice_per_intf() as a static inline, this 
function can be a static inline too. Nothing more than a single 
DIV_ROUND_UP.

> +
> +int msm_dsc_get_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp)
> +{
> +	s64 data_width;
> +	int comp_ratio = get_comp_ratio(dsc, src_bpp);
> +
> +	if (!dsc->slice_width || (intf_width < dsc->slice_width))
> +		return -EINVAL;
> +
> +	data_width = get_bytes_per_soft_slice(dsc, intf_width, comp_ratio);
> +	data_width = drm_fixp_mul(dsc->slice_count, data_width);
> +	data_width = drm_fixp_from_fraction(data_width, 3);

Reusing a variable is a nice trick, but it can be confusing. Not to 
mention that the last call should probably be drm_fixp_div()

> +
> +	return drm_fixp2int_ceil(data_width);
> +}
> diff --git a/drivers/gpu/drm/msm/disp/msm_dsc_helper.h b/drivers/gpu/drm/msm/disp/msm_dsc_helper.h
> new file mode 100644
> index 000000000000..308069b2b5a4
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/msm_dsc_helper.h
> @@ -0,0 +1,28 @@
> +/* 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.
> + */
> +
> +#define MSM_DSC_SLICE_PER_PKT 1
> +#define DSC_BPP(config) ((config).bits_per_pixel >> 4)

Oh. Please. If you have used (config)->bits_per_pixel here, you wouldn't 
have to use clumsy DSC_BPP(*dsc). It might make sense to add:

static inline drm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
{
     // most probably WARN_ON_ONCE is enough.
     WARN_ON(dsc->bits_per_fixel & 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);
> +}
> +
> +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);
> +int msm_dsc_get_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp);
> +#endif /* MSM_DSC_HELPER_H_ */
>
Dmitry Baryshkov March 30, 2023, 12:49 a.m. UTC | #2
On 30/03/2023 02:18, Jessica Zhang wrote:
> Introduce MSM-specific DSC helper methods, as some calculations are
> common between DP and DSC.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/Makefile              |  1 +
>   drivers/gpu/drm/msm/disp/msm_dsc_helper.c | 74 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/msm/disp/msm_dsc_helper.h | 28 ++++++++++++

Also: outside of disp, please.

>   3 files changed, 103 insertions(+)
>
Jessica Zhang March 30, 2023, 11:06 p.m. UTC | #3
On 3/29/2023 5:40 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> Introduce MSM-specific DSC helper methods, as some calculations are
>> common between DP and DSC.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/Makefile              |  1 +
>>   drivers/gpu/drm/msm/disp/msm_dsc_helper.c | 74 
>> +++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/disp/msm_dsc_helper.h | 28 ++++++++++++
>>   3 files changed, 103 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index 7274c41228ed..897a5b1c88f6 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -90,6 +90,7 @@ msm-y += \
>>       disp/mdp_kms.o \
>>       disp/msm_disp_snapshot.o \
>>       disp/msm_disp_snapshot_util.o \
>> +    disp/msm_dsc_helper.o \
>>       msm_atomic.o \
>>       msm_atomic_tracepoints.o \
>>       msm_debugfs.o \
>> diff --git a/drivers/gpu/drm/msm/disp/msm_dsc_helper.c 
>> b/drivers/gpu/drm/msm/disp/msm_dsc_helper.c
>> new file mode 100644
>> index 000000000000..ec15c0d829e8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/msm_dsc_helper.c
>> @@ -0,0 +1,74 @@
>> +// 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 int get_comp_ratio(struct drm_dsc_config *dsc, u32 src_bpp)
>> +{
>> +    return mult_frac(100, src_bpp, DSC_BPP(*dsc));
>> +}
>> +
>> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int 
>> intf_width, int comp_ratio)
>> +{
>> +    s64 comp_ratio_fp, num_bits_fp;
>> +    s64 numerator_fp, denominator_fp;
>> +
>> +    comp_ratio_fp = drm_fixp_from_fraction(comp_ratio, 100);
> 
> Please inline comp_ration calculation here. Don't use mult_frac().
> 
>> +    num_bits_fp = drm_fixp_from_fraction(8, 1);
> 
> drm_int2fixp
> 
>> +
>> +    numerator_fp = drm_fixp_from_fraction(dsc->slice_width * 
>> dsc->bits_per_component * 3, 1);
> 
> And here too.
> 
>> +    denominator_fp = drm_fixp_mul(comp_ratio_fp, num_bits_fp);
> 
> And num_bits_fp can be inlined too.
> denominator_fp = drm_fixp_from_fraction(src_bpp * 8, DSC_BPP)
> 
>> +
>> +    return drm_fixp_div(numerator_fp, denominator_fp);
> 
> dsc->slice_width * bpc * 3 / (8 * src_bpp / DSC_BPP), thus:
> 
> drm_fixp_from_fraction(dsc->slice_width * bpc * 3 * DSC_BPP, 8 * src_bpp)
> 
> but I will not insist on this one.
> 
>> +}
>> +
>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int 
>> intf_width, u32 src_bpp)
>> +{
>> +    u32 bytes_per_ss, extra_eol_bytes, bytes_per_intf;
>> +    s64 bytes_per_ss_fp;
>> +    int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
>> +    int comp_ratio = get_comp_ratio(dsc, src_bpp);
>> +
>> +    bytes_per_ss_fp = get_bytes_per_soft_slice(dsc, intf_width, 
>> comp_ratio);
>> +    bytes_per_ss = drm_fixp2int_ceil(bytes_per_ss_fp);
> 
> s/_ss/_soft_slice/g
> 
>> +
>> +    bytes_per_intf = bytes_per_ss * slice_per_intf;
>> +    extra_eol_bytes = bytes_per_intf % 3;
>> +    if (extra_eol_bytes != 0)
>> +        extra_eol_bytes = 3 - extra_eol_bytes;
>> +
>> +    return extra_eol_bytes;
>> +}
>> +
>> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int 
>> intf_width)
>> +{
>> +    u32 bpp;
>> +    u32 dce_bytes_per_line;
>> +
>> +    bpp = DSC_BPP(*dsc);
> 
> Didn't this cause a warning on the unused-but-set variable?
> 
>> +    dce_bytes_per_line = DIV_ROUND_UP(dsc->bits_per_pixel * 
>> intf_width, 8);
>> +
>> +    return dce_bytes_per_line;
>> +}
> 
> If you have msm_dsc_get_slice_per_intf() as a static inline, this 
> function can be a static inline too. Nothing more than a single 
> DIV_ROUND_UP.
> 
>> +
>> +int msm_dsc_get_pclk_per_line(struct drm_dsc_config *dsc, int 
>> intf_width, u32 src_bpp)
>> +{
>> +    s64 data_width;
>> +    int comp_ratio = get_comp_ratio(dsc, src_bpp);
>> +
>> +    if (!dsc->slice_width || (intf_width < dsc->slice_width))
>> +        return -EINVAL;
>> +
>> +    data_width = get_bytes_per_soft_slice(dsc, intf_width, comp_ratio);
>> +    data_width = drm_fixp_mul(dsc->slice_count, data_width);
>> +    data_width = drm_fixp_from_fraction(data_width, 3);
> 
> Reusing a variable is a nice trick, but it can be confusing. Not to 
> mention that the last call should probably be drm_fixp_div()
> 

Hi Dmitry,

Acked (for all the comments here).

Planning to move the last divide by 3 out of this method (as the value 
that uncompressed pclk is divided by depends on DSI/DP and if widebus is 
enabled), so I'll merge the get_bytes_per_soft_slice call with the 2nd line.

Thanks,

Jessica Zhang

>> +
>> +    return drm_fixp2int_ceil(data_width);
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/msm_dsc_helper.h 
>> b/drivers/gpu/drm/msm/disp/msm_dsc_helper.h
>> new file mode 100644
>> index 000000000000..308069b2b5a4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/msm_dsc_helper.h
>> @@ -0,0 +1,28 @@
>> +/* 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.
>> + */
>> +
>> +#define MSM_DSC_SLICE_PER_PKT 1
>> +#define DSC_BPP(config) ((config).bits_per_pixel >> 4)
> 
> Oh. Please. If you have used (config)->bits_per_pixel here, you wouldn't 
> have to use clumsy DSC_BPP(*dsc). It might make sense to add:
> 
> static inline drm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
> {
>      // most probably WARN_ON_ONCE is enough.
>      WARN_ON(dsc->bits_per_fixel & 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);
>> +}
>> +
>> +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);
>> +int msm_dsc_get_pclk_per_line(struct drm_dsc_config *dsc, int 
>> intf_width, u32 src_bpp);
>> +#endif /* MSM_DSC_HELPER_H_ */
>>
> 
> -- 
> With best wishes
> Dmitry
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 7274c41228ed..897a5b1c88f6 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -90,6 +90,7 @@  msm-y += \
 	disp/mdp_kms.o \
 	disp/msm_disp_snapshot.o \
 	disp/msm_disp_snapshot_util.o \
+	disp/msm_dsc_helper.o \
 	msm_atomic.o \
 	msm_atomic_tracepoints.o \
 	msm_debugfs.o \
diff --git a/drivers/gpu/drm/msm/disp/msm_dsc_helper.c b/drivers/gpu/drm/msm/disp/msm_dsc_helper.c
new file mode 100644
index 000000000000..ec15c0d829e8
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/msm_dsc_helper.c
@@ -0,0 +1,74 @@ 
+// 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 int get_comp_ratio(struct drm_dsc_config *dsc, u32 src_bpp)
+{
+	return mult_frac(100, src_bpp, DSC_BPP(*dsc));
+}
+
+static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int intf_width, int comp_ratio)
+{
+	s64 comp_ratio_fp, num_bits_fp;
+	s64 numerator_fp, denominator_fp;
+
+	comp_ratio_fp = drm_fixp_from_fraction(comp_ratio, 100);
+	num_bits_fp = drm_fixp_from_fraction(8, 1);
+
+	numerator_fp = drm_fixp_from_fraction(dsc->slice_width * dsc->bits_per_component * 3, 1);
+	denominator_fp = drm_fixp_mul(comp_ratio_fp, num_bits_fp);
+
+	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_ss, extra_eol_bytes, bytes_per_intf;
+	s64 bytes_per_ss_fp;
+	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
+	int comp_ratio = get_comp_ratio(dsc, src_bpp);
+
+	bytes_per_ss_fp = get_bytes_per_soft_slice(dsc, intf_width, comp_ratio);
+	bytes_per_ss = drm_fixp2int_ceil(bytes_per_ss_fp);
+
+	bytes_per_intf = bytes_per_ss * slice_per_intf;
+	extra_eol_bytes = bytes_per_intf % 3;
+	if (extra_eol_bytes != 0)
+		extra_eol_bytes = 3 - extra_eol_bytes;
+
+	return extra_eol_bytes;
+}
+
+u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int intf_width)
+{
+	u32 bpp;
+	u32 dce_bytes_per_line;
+
+	bpp = DSC_BPP(*dsc);
+	dce_bytes_per_line = DIV_ROUND_UP(dsc->bits_per_pixel * intf_width, 8);
+
+	return dce_bytes_per_line;
+}
+
+int msm_dsc_get_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp)
+{
+	s64 data_width;
+	int comp_ratio = get_comp_ratio(dsc, src_bpp);
+
+	if (!dsc->slice_width || (intf_width < dsc->slice_width))
+		return -EINVAL;
+
+	data_width = get_bytes_per_soft_slice(dsc, intf_width, comp_ratio);
+	data_width = drm_fixp_mul(dsc->slice_count, data_width);
+	data_width = drm_fixp_from_fraction(data_width, 3);
+
+	return drm_fixp2int_ceil(data_width);
+}
diff --git a/drivers/gpu/drm/msm/disp/msm_dsc_helper.h b/drivers/gpu/drm/msm/disp/msm_dsc_helper.h
new file mode 100644
index 000000000000..308069b2b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/msm_dsc_helper.h
@@ -0,0 +1,28 @@ 
+/* 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.
+ */
+
+#define MSM_DSC_SLICE_PER_PKT 1
+#define DSC_BPP(config) ((config).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);
+}
+
+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);
+int msm_dsc_get_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp);
+#endif /* MSM_DSC_HELPER_H_ */