[PATCH RFC v2 0/6] Introduce MSM-specific DSC helpers

[PATCH RFC v2 0/6] Introduce MSM-specific DSC helpers

[Jessica Zhang]

There are some overlap in calculations for MSM-specific DSC variables between DP and DSI. In addition, the calculations for initial_scale_value and det_thresh_flatness that are defined within the DSC 1.2 specifications, but aren't yet included in drm_dsc_helper.c.

This series moves these calculations to a shared msm_dsc_helper.c file and defines drm_dsc_helper methods for initial_scale_value and det_thresh_flatness.

Note: For now, the MSM specific helper methods are only called for the DSI path, but will called for DP once DSC 1.2 support for DP has been added.

Depends on: "drm/i915: move DSC RC tables to drm_dsc_helper.c" [1]

[1] https://patchwork.freedesktop.org/series/114472/

---
Changes in v2:
- Changed det_thresh_flatness to flatness_det_thresh
- Moved msm_dsc_helper files to msm/ directory
- Fixed type mismatch issues in MSM DSC helpers
- Dropped MSM_DSC_SLICE_PER_PKT macro
- Removed get_comp_ratio() helper
- Style changes to improve readability
- Use drm_dsc_get_bpp_int() instead of DSC_BPP macro
- Picked up Fixes tags for patches 3/5 and 4/5
- Picked up Reviewed-by for patch 4/5
- Split eol_byte_num and pkt_per_line calculation into a separate patch
- Moved pclk_per_line calculation into `if (dsc)` block in
  dsi_timing_setup()
- Link to v1: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v1-0-f3e479f59b6d@quicinc.com

---
Jessica Zhang (6):
      drm/display/dsc: Add flatness and initial scale value calculations
      drm/msm: Add MSM-specific DSC helper methods
      drm/msm/dpu: Use DRM DSC helper for det_thresh_flatness
      drm/msm/dpu: Fix slice_last_group_size calculation
      drm/msm/dsi: Use MSM and DRM DSC helper methods
      drm/msm/dsi: Fix calculations for eol_byte_num and pkt_per_line

 drivers/gpu/drm/msm/Makefile               |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 10 ++++--
 drivers/gpu/drm/msm/dsi/dsi_host.c         | 21 ++++++++----
 drivers/gpu/drm/msm/msm_dsc_helper.c       | 53 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_dsc_helper.h       | 42 +++++++++++++++++++++++
 include/drm/display/drm_dsc_helper.h       | 11 +++++++
 6 files changed, 129 insertions(+), 9 deletions(-)
---
base-commit: 56777fc93a145afcf71b92ba4281250f59ba6d9b
change-id: 20230329-rfc-msm-dsc-helper-981a95edfbd0

Best regards,
-- 
Jessica Zhang <quic_jesszhan@quicinc.com>

[PATCH RFC v2 1/6] drm/display/dsc: Add flatness and initial scale value calculations

[Jessica Zhang]

Add helpers to calculate det_thresh_flatness and initial_scale_value as
these calculations are defined within the DSC spec.

Changes in v2:
- Renamed det_thresh_flatness to flatness_det_thresh
- Set initial_scale_value directly in helper

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 include/drm/display/drm_dsc_helper.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
index 4448c482b092..f6bc268c1719 100644
--- a/include/drm/display/drm_dsc_helper.h
+++ b/include/drm/display/drm_dsc_helper.h
@@ -17,6 +17,17 @@ enum drm_dsc_params_kind {
 	DRM_DSC_1_2_420,
 };
 
+static inline void drm_dsc_set_initial_scale_value(struct drm_dsc_config *dsc)
+{
+	dsc->initial_scale_value = 8 * dsc->rc_model_size /
+		(dsc->rc_model_size - dsc->initial_offset);
+}
+
+static inline int drm_dsc_calculate_flatness_det_thresh(struct drm_dsc_config *dsc)
+{
+	return 2 << (dsc->bits_per_component - 8);
+}
+
 void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
 int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
 void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,

-- 
2.39.2

Re: [PATCH RFC v2 1/6] drm/display/dsc: Add flatness and initial scale value calculations

[Dmitry Baryshkov]

On 31/03/2023 21:49, Jessica Zhang wrote:
> Add helpers to calculate det_thresh_flatness and initial_scale_value as
> these calculations are defined within the DSC spec.
> 
> Changes in v2:
> - Renamed det_thresh_flatness to flatness_det_thresh
> - Set initial_scale_value directly in helper
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   include/drm/display/drm_dsc_helper.h | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

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

[Jessica Zhang]

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@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)
+{
+	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);
+	denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, drm_int2fixp(bpp));
+
+	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;
+
+	return extra_eol_bytes;
+}
+
+int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp)
+{
+	s64 data_width;
+
+	if (!dsc->slice_width || (intf_width < dsc->slice_width))
+		return -EINVAL;
+
+	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_ */

-- 
2.39.2

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

[Dmitry Baryshkov]

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@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

> +{
> +	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.

> +	denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, drm_int2fixp(bpp));

denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp);

> +
> +	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.

> +
> +	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.

> +
> +	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

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

[Jessica Zhang]

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@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.

> 
>> +    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.

> 
>> +
>> +    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
> 

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

[Dmitry Baryshkov]

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@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

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

[Jessica Zhang]

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@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
> 

Re: [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@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

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

[Jessica Zhang]

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@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.

Hey Dmitry,

Sorry, forgot to respond to this comment in my earlier reply.

This was a mistake in the v2 code -- should have used 
dsc->bits_per_component instead of bpp and did not catch it during 
validation as bpp == bpc for the panel I'm testing on.

Will use bits_per_compnent going forward.

Thanks,

Jessica Zhang

> 
>>
>>>
>>>> +    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
> 

[PATCH RFC v2 3/6] drm/msm/dpu: Use DRM DSC helper for det_thresh_flatness

[Jessica Zhang]

Use the DRM DSC helper for det_thresh_flatness to match downstream
implementation and the DSC spec.

Changes in V2:
- Added a Fixes tag

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index 619926da1441..b952f7d2b7f5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2020-2022, Linaro Limited
  */
 
+#include <drm/display/drm_dsc_helper.h>
+
 #include "dpu_kms.h"
 #include "dpu_hw_catalog.h"
 #include "dpu_hwio.h"
@@ -102,7 +104,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 	data |= dsc->final_offset;
 	DPU_REG_WRITE(c, DSC_DSC_OFFSET, data);
 
-	det_thresh_flatness = 7 + 2 * (dsc->bits_per_component - 8);
+	det_thresh_flatness = drm_dsc_calculate_flatness_det_thresh(dsc);
 	data = det_thresh_flatness << 10;
 	data |= dsc->flatness_max_qp << 5;
 	data |= dsc->flatness_min_qp;

-- 
2.39.2

Re: [PATCH RFC v2 3/6] drm/msm/dpu: Use DRM DSC helper for det_thresh_flatness

[Dmitry Baryshkov]

On 31/03/2023 21:49, Jessica Zhang wrote:
> Use the DRM DSC helper for det_thresh_flatness to match downstream
> implementation and the DSC spec.
> 
> Changes in V2:
> - Added a Fixes tag
> 
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH RFC v2 4/6] drm/msm/dpu: Fix slice_last_group_size calculation

[Jessica Zhang]

Correct the math for slice_last_group_size so that it matches the
calculations downstream.

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index b952f7d2b7f5..9312a8d7fbd9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 	if (is_cmd_mode)
 		initial_lines += 1;
 
-	slice_last_group_size = 3 - (dsc->slice_width % 3);
+	slice_last_group_size = dsc->slice_width % 3;
+
+	if (slice_last_group_size == 0)
+		slice_last_group_size = 3;
+
 	data = (initial_lines << 20);
 	data |= ((slice_last_group_size - 1) << 18);
 	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */

-- 
2.39.2

Re: [PATCH RFC v2 4/6] drm/msm/dpu: Fix slice_last_group_size calculation

[Dmitry Baryshkov]

On 31/03/2023 21:49, Jessica Zhang wrote:
> Correct the math for slice_last_group_size so that it matches the
> calculations downstream.
> 
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index b952f7d2b7f5..9312a8d7fbd9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   	if (is_cmd_mode)
>   		initial_lines += 1;
>   
> -	slice_last_group_size = 3 - (dsc->slice_width % 3);
> +	slice_last_group_size = dsc->slice_width % 3;
> +
> +	if (slice_last_group_size == 0)
> +		slice_last_group_size = 3;

Hmm. As I went on checking this against techpack:

mod = dsc->slice_width % 3

mod | techpack | old | your_patch
0   | 2        | 3   | 3
1   | 0        | 2   | 1
2   | 1        | 1   | 2

So, obviously neither old nor new code match the calculations of the 
techpack. If we assume that sde_dsc_helper code is correct (which I have 
no reasons to doubt), then the proper code should be:

slice_last_group_size = (dsc->slice_width + 2) % 3;

Could you please doublecheck and adjust.

> +
>   	data = (initial_lines << 20);
>   	data |= ((slice_last_group_size - 1) << 18);
>   	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
> 

-- 
With best wishes
Dmitry

Re: [PATCH RFC v2 4/6] drm/msm/dpu: Fix slice_last_group_size calculation

[Jessica Zhang]

On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote:
> On 31/03/2023 21:49, Jessica Zhang wrote:
>> Correct the math for slice_last_group_size so that it matches the
>> calculations downstream.
>>
>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> index b952f7d2b7f5..9312a8d7fbd9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc 
>> *hw_dsc,
>>       if (is_cmd_mode)
>>           initial_lines += 1;
>> -    slice_last_group_size = 3 - (dsc->slice_width % 3);
>> +    slice_last_group_size = dsc->slice_width % 3;
>> +
>> +    if (slice_last_group_size == 0)
>> +        slice_last_group_size = 3;
> 
> Hmm. As I went on checking this against techpack:
> 
> mod = dsc->slice_width % 3
> 
> mod | techpack | old | your_patch
> 0   | 2        | 3   | 3
> 1   | 0        | 2   | 1
> 2   | 1        | 1   | 2
> 
> So, obviously neither old nor new code match the calculations of the 
> techpack. If we assume that sde_dsc_helper code is correct (which I have 
> no reasons to doubt), then the proper code should be:
> 
> slice_last_group_size = (dsc->slice_width + 2) % 3;
> 
> Could you please doublecheck and adjust.

Hi Dmitry,

The calculation should match the techpack calculation (I kept the `data 
|= ((slice_last_group_size - 1) << 18);` a few lines down).

Thanks,

Jessica Zhang

> 
>> +
>>       data = (initial_lines << 20);
>>       data |= ((slice_last_group_size - 1) << 18);
>>       /* bpp is 6.4 format, 4 LSBs bits are for fractional part */
>>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH RFC v2 4/6] drm/msm/dpu: Fix slice_last_group_size calculation

[Dmitry Baryshkov]

On 04/04/2023 00:45, Jessica Zhang wrote:
> 
> 
> On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote:
>> On 31/03/2023 21:49, Jessica Zhang wrote:
>>> Correct the math for slice_last_group_size so that it matches the
>>> calculations downstream.
>>>
>>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>> index b952f7d2b7f5..9312a8d7fbd9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc 
>>> *hw_dsc,
>>>       if (is_cmd_mode)
>>>           initial_lines += 1;
>>> -    slice_last_group_size = 3 - (dsc->slice_width % 3);
>>> +    slice_last_group_size = dsc->slice_width % 3;
>>> +
>>> +    if (slice_last_group_size == 0)
>>> +        slice_last_group_size = 3;
>>
>> Hmm. As I went on checking this against techpack:
>>
>> mod = dsc->slice_width % 3
>>
>> mod | techpack | old | your_patch
>> 0   | 2        | 3   | 3
>> 1   | 0        | 2   | 1
>> 2   | 1        | 1   | 2
>>
>> So, obviously neither old nor new code match the calculations of the 
>> techpack. If we assume that sde_dsc_helper code is correct (which I 
>> have no reasons to doubt), then the proper code should be:
>>
>> slice_last_group_size = (dsc->slice_width + 2) % 3;
>>
>> Could you please doublecheck and adjust.
> 
> Hi Dmitry,
> 
> The calculation should match the techpack calculation (I kept the `data 
> |= ((slice_last_group_size - 1) << 18);` a few lines down).

And the techpack doesn't have -1.

I think the following code piece would be more convenient as it is simpler:

slice_last_group_size = (dsc->slice_width + 2) % 3;
[...]
data |= slice_last_group_size << 18;

If you agree, could you please switch to it?

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>> +
>>>       data = (initial_lines << 20);
>>>       data |= ((slice_last_group_size - 1) << 18);
>>>       /* bpp is 6.4 format, 4 LSBs bits are for fractional part */
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH RFC v2 4/6] drm/msm/dpu: Fix slice_last_group_size calculation

[Jessica Zhang]

On 4/3/2023 2:51 PM, Dmitry Baryshkov wrote:
> On 04/04/2023 00:45, Jessica Zhang wrote:
>>
>>
>> On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote:
>>> On 31/03/2023 21:49, Jessica Zhang wrote:
>>>> Correct the math for slice_last_group_size so that it matches the
>>>> calculations downstream.
>>>>
>>>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>>> index b952f7d2b7f5..9312a8d7fbd9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>>>> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc 
>>>> *hw_dsc,
>>>>       if (is_cmd_mode)
>>>>           initial_lines += 1;
>>>> -    slice_last_group_size = 3 - (dsc->slice_width % 3);
>>>> +    slice_last_group_size = dsc->slice_width % 3;
>>>> +
>>>> +    if (slice_last_group_size == 0)
>>>> +        slice_last_group_size = 3;
>>>
>>> Hmm. As I went on checking this against techpack:
>>>
>>> mod = dsc->slice_width % 3
>>>
>>> mod | techpack | old | your_patch
>>> 0   | 2        | 3   | 3
>>> 1   | 0        | 2   | 1
>>> 2   | 1        | 1   | 2
>>>
>>> So, obviously neither old nor new code match the calculations of the 
>>> techpack. If we assume that sde_dsc_helper code is correct (which I 
>>> have no reasons to doubt), then the proper code should be:
>>>
>>> slice_last_group_size = (dsc->slice_width + 2) % 3;
>>>
>>> Could you please doublecheck and adjust.
>>
>> Hi Dmitry,
>>
>> The calculation should match the techpack calculation (I kept the 
>> `data |= ((slice_last_group_size - 1) << 18);` a few lines down).
> 
> And the techpack doesn't have -1.
> 
> I think the following code piece would be more convenient as it is simpler:
> 
> slice_last_group_size = (dsc->slice_width + 2) % 3;
> [...]
> data |= slice_last_group_size << 18;
> 
> If you agree, could you please switch to it?

Sure.

Thanks,

Jessica Zhang

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> +
>>>>       data = (initial_lines << 20);
>>>>       data |= ((slice_last_group_size - 1) << 18);
>>>>       /* bpp is 6.4 format, 4 LSBs bits are for fractional part */
>>>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
>>>
> 
> -- 
> With best wishes
> Dmitry
> 

[PATCH RFC v2 5/6] drm/msm/dsi: Use MSM and DRM DSC helper methods

[Jessica Zhang]

Use MSM and DRM DSC helper methods to configure DSC for DSI.

Changes in V2:
- *_calculate_initial_scale_value --> *_set_initial_scale_value
- Split pkt_per_line and eol_byte_num changes to a separate patch
- Moved pclk_per_line calculation to hdisplay adjustment in `if (dsc)`
  block of dsi_update_dsc_timing()

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 74d38f90398a..b7ab81737473 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -28,6 +28,7 @@
 #include "dsi.xml.h"
 #include "sfpb.xml.h"
 #include "dsi_cfg.h"
+#include "msm_dsc_helper.h"
 #include "msm_kms.h"
 #include "msm_gem.h"
 #include "phy/dsi_phy.h"
@@ -848,7 +849,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	/* first calculate dsc parameters and then program
 	 * compress mode registers
 	 */
-	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
+	slice_per_intf = msm_dsc_get_slice_per_intf(dsc, hdisplay);
 
 	/*
 	 * If slice_count is greater than slice_per_intf
@@ -951,7 +952,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * pulse width same
 		 */
 		h_total -= hdisplay;
-		hdisplay /= 3;
+		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
+			hdisplay = msm_dsc_get_uncompressed_pclk_per_line(dsc, hdisplay,
+					dsi_get_bpp(msm_host->format)) / 3;
+		else
+			hdisplay /= 3;
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}
@@ -1759,7 +1764,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 		return ret;
 	}
 
-	dsc->initial_scale_value = 32;
+	drm_dsc_set_initial_scale_value(dsc);
 	dsc->line_buf_depth = dsc->bits_per_component + 1;
 
 	return drm_dsc_compute_rc_parameters(dsc);

-- 
2.39.2

Re: [PATCH RFC v2 5/6] drm/msm/dsi: Use MSM and DRM DSC helper methods

[Dmitry Baryshkov]

On 31/03/2023 21:49, Jessica Zhang wrote:
> Use MSM and DRM DSC helper methods to configure DSC for DSI.
> 
> Changes in V2:
> - *_calculate_initial_scale_value --> *_set_initial_scale_value
> - Split pkt_per_line and eol_byte_num changes to a separate patch
> - Moved pclk_per_line calculation to hdisplay adjustment in `if (dsc)`
>    block of dsi_update_dsc_timing()
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 74d38f90398a..b7ab81737473 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -28,6 +28,7 @@
>   #include "dsi.xml.h"
>   #include "sfpb.xml.h"
>   #include "dsi_cfg.h"
> +#include "msm_dsc_helper.h"
>   #include "msm_kms.h"
>   #include "msm_gem.h"
>   #include "phy/dsi_phy.h"
> @@ -848,7 +849,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   	/* first calculate dsc parameters and then program
>   	 * compress mode registers
>   	 */
> -	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
> +	slice_per_intf = msm_dsc_get_slice_per_intf(dsc, hdisplay);
>   
>   	/*
>   	 * If slice_count is greater than slice_per_intf
> @@ -951,7 +952,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   		 * pulse width same
>   		 */
>   		h_total -= hdisplay;
> -		hdisplay /= 3;
> +		if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> +			hdisplay = msm_dsc_get_uncompressed_pclk_per_line(dsc, hdisplay,
> +					dsi_get_bpp(msm_host->format)) / 3;
> +		else
> +			hdisplay /= 3;
>   		h_total += hdisplay;
>   		ha_end = ha_start + hdisplay;

This chunk changes the calculated value (two other are mere updates to 
use new functions). Please move it to a separate patch, add proper 
description/justification and possibly a Fixes tag, if the original code 
was incorrect.

>   	}
> @@ -1759,7 +1764,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>   		return ret;
>   	}
>   
> -	dsc->initial_scale_value = 32;
> +	drm_dsc_set_initial_scale_value(dsc);
>   	dsc->line_buf_depth = dsc->bits_per_component + 1;
>   
>   	return drm_dsc_compute_rc_parameters(dsc);
> 

-- 
With best wishes
Dmitry

Re: [PATCH RFC v2 5/6] drm/msm/dsi: Use MSM and DRM DSC helper methods

[Jessica Zhang]

On 4/2/2023 4:29 AM, Dmitry Baryshkov wrote:
> On 31/03/2023 21:49, Jessica Zhang wrote:
>> Use MSM and DRM DSC helper methods to configure DSC for DSI.
>>
>> Changes in V2:
>> - *_calculate_initial_scale_value --> *_set_initial_scale_value
>> - Split pkt_per_line and eol_byte_num changes to a separate patch
>> - Moved pclk_per_line calculation to hdisplay adjustment in `if (dsc)`
>>    block of dsi_update_dsc_timing()
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 74d38f90398a..b7ab81737473 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -28,6 +28,7 @@
>>   #include "dsi.xml.h"
>>   #include "sfpb.xml.h"
>>   #include "dsi_cfg.h"
>> +#include "msm_dsc_helper.h"
>>   #include "msm_kms.h"
>>   #include "msm_gem.h"
>>   #include "phy/dsi_phy.h"
>> @@ -848,7 +849,7 @@ static void dsi_update_dsc_timing(struct 
>> msm_dsi_host *msm_host, bool is_cmd_mod
>>       /* first calculate dsc parameters and then program
>>        * compress mode registers
>>        */
>> -    slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
>> +    slice_per_intf = msm_dsc_get_slice_per_intf(dsc, hdisplay);
>>       /*
>>        * If slice_count is greater than slice_per_intf
>> @@ -951,7 +952,11 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>            * pulse width same
>>            */
>>           h_total -= hdisplay;
>> -        hdisplay /= 3;
>> +        if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>> +            hdisplay = msm_dsc_get_uncompressed_pclk_per_line(dsc, 
>> hdisplay,
>> +                    dsi_get_bpp(msm_host->format)) / 3;
>> +        else
>> +            hdisplay /= 3;
>>           h_total += hdisplay;
>>           ha_end = ha_start + hdisplay;
> 
> This chunk changes the calculated value (two other are mere updates to 
> use new functions). Please move it to a separate patch, add proper 
> description/justification and possibly a Fixes tag, if the original code 
> was incorrect.

Hi Dmitry,

Acked.

Thanks,

Jessica Zhang

> 
>>       }
>> @@ -1759,7 +1764,7 @@ static int dsi_populate_dsc_params(struct 
>> msm_dsi_host *msm_host, struct drm_dsc
>>           return ret;
>>       }
>> -    dsc->initial_scale_value = 32;
>> +    drm_dsc_set_initial_scale_value(dsc);
>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>       return drm_dsc_compute_rc_parameters(dsc);
>>
> 
> -- 
> With best wishes
> Dmitry
> 

[PATCH RFC v2 6/6] drm/msm/dsi: Fix calculations for eol_byte_num and pkt_per_line

[Jessica Zhang]

Use the correct calculations for eol_byte_num and pkt_per_line.

Currently, pkt_per_line is calculated by dividing slice_per_intf by
slice_count. This is incorrect, as slice_per_intf should be divided by
slice_per_pkt, which is not always equivalent to slice_count as it is
possible for there to be multiple soft slices per interface even though
a panel only specifies one slice per packet.

For eol_byte_num, the current calculation describes the size of the
trailing bytes in the line. Change the calculation so that it describes
the number of padding bytes instead.

Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index b7ab81737473..613ec19f4383 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -842,7 +842,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 {
 	struct drm_dsc_config *dsc = msm_host->dsc;
 	u32 reg, reg_ctrl, reg_ctrl2;
-	u32 slice_per_intf, total_bytes_per_intf;
+	u32 slice_per_intf;
 	u32 pkt_per_line;
 	u32 eol_byte_num;
 
@@ -859,10 +859,12 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	if (dsc->slice_count > slice_per_intf)
 		dsc->slice_count = 1;
 
-	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
+	eol_byte_num = msm_dsc_get_eol_byte_num(dsc, hdisplay, dsi_get_bpp(msm_host->format));
 
-	eol_byte_num = total_bytes_per_intf % 3;
-	pkt_per_line = slice_per_intf / dsc->slice_count;
+	/* Default to 1 slice_per_pkt, so pkt_per_line will be equal to
+	 * slice per intf.
+	 */
+	pkt_per_line = slice_per_intf;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);

-- 
2.39.2

Re: [PATCH RFC v2 6/6] drm/msm/dsi: Fix calculations for eol_byte_num and pkt_per_line

[Dmitry Baryshkov]

On 31/03/2023 21:49, Jessica Zhang wrote:
> Use the correct calculations for eol_byte_num and pkt_per_line.

Nit: this line duplicates commit subject and thus is mostly useless.

> 
> Currently, pkt_per_line is calculated by dividing slice_per_intf by
> slice_count. This is incorrect, as slice_per_intf should be divided by
> slice_per_pkt, which is not always equivalent to slice_count as it is
> possible for there to be multiple soft slices per interface even though
> a panel only specifies one slice per packet.
> 
> For eol_byte_num, the current calculation describes the size of the
> trailing bytes in the line. Change the calculation so that it describes
> the number of padding bytes instead.
> 
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry