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

[PATCH RFC 0/5] 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/

---
Jessica Zhang (5):
      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

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

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

[PATCH RFC 0/5] 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/

---
Jessica Zhang (5):
      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

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

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

[PATCH RFC 1/5] 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.

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

diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
index 4448c482b092..63175650a45e 100644
--- a/include/drm/display/drm_dsc_helper.h
+++ b/include/drm/display/drm_dsc_helper.h
@@ -17,6 +17,16 @@ enum drm_dsc_params_kind {
 	DRM_DSC_1_2_420,
 };
 
+static inline int drm_dsc_calculate_initial_scale_value(struct drm_dsc_config *dsc)
+{
+	return 8 * dsc->rc_model_size / (dsc->rc_model_size - dsc->initial_offset);
+}
+
+static inline int drm_dsc_calculate_det_thresh_flatness(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

[PATCH RFC 1/5] 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.

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

diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
index 4448c482b092..63175650a45e 100644
--- a/include/drm/display/drm_dsc_helper.h
+++ b/include/drm/display/drm_dsc_helper.h
@@ -17,6 +17,16 @@ enum drm_dsc_params_kind {
 	DRM_DSC_1_2_420,
 };
 
+static inline int drm_dsc_calculate_initial_scale_value(struct drm_dsc_config *dsc)
+{
+	return 8 * dsc->rc_model_size / (dsc->rc_model_size - dsc->initial_offset);
+}
+
+static inline int drm_dsc_calculate_det_thresh_flatness(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

[PATCH RFC 2/5] 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.

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);
+	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_ */

-- 
2.39.2

[PATCH RFC 2/5] 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.

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);
+	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_ */

-- 
2.39.2

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

[Jessica Zhang]

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..648c530b5d05 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_det_thresh_flatness(dsc);
 	data = det_thresh_flatness << 10;
 	data |= dsc->flatness_max_qp << 5;
 	data |= dsc->flatness_min_qp;

-- 
2.39.2

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

[Jessica Zhang]

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..648c530b5d05 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_det_thresh_flatness(dsc);
 	data = det_thresh_flatness << 10;
 	data |= dsc->flatness_max_qp << 5;
 	data |= dsc->flatness_min_qp;

-- 
2.39.2

[PATCH RFC 4/5] 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.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 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 648c530b5d05..1a1a0f6523f6 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

[PATCH RFC 4/5] 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.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 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 648c530b5d05..1a1a0f6523f6 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

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

[Jessica Zhang]

Use MSM and DRM DSC helper methods.

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

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 74d38f90398a..7419fe58a941 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -31,6 +31,7 @@
 #include "msm_kms.h"
 #include "msm_gem.h"
 #include "phy/dsi_phy.h"
+#include "disp/msm_dsc_helper.h"
 
 #define DSI_RESET_TOGGLE_DELAY_MS 20
 
@@ -841,14 +842,14 @@ 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;
 
 	/* 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
@@ -858,10 +859,10 @@ 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(msm_host->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;
+	pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	DBG("");
 
+	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
+		/* Default widebus_en to false for now. */
+		hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, mode->hdisplay,
+				dsi_get_bpp(msm_host->format));
+
 	/*
 	 * For bonded DSI mode, the current DRM mode has
 	 * the complete width of the panel. Since, the complete
@@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 		return ret;
 	}
 
-	dsc->initial_scale_value = 32;
+	dsc->initial_scale_value = drm_dsc_calculate_initial_scale_value(dsc);
 	dsc->line_buf_depth = dsc->bits_per_component + 1;
 
 	return drm_dsc_compute_rc_parameters(dsc);

-- 
2.39.2

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

[Jessica Zhang]

Use MSM and DRM DSC helper methods.

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

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 74d38f90398a..7419fe58a941 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -31,6 +31,7 @@
 #include "msm_kms.h"
 #include "msm_gem.h"
 #include "phy/dsi_phy.h"
+#include "disp/msm_dsc_helper.h"
 
 #define DSI_RESET_TOGGLE_DELAY_MS 20
 
@@ -841,14 +842,14 @@ 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;
 
 	/* 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
@@ -858,10 +859,10 @@ 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(msm_host->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;
+	pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	DBG("");
 
+	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
+		/* Default widebus_en to false for now. */
+		hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, mode->hdisplay,
+				dsi_get_bpp(msm_host->format));
+
 	/*
 	 * For bonded DSI mode, the current DRM mode has
 	 * the complete width of the panel. Since, the complete
@@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 		return ret;
 	}
 
-	dsc->initial_scale_value = 32;
+	dsc->initial_scale_value = drm_dsc_calculate_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 1/5] drm/display/dsc: Add flatness and initial scale value calculations

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> Add helpers to calculate det_thresh_flatness and initial_scale_value as
> these calculations are defined within the DSC spec.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   include/drm/display/drm_dsc_helper.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> index 4448c482b092..63175650a45e 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -17,6 +17,16 @@ enum drm_dsc_params_kind {
>   	DRM_DSC_1_2_420,
>   };
>   
> +static inline int drm_dsc_calculate_initial_scale_value(struct drm_dsc_config *dsc)
> +{
> +	return 8 * dsc->rc_model_size / (dsc->rc_model_size - dsc->initial_offset);

Just set this in drm_dsc_config, like other functions do.

> +}
> +
> +static inline int drm_dsc_calculate_det_thresh_flatness(struct drm_dsc_config *dsc)
> +{
> +	return 2 << (dsc->bits_per_component - 8);
> +}

this is flatness_det_thresh, per the standard.

Otherwise LGTM

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> Add helpers to calculate det_thresh_flatness and initial_scale_value as
> these calculations are defined within the DSC spec.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   include/drm/display/drm_dsc_helper.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> index 4448c482b092..63175650a45e 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -17,6 +17,16 @@ enum drm_dsc_params_kind {
>   	DRM_DSC_1_2_420,
>   };
>   
> +static inline int drm_dsc_calculate_initial_scale_value(struct drm_dsc_config *dsc)
> +{
> +	return 8 * dsc->rc_model_size / (dsc->rc_model_size - dsc->initial_offset);

Just set this in drm_dsc_config, like other functions do.

> +}
> +
> +static inline int drm_dsc_calculate_det_thresh_flatness(struct drm_dsc_config *dsc)
> +{
> +	return 2 << (dsc->bits_per_component - 8);
> +}

this is flatness_det_thresh, per the standard.

Otherwise LGTM

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> 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..648c530b5d05 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_det_thresh_flatness(dsc);

But this changes the value! Compare:

bpc | old | new
8   | 7   | 2
10  | 11  | 8
12  | 15  | 256

If this is intentional, please state so and maybe add a Fixes tag.


>   	data = det_thresh_flatness << 10;
>   	data |= dsc->flatness_max_qp << 5;
>   	data |= dsc->flatness_min_qp;
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> 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..648c530b5d05 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_det_thresh_flatness(dsc);

But this changes the value! Compare:

bpc | old | new
8   | 7   | 2
10  | 11  | 8
12  | 15  | 256

If this is intentional, please state so and maybe add a Fixes tag.


>   	data = det_thresh_flatness << 10;
>   	data |= dsc->flatness_max_qp << 5;
>   	data |= dsc->flatness_min_qp;
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> Correct the math for slice_last_group_size so that it matches the
> calculations downstream.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
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 648c530b5d05..1a1a0f6523f6 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 */
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> Correct the math for slice_last_group_size so that it matches the
> calculations downstream.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
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 648c530b5d05..1a1a0f6523f6 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 */
> 

-- 
With best wishes
Dmitry

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

[Jessica Zhang]

On 3/29/2023 4:31 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> 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..648c530b5d05 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_det_thresh_flatness(dsc);
> 
> But this changes the value! Compare:
> 
> bpc | old | new
> 8   | 7   | 2
> 10  | 11  | 8
> 12  | 15  | 256
> 
> If this is intentional, please state so and maybe add a Fixes tag.

Hi Dmitry,

Yep this was intentional to match downstream and the spec. Will add a 
fixes tag for this.

Thanks,

Jessica Zhang

> 
> 
>>       data = det_thresh_flatness << 10;
>>       data |= dsc->flatness_max_qp << 5;
>>       data |= dsc->flatness_min_qp;
>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jessica Zhang]

On 3/29/2023 4:31 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> 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..648c530b5d05 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_det_thresh_flatness(dsc);
> 
> But this changes the value! Compare:
> 
> bpc | old | new
> 8   | 7   | 2
> 10  | 11  | 8
> 12  | 15  | 256
> 
> If this is intentional, please state so and maybe add a Fixes tag.

Hi Dmitry,

Yep this was intentional to match downstream and the spec. Will add a 
fixes tag for this.

Thanks,

Jessica Zhang

> 
> 
>>       data = det_thresh_flatness << 10;
>>       data |= dsc->flatness_max_qp << 5;
>>       data |= dsc->flatness_min_qp;
>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jessica Zhang]

On 3/29/2023 4:25 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> Add helpers to calculate det_thresh_flatness and initial_scale_value as
>> these calculations are defined within the DSC spec.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   include/drm/display/drm_dsc_helper.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/drm/display/drm_dsc_helper.h 
>> b/include/drm/display/drm_dsc_helper.h
>> index 4448c482b092..63175650a45e 100644
>> --- a/include/drm/display/drm_dsc_helper.h
>> +++ b/include/drm/display/drm_dsc_helper.h
>> @@ -17,6 +17,16 @@ enum drm_dsc_params_kind {
>>       DRM_DSC_1_2_420,
>>   };
>> +static inline int drm_dsc_calculate_initial_scale_value(struct 
>> drm_dsc_config *dsc)
>> +{
>> +    return 8 * dsc->rc_model_size / (dsc->rc_model_size - 
>> dsc->initial_offset);
> 
> Just set this in drm_dsc_config, like other functions do.

Hi Dmitry,

Thanks for the feedback. Acked for both comments here.

Thanks,

Jessica Zhang

> 
>> +}
>> +
>> +static inline int drm_dsc_calculate_det_thresh_flatness(struct 
>> drm_dsc_config *dsc)
>> +{
>> +    return 2 << (dsc->bits_per_component - 8);
>> +}
> 
> this is flatness_det_thresh, per the standard.
> 
> Otherwise LGTM
> 
>> +
>>   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,
>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jessica Zhang]

On 3/29/2023 4:25 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> Add helpers to calculate det_thresh_flatness and initial_scale_value as
>> these calculations are defined within the DSC spec.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   include/drm/display/drm_dsc_helper.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/drm/display/drm_dsc_helper.h 
>> b/include/drm/display/drm_dsc_helper.h
>> index 4448c482b092..63175650a45e 100644
>> --- a/include/drm/display/drm_dsc_helper.h
>> +++ b/include/drm/display/drm_dsc_helper.h
>> @@ -17,6 +17,16 @@ enum drm_dsc_params_kind {
>>       DRM_DSC_1_2_420,
>>   };
>> +static inline int drm_dsc_calculate_initial_scale_value(struct 
>> drm_dsc_config *dsc)
>> +{
>> +    return 8 * dsc->rc_model_size / (dsc->rc_model_size - 
>> dsc->initial_offset);
> 
> Just set this in drm_dsc_config, like other functions do.

Hi Dmitry,

Thanks for the feedback. Acked for both comments here.

Thanks,

Jessica Zhang

> 
>> +}
>> +
>> +static inline int drm_dsc_calculate_det_thresh_flatness(struct 
>> drm_dsc_config *dsc)
>> +{
>> +    return 2 << (dsc->bits_per_component - 8);
>> +}
> 
> this is flatness_det_thresh, per the standard.
> 
> Otherwise LGTM
> 
>> +
>>   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,
>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> Use MSM and DRM DSC helper methods.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 74d38f90398a..7419fe58a941 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -31,6 +31,7 @@
>   #include "msm_kms.h"
>   #include "msm_gem.h"
>   #include "phy/dsi_phy.h"
> +#include "disp/msm_dsc_helper.h"
>   
>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>   
> @@ -841,14 +842,14 @@ 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;
>   
>   	/* 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);

This looks good

>   
>   	/*
>   	 * If slice_count is greater than slice_per_intf
> @@ -858,10 +859,10 @@ 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(msm_host->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;
> +	pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;

And for these values the result is definitely changed. Separate patch & 
description please. Just in case, "values per downstream kernel" is not 
a proper description for such changes.

>   
>   	if (is_cmd_mode) /* packet data type */
>   		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   	DBG("");
>   
> +	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> +		/* Default widebus_en to false for now. */
> +		hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, mode->hdisplay,
> +				dsi_get_bpp(msm_host->format));
> +

This is definitely something new and thus should probably go into a 
separate patch and be described. Also I'm not sure how does that 
interact with the hdisplay-related calculations below, under the if(dsc) 
clause.

>   	/*
>   	 * For bonded DSI mode, the current DRM mode has
>   	 * the complete width of the panel. Since, the complete
> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>   		return ret;
>   	}
>   
> -	dsc->initial_scale_value = 32;
> +	dsc->initial_scale_value = drm_dsc_calculate_initial_scale_value(dsc);

This is fine, we only support 8bpp where these values match.

>   	dsc->line_buf_depth = dsc->bits_per_component + 1;
>   
>   	return drm_dsc_compute_rc_parameters(dsc);
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:18, Jessica Zhang wrote:
> Use MSM and DRM DSC helper methods.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 74d38f90398a..7419fe58a941 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -31,6 +31,7 @@
>   #include "msm_kms.h"
>   #include "msm_gem.h"
>   #include "phy/dsi_phy.h"
> +#include "disp/msm_dsc_helper.h"
>   
>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>   
> @@ -841,14 +842,14 @@ 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;
>   
>   	/* 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);

This looks good

>   
>   	/*
>   	 * If slice_count is greater than slice_per_intf
> @@ -858,10 +859,10 @@ 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(msm_host->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;
> +	pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;

And for these values the result is definitely changed. Separate patch & 
description please. Just in case, "values per downstream kernel" is not 
a proper description for such changes.

>   
>   	if (is_cmd_mode) /* packet data type */
>   		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   	DBG("");
>   
> +	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> +		/* Default widebus_en to false for now. */
> +		hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, mode->hdisplay,
> +				dsi_get_bpp(msm_host->format));
> +

This is definitely something new and thus should probably go into a 
separate patch and be described. Also I'm not sure how does that 
interact with the hdisplay-related calculations below, under the if(dsc) 
clause.

>   	/*
>   	 * For bonded DSI mode, the current DRM mode has
>   	 * the complete width of the panel. Since, the complete
> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>   		return ret;
>   	}
>   
> -	dsc->initial_scale_value = 32;
> +	dsc->initial_scale_value = drm_dsc_calculate_initial_scale_value(dsc);

This is fine, we only support 8bpp where these values match.

>   	dsc->line_buf_depth = dsc->bits_per_component + 1;
>   
>   	return drm_dsc_compute_rc_parameters(dsc);
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:45, Jessica Zhang wrote:
> 
> 
> On 3/29/2023 4:31 PM, Dmitry Baryshkov wrote:
>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>> 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..648c530b5d05 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_det_thresh_flatness(dsc);
>>
>> But this changes the value! Compare:
>>
>> bpc | old | new
>> 8   | 7   | 2
>> 10  | 11  | 8
>> 12  | 15  | 256
>>
>> If this is intentional, please state so and maybe add a Fixes tag.
> 
> Hi Dmitry,
> 
> Yep this was intentional to match downstream and the spec. Will add a 
> fixes tag for this.

Good! I found corresponding change in msm-4.14, so now I understand why 
previously we had what we had.

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>
>>>       data = det_thresh_flatness << 10;
>>>       data |= dsc->flatness_max_qp << 5;
>>>       data |= dsc->flatness_min_qp;
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 30/03/2023 02:45, Jessica Zhang wrote:
> 
> 
> On 3/29/2023 4:31 PM, Dmitry Baryshkov wrote:
>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>> 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..648c530b5d05 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_det_thresh_flatness(dsc);
>>
>> But this changes the value! Compare:
>>
>> bpc | old | new
>> 8   | 7   | 2
>> 10  | 11  | 8
>> 12  | 15  | 256
>>
>> If this is intentional, please state so and maybe add a Fixes tag.
> 
> Hi Dmitry,
> 
> Yep this was intentional to match downstream and the spec. Will add a 
> fixes tag for this.

Good! I found corresponding change in msm-4.14, so now I understand why 
previously we had what we had.

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>
>>>       data = det_thresh_flatness << 10;
>>>       data |= dsc->flatness_max_qp << 5;
>>>       data |= dsc->flatness_min_qp;
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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_ */
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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_ */
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Jessica Zhang]

On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> Use MSM and DRM DSC helper methods.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 74d38f90398a..7419fe58a941 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -31,6 +31,7 @@
>>   #include "msm_kms.h"
>>   #include "msm_gem.h"
>>   #include "phy/dsi_phy.h"
>> +#include "disp/msm_dsc_helper.h"
>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>> @@ -841,14 +842,14 @@ 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;
>>       /* 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);
> 
> This looks good
> 
>>       /*
>>        * If slice_count is greater than slice_per_intf
>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
> 
> And for these values the result is definitely changed. Separate patch & 
> description please. Just in case, "values per downstream kernel" is not 
> a proper description for such changes.

Hi Dmitry,

Sure, I can put this into a separate patch.

The reason this was changed from slice_count to SLICE_PER_PKT was 
because slice count and slice per packet aren't always equivalent. There 
can be cases where panel configures DSC to have multiple soft slices per 
interface, but the panel only specifies 1 slice per packet.

> 
>>       if (is_cmd_mode) /* packet data type */
>>           reg = 
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>       DBG("");
>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>> +        /* Default widebus_en to false for now. */
>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, 
>> mode->hdisplay,
>> +                dsi_get_bpp(msm_host->format));
>> +
> 
> This is definitely something new and thus should probably go into a 
> separate patch and be described. Also I'm not sure how does that 
> interact with the hdisplay-related calculations below, under the if(dsc) 
> clause.

After double-checking the math here, I think this part of the change is 
actually wrong. pclk_per_line is essentially doing hdisplay / 3, which 
is a repeat of what's being done in the `if (dsc)` block.

Will replace `hdisplay /= 3` with the pclk_per_line calculation.

Thanks,

Jessica Zhang

> 
>>       /*
>>        * For bonded DSI mode, the current DRM mode has
>>        * the complete width of the panel. Since, the complete
>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct 
>> msm_dsi_host *msm_host, struct drm_dsc
>>           return ret;
>>       }
>> -    dsc->initial_scale_value = 32;
>> +    dsc->initial_scale_value = 
>> drm_dsc_calculate_initial_scale_value(dsc);
> 
> This is fine, we only support 8bpp where these values match.
> 
>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>       return drm_dsc_compute_rc_parameters(dsc);
>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jessica Zhang]

On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
> On 30/03/2023 02:18, Jessica Zhang wrote:
>> Use MSM and DRM DSC helper methods.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 74d38f90398a..7419fe58a941 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -31,6 +31,7 @@
>>   #include "msm_kms.h"
>>   #include "msm_gem.h"
>>   #include "phy/dsi_phy.h"
>> +#include "disp/msm_dsc_helper.h"
>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>> @@ -841,14 +842,14 @@ 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;
>>       /* 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);
> 
> This looks good
> 
>>       /*
>>        * If slice_count is greater than slice_per_intf
>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
> 
> And for these values the result is definitely changed. Separate patch & 
> description please. Just in case, "values per downstream kernel" is not 
> a proper description for such changes.

Hi Dmitry,

Sure, I can put this into a separate patch.

The reason this was changed from slice_count to SLICE_PER_PKT was 
because slice count and slice per packet aren't always equivalent. There 
can be cases where panel configures DSC to have multiple soft slices per 
interface, but the panel only specifies 1 slice per packet.

> 
>>       if (is_cmd_mode) /* packet data type */
>>           reg = 
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>       DBG("");
>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>> +        /* Default widebus_en to false for now. */
>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, 
>> mode->hdisplay,
>> +                dsi_get_bpp(msm_host->format));
>> +
> 
> This is definitely something new and thus should probably go into a 
> separate patch and be described. Also I'm not sure how does that 
> interact with the hdisplay-related calculations below, under the if(dsc) 
> clause.

After double-checking the math here, I think this part of the change is 
actually wrong. pclk_per_line is essentially doing hdisplay / 3, which 
is a repeat of what's being done in the `if (dsc)` block.

Will replace `hdisplay /= 3` with the pclk_per_line calculation.

Thanks,

Jessica Zhang

> 
>>       /*
>>        * For bonded DSI mode, the current DRM mode has
>>        * the complete width of the panel. Since, the complete
>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct 
>> msm_dsi_host *msm_host, struct drm_dsc
>>           return ret;
>>       }
>> -    dsc->initial_scale_value = 32;
>> +    dsc->initial_scale_value = 
>> drm_dsc_calculate_initial_scale_value(dsc);
> 
> This is fine, we only support 8bpp where these values match.
> 
>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>       return drm_dsc_compute_rc_parameters(dsc);
>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jessica Zhang]

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
> 

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

[Jessica Zhang]

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
> 

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

[Dmitry Baryshkov]

On 31/03/2023 01:49, Jessica Zhang wrote:
> 
> 
> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>> Use MSM and DRM DSC helper methods.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 74d38f90398a..7419fe58a941 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -31,6 +31,7 @@
>>>   #include "msm_kms.h"
>>>   #include "msm_gem.h"
>>>   #include "phy/dsi_phy.h"
>>> +#include "disp/msm_dsc_helper.h"
>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>>> @@ -841,14 +842,14 @@ 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;
>>>       /* 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);
>>
>> This looks good
>>
>>>       /*
>>>        * If slice_count is greater than slice_per_intf
>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>
>> And for these values the result is definitely changed. Separate patch 
>> & description please. Just in case, "values per downstream kernel" is 
>> not a proper description for such changes.
> 
> Hi Dmitry,
> 
> Sure, I can put this into a separate patch.
> 
> The reason this was changed from slice_count to SLICE_PER_PKT was 
> because slice count and slice per packet aren't always equivalent. There 
> can be cases where panel configures DSC to have multiple soft slices per 
> interface, but the panel only specifies 1 slice per packet.

Please put this nice description into the commit message. It is exactly 
what I was looking for!

BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay 
at "1"? If so, it might be easier to drop it and instead add a comment.

Regarding eol_byte_num, probably the best explanation would be that is 
is a size of a padding rather than a size of a trailer bytes in a line 
(and thus original calculation was incorrect).

> 
>>
>>>       if (is_cmd_mode) /* packet data type */
>>>           reg = 
>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>       DBG("");
>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>> +        /* Default widebus_en to false for now. */
>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, 
>>> mode->hdisplay,
>>> +                dsi_get_bpp(msm_host->format));
>>> +
>>
>> This is definitely something new and thus should probably go into a 
>> separate patch and be described. Also I'm not sure how does that 
>> interact with the hdisplay-related calculations below, under the 
>> if(dsc) clause.
> 
> After double-checking the math here, I think this part of the change is 
> actually wrong. pclk_per_line is essentially doing hdisplay / 3, which 
> is a repeat of what's being done in the `if (dsc)` block.
> 
> Will replace `hdisplay /= 3` with the pclk_per_line calculation.

Thanks!

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>>       /*
>>>        * For bonded DSI mode, the current DRM mode has
>>>        * the complete width of the panel. Since, the complete
>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct 
>>> msm_dsi_host *msm_host, struct drm_dsc
>>>           return ret;
>>>       }
>>> -    dsc->initial_scale_value = 32;
>>> +    dsc->initial_scale_value = 
>>> drm_dsc_calculate_initial_scale_value(dsc);
>>
>> This is fine, we only support 8bpp where these values match.
>>
>>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>       return drm_dsc_compute_rc_parameters(dsc);
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 31/03/2023 01:49, Jessica Zhang wrote:
> 
> 
> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>> Use MSM and DRM DSC helper methods.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 74d38f90398a..7419fe58a941 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -31,6 +31,7 @@
>>>   #include "msm_kms.h"
>>>   #include "msm_gem.h"
>>>   #include "phy/dsi_phy.h"
>>> +#include "disp/msm_dsc_helper.h"
>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>>> @@ -841,14 +842,14 @@ 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;
>>>       /* 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);
>>
>> This looks good
>>
>>>       /*
>>>        * If slice_count is greater than slice_per_intf
>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>
>> And for these values the result is definitely changed. Separate patch 
>> & description please. Just in case, "values per downstream kernel" is 
>> not a proper description for such changes.
> 
> Hi Dmitry,
> 
> Sure, I can put this into a separate patch.
> 
> The reason this was changed from slice_count to SLICE_PER_PKT was 
> because slice count and slice per packet aren't always equivalent. There 
> can be cases where panel configures DSC to have multiple soft slices per 
> interface, but the panel only specifies 1 slice per packet.

Please put this nice description into the commit message. It is exactly 
what I was looking for!

BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay 
at "1"? If so, it might be easier to drop it and instead add a comment.

Regarding eol_byte_num, probably the best explanation would be that is 
is a size of a padding rather than a size of a trailer bytes in a line 
(and thus original calculation was incorrect).

> 
>>
>>>       if (is_cmd_mode) /* packet data type */
>>>           reg = 
>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>       DBG("");
>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>> +        /* Default widebus_en to false for now. */
>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, 
>>> mode->hdisplay,
>>> +                dsi_get_bpp(msm_host->format));
>>> +
>>
>> This is definitely something new and thus should probably go into a 
>> separate patch and be described. Also I'm not sure how does that 
>> interact with the hdisplay-related calculations below, under the 
>> if(dsc) clause.
> 
> After double-checking the math here, I think this part of the change is 
> actually wrong. pclk_per_line is essentially doing hdisplay / 3, which 
> is a repeat of what's being done in the `if (dsc)` block.
> 
> Will replace `hdisplay /= 3` with the pclk_per_line calculation.

Thanks!

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>>       /*
>>>        * For bonded DSI mode, the current DRM mode has
>>>        * the complete width of the panel. Since, the complete
>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct 
>>> msm_dsi_host *msm_host, struct drm_dsc
>>>           return ret;
>>>       }
>>> -    dsc->initial_scale_value = 32;
>>> +    dsc->initial_scale_value = 
>>> drm_dsc_calculate_initial_scale_value(dsc);
>>
>> This is fine, we only support 8bpp where these values match.
>>
>>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>       return drm_dsc_compute_rc_parameters(dsc);
>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

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

[Jessica Zhang]

On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
> On 31/03/2023 01:49, Jessica Zhang wrote:
>>
>>
>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>> Use MSM and DRM DSC helper methods.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 74d38f90398a..7419fe58a941 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -31,6 +31,7 @@
>>>>   #include "msm_kms.h"
>>>>   #include "msm_gem.h"
>>>>   #include "phy/dsi_phy.h"
>>>> +#include "disp/msm_dsc_helper.h"
>>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>> @@ -841,14 +842,14 @@ 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;
>>>>       /* 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);
>>>
>>> This looks good
>>>
>>>>       /*
>>>>        * If slice_count is greater than slice_per_intf
>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>
>>> And for these values the result is definitely changed. Separate patch 
>>> & description please. Just in case, "values per downstream kernel" is 
>>> not a proper description for such changes.
>>
>> Hi Dmitry,
>>
>> Sure, I can put this into a separate patch.
>>
>> The reason this was changed from slice_count to SLICE_PER_PKT was 
>> because slice count and slice per packet aren't always equivalent. 
>> There can be cases where panel configures DSC to have multiple soft 
>> slices per interface, but the panel only specifies 1 slice per packet.
> 
> Please put this nice description into the commit message. It is exactly 
> what I was looking for!
> 
> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay 
> at "1"? If so, it might be easier to drop it and instead add a comment.

MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify 
a slice_per_pkt value. (Now that I think about it, might be better to 
call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)

I don't expect it to change in the future, but it's a little more 
readable than just dividing by 1 IMO. If you prefer dropping the macro 
and adding a comment, I'm also okay with that.

Thanks,

Jessica Zhang

> 
> Regarding eol_byte_num, probably the best explanation would be that is 
> is a size of a padding rather than a size of a trailer bytes in a line 
> (and thus original calculation was incorrect).
> 
>>
>>>
>>>>       if (is_cmd_mode) /* packet data type */
>>>>           reg = 
>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct 
>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>       DBG("");
>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>> +        /* Default widebus_en to false for now. */
>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, 
>>>> mode->hdisplay,
>>>> +                dsi_get_bpp(msm_host->format));
>>>> +
>>>
>>> This is definitely something new and thus should probably go into a 
>>> separate patch and be described. Also I'm not sure how does that 
>>> interact with the hdisplay-related calculations below, under the 
>>> if(dsc) clause.
>>
>> After double-checking the math here, I think this part of the change 
>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3, 
>> which is a repeat of what's being done in the `if (dsc)` block.
>>
>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
> 
> Thanks!
> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>       /*
>>>>        * For bonded DSI mode, the current DRM mode has
>>>>        * the complete width of the panel. Since, the complete
>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct 
>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>           return ret;
>>>>       }
>>>> -    dsc->initial_scale_value = 32;
>>>> +    dsc->initial_scale_value = 
>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>
>>> This is fine, we only support 8bpp where these values match.
>>>
>>>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>       return drm_dsc_compute_rc_parameters(dsc);
>>>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
>>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jessica Zhang]

On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
> On 31/03/2023 01:49, Jessica Zhang wrote:
>>
>>
>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>> Use MSM and DRM DSC helper methods.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 74d38f90398a..7419fe58a941 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -31,6 +31,7 @@
>>>>   #include "msm_kms.h"
>>>>   #include "msm_gem.h"
>>>>   #include "phy/dsi_phy.h"
>>>> +#include "disp/msm_dsc_helper.h"
>>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>> @@ -841,14 +842,14 @@ 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;
>>>>       /* 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);
>>>
>>> This looks good
>>>
>>>>       /*
>>>>        * If slice_count is greater than slice_per_intf
>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>
>>> And for these values the result is definitely changed. Separate patch 
>>> & description please. Just in case, "values per downstream kernel" is 
>>> not a proper description for such changes.
>>
>> Hi Dmitry,
>>
>> Sure, I can put this into a separate patch.
>>
>> The reason this was changed from slice_count to SLICE_PER_PKT was 
>> because slice count and slice per packet aren't always equivalent. 
>> There can be cases where panel configures DSC to have multiple soft 
>> slices per interface, but the panel only specifies 1 slice per packet.
> 
> Please put this nice description into the commit message. It is exactly 
> what I was looking for!
> 
> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay 
> at "1"? If so, it might be easier to drop it and instead add a comment.

MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify 
a slice_per_pkt value. (Now that I think about it, might be better to 
call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)

I don't expect it to change in the future, but it's a little more 
readable than just dividing by 1 IMO. If you prefer dropping the macro 
and adding a comment, I'm also okay with that.

Thanks,

Jessica Zhang

> 
> Regarding eol_byte_num, probably the best explanation would be that is 
> is a size of a padding rather than a size of a trailer bytes in a line 
> (and thus original calculation was incorrect).
> 
>>
>>>
>>>>       if (is_cmd_mode) /* packet data type */
>>>>           reg = 
>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct 
>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>       DBG("");
>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>> +        /* Default widebus_en to false for now. */
>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc, 
>>>> mode->hdisplay,
>>>> +                dsi_get_bpp(msm_host->format));
>>>> +
>>>
>>> This is definitely something new and thus should probably go into a 
>>> separate patch and be described. Also I'm not sure how does that 
>>> interact with the hdisplay-related calculations below, under the 
>>> if(dsc) clause.
>>
>> After double-checking the math here, I think this part of the change 
>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3, 
>> which is a repeat of what's being done in the `if (dsc)` block.
>>
>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
> 
> Thanks!
> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>       /*
>>>>        * For bonded DSI mode, the current DRM mode has
>>>>        * the complete width of the panel. Since, the complete
>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct 
>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>           return ret;
>>>>       }
>>>> -    dsc->initial_scale_value = 32;
>>>> +    dsc->initial_scale_value = 
>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>
>>> This is fine, we only support 8bpp where these values match.
>>>
>>>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>       return drm_dsc_compute_rc_parameters(dsc);
>>>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
>>>
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
> > On 31/03/2023 01:49, Jessica Zhang wrote:
> >>
> >>
> >> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
> >>> On 30/03/2023 02:18, Jessica Zhang wrote:
> >>>> Use MSM and DRM DSC helper methods.
> >>>>
> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>> ---
> >>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
> >>>>   1 file changed, 12 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> index 74d38f90398a..7419fe58a941 100644
> >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> @@ -31,6 +31,7 @@
> >>>>   #include "msm_kms.h"
> >>>>   #include "msm_gem.h"
> >>>>   #include "phy/dsi_phy.h"
> >>>> +#include "disp/msm_dsc_helper.h"
> >>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
> >>>> @@ -841,14 +842,14 @@ 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;
> >>>>       /* 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);
> >>>
> >>> This looks good
> >>>
> >>>>       /*
> >>>>        * If slice_count is greater than slice_per_intf
> >>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
> >>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
> >>>
> >>> And for these values the result is definitely changed. Separate patch
> >>> & description please. Just in case, "values per downstream kernel" is
> >>> not a proper description for such changes.
> >>
> >> Hi Dmitry,
> >>
> >> Sure, I can put this into a separate patch.
> >>
> >> The reason this was changed from slice_count to SLICE_PER_PKT was
> >> because slice count and slice per packet aren't always equivalent.
> >> There can be cases where panel configures DSC to have multiple soft
> >> slices per interface, but the panel only specifies 1 slice per packet.
> >
> > Please put this nice description into the commit message. It is exactly
> > what I was looking for!
> >
> > BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
> > at "1"? If so, it might be easier to drop it and instead add a comment.
>
> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
> a slice_per_pkt value. (Now that I think about it, might be better to
> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)

Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
with another way to pass this data from the panel or to deduce the
value in our driver.

>
> I don't expect it to change in the future, but it's a little more
> readable than just dividing by 1 IMO. If you prefer dropping the macro
> and adding a comment, I'm also okay with that.

There is no need to divide by 1, the value doesn't change. So I'd
probably prefer something like:

/* Default to 1 slice per packet */
if (panel_slice_per_pkt)
    pkt_per_line = slice_per_intf / panel_slice_per_pkt;
else
    pkt_per_line = slice_per_intf;

Or:

/* Default to 1 slice per packet */
slice_per_pkt = 1;
if (panel_slice_per_pkt)
    slice_per_pkt = panel_slice_per_pkt;
pkt_per_line = slice_per_intf / slice_per_pkt;

BTW: could you possibly change 'intf' to 'line' to v2? It seems there
is a mixture of them through the code. If there is a difference
between intf and line which is not yet posted, it's fine to keep the
current code. WDYT?

>
> Thanks,
>
> Jessica Zhang
>
> >
> > Regarding eol_byte_num, probably the best explanation would be that is
> > is a size of a padding rather than a size of a trailer bytes in a line
> > (and thus original calculation was incorrect).
> >
> >>
> >>>
> >>>>       if (is_cmd_mode) /* packet data type */
> >>>>           reg =
> >>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> >>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
> >>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>>>       DBG("");
> >>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> >>>> +        /* Default widebus_en to false for now. */
> >>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
> >>>> mode->hdisplay,
> >>>> +                dsi_get_bpp(msm_host->format));
> >>>> +
> >>>
> >>> This is definitely something new and thus should probably go into a
> >>> separate patch and be described. Also I'm not sure how does that
> >>> interact with the hdisplay-related calculations below, under the
> >>> if(dsc) clause.
> >>
> >> After double-checking the math here, I think this part of the change
> >> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
> >> which is a repeat of what's being done in the `if (dsc)` block.
> >>
> >> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
> >
> > Thanks!
> >
> >>
> >> Thanks,
> >>
> >> Jessica Zhang
> >>
> >>>
> >>>>       /*
> >>>>        * For bonded DSI mode, the current DRM mode has
> >>>>        * the complete width of the panel. Since, the complete
> >>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
> >>>> msm_dsi_host *msm_host, struct drm_dsc
> >>>>           return ret;
> >>>>       }
> >>>> -    dsc->initial_scale_value = 32;
> >>>> +    dsc->initial_scale_value =
> >>>> drm_dsc_calculate_initial_scale_value(dsc);
> >>>
> >>> This is fine, we only support 8bpp where these values match.
> >>>
> >>>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
> >>>>       return drm_dsc_compute_rc_parameters(dsc);
> >>>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry
> >>>
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
> > On 31/03/2023 01:49, Jessica Zhang wrote:
> >>
> >>
> >> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
> >>> On 30/03/2023 02:18, Jessica Zhang wrote:
> >>>> Use MSM and DRM DSC helper methods.
> >>>>
> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>> ---
> >>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
> >>>>   1 file changed, 12 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> index 74d38f90398a..7419fe58a941 100644
> >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >>>> @@ -31,6 +31,7 @@
> >>>>   #include "msm_kms.h"
> >>>>   #include "msm_gem.h"
> >>>>   #include "phy/dsi_phy.h"
> >>>> +#include "disp/msm_dsc_helper.h"
> >>>>   #define DSI_RESET_TOGGLE_DELAY_MS 20
> >>>> @@ -841,14 +842,14 @@ 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;
> >>>>       /* 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);
> >>>
> >>> This looks good
> >>>
> >>>>       /*
> >>>>        * If slice_count is greater than slice_per_intf
> >>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
> >>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
> >>>
> >>> And for these values the result is definitely changed. Separate patch
> >>> & description please. Just in case, "values per downstream kernel" is
> >>> not a proper description for such changes.
> >>
> >> Hi Dmitry,
> >>
> >> Sure, I can put this into a separate patch.
> >>
> >> The reason this was changed from slice_count to SLICE_PER_PKT was
> >> because slice count and slice per packet aren't always equivalent.
> >> There can be cases where panel configures DSC to have multiple soft
> >> slices per interface, but the panel only specifies 1 slice per packet.
> >
> > Please put this nice description into the commit message. It is exactly
> > what I was looking for!
> >
> > BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
> > at "1"? If so, it might be easier to drop it and instead add a comment.
>
> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
> a slice_per_pkt value. (Now that I think about it, might be better to
> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)

Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
with another way to pass this data from the panel or to deduce the
value in our driver.

>
> I don't expect it to change in the future, but it's a little more
> readable than just dividing by 1 IMO. If you prefer dropping the macro
> and adding a comment, I'm also okay with that.

There is no need to divide by 1, the value doesn't change. So I'd
probably prefer something like:

/* Default to 1 slice per packet */
if (panel_slice_per_pkt)
    pkt_per_line = slice_per_intf / panel_slice_per_pkt;
else
    pkt_per_line = slice_per_intf;

Or:

/* Default to 1 slice per packet */
slice_per_pkt = 1;
if (panel_slice_per_pkt)
    slice_per_pkt = panel_slice_per_pkt;
pkt_per_line = slice_per_intf / slice_per_pkt;

BTW: could you possibly change 'intf' to 'line' to v2? It seems there
is a mixture of them through the code. If there is a difference
between intf and line which is not yet posted, it's fine to keep the
current code. WDYT?

>
> Thanks,
>
> Jessica Zhang
>
> >
> > Regarding eol_byte_num, probably the best explanation would be that is
> > is a size of a padding rather than a size of a trailer bytes in a line
> > (and thus original calculation was incorrect).
> >
> >>
> >>>
> >>>>       if (is_cmd_mode) /* packet data type */
> >>>>           reg =
> >>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> >>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
> >>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>>>       DBG("");
> >>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
> >>>> +        /* Default widebus_en to false for now. */
> >>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
> >>>> mode->hdisplay,
> >>>> +                dsi_get_bpp(msm_host->format));
> >>>> +
> >>>
> >>> This is definitely something new and thus should probably go into a
> >>> separate patch and be described. Also I'm not sure how does that
> >>> interact with the hdisplay-related calculations below, under the
> >>> if(dsc) clause.
> >>
> >> After double-checking the math here, I think this part of the change
> >> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
> >> which is a repeat of what's being done in the `if (dsc)` block.
> >>
> >> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
> >
> > Thanks!
> >
> >>
> >> Thanks,
> >>
> >> Jessica Zhang
> >>
> >>>
> >>>>       /*
> >>>>        * For bonded DSI mode, the current DRM mode has
> >>>>        * the complete width of the panel. Since, the complete
> >>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
> >>>> msm_dsi_host *msm_host, struct drm_dsc
> >>>>           return ret;
> >>>>       }
> >>>> -    dsc->initial_scale_value = 32;
> >>>> +    dsc->initial_scale_value =
> >>>> drm_dsc_calculate_initial_scale_value(dsc);
> >>>
> >>> This is fine, we only support 8bpp where these values match.
> >>>
> >>>>       dsc->line_buf_depth = dsc->bits_per_component + 1;
> >>>>       return drm_dsc_compute_rc_parameters(dsc);
> >>>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry
> >>>
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry

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

[Jessica Zhang]

On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -31,6 +31,7 @@
>>>>>>    #include "msm_kms.h"
>>>>>>    #include "msm_gem.h"
>>>>>>    #include "phy/dsi_phy.h"
>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>        /* 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);
>>>>>
>>>>> This looks good
>>>>>
>>>>>>        /*
>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>
>>>>> And for these values the result is definitely changed. Separate patch
>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>> not a proper description for such changes.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Sure, I can put this into a separate patch.
>>>>
>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>> because slice count and slice per packet aren't always equivalent.
>>>> There can be cases where panel configures DSC to have multiple soft
>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>
>>> Please put this nice description into the commit message. It is exactly
>>> what I was looking for!
>>>
>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>
>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>> a slice_per_pkt value. (Now that I think about it, might be better to
>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
> 
> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
> with another way to pass this data from the panel or to deduce the
> value in our driver.

AFAIK we aren't developing on any panels that have a non-default 
slice-per-packet count right now, so I'm not sure if it would be worth 
the effort to add this if there's no panel we can validate it on.

FWIW, downstream reads slice_per_pkt from a custom DT entry [1]

[1] 
https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115

> 
>>
>> I don't expect it to change in the future, but it's a little more
>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>> and adding a comment, I'm also okay with that.
> 
> There is no need to divide by 1, the value doesn't change. So I'd
> probably prefer something like:
> 
> /* Default to 1 slice per packet */
> if (panel_slice_per_pkt)
>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
> else
>      pkt_per_line = slice_per_intf;
> 
> Or:
> 
> /* Default to 1 slice per packet */
> slice_per_pkt = 1;
> if (panel_slice_per_pkt)
>      slice_per_pkt = panel_slice_per_pkt;
> pkt_per_line = slice_per_intf / slice_per_pkt;

Acked.

> 
> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
> is a mixture of them through the code. If there is a difference
> between intf and line which is not yet posted, it's fine to keep the
> current code. WDYT?

AFAIK 'line' and 'intf' seem to be equivalent. I'll make this change 
once I confirm this.

Thanks,

Jessica Zhang

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> Regarding eol_byte_num, probably the best explanation would be that is
>>> is a size of a padding rather than a size of a trailer bytes in a line
>>> (and thus original calculation was incorrect).
>>>
>>>>
>>>>>
>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>            reg =
>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>        DBG("");
>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>> +        /* Default widebus_en to false for now. */
>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>> mode->hdisplay,
>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>> +
>>>>>
>>>>> This is definitely something new and thus should probably go into a
>>>>> separate patch and be described. Also I'm not sure how does that
>>>>> interact with the hdisplay-related calculations below, under the
>>>>> if(dsc) clause.
>>>>
>>>> After double-checking the math here, I think this part of the change
>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>
>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>
>>> Thanks!
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>>>        /*
>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>         * the complete width of the panel. Since, the complete
>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>            return ret;
>>>>>>        }
>>>>>> -    dsc->initial_scale_value = 32;
>>>>>> +    dsc->initial_scale_value =
>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>
>>>>> This is fine, we only support 8bpp where these values match.
>>>>>
>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
> 
> 
> 
> -- 
> With best wishes
> Dmitry

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

[Jessica Zhang]

On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -31,6 +31,7 @@
>>>>>>    #include "msm_kms.h"
>>>>>>    #include "msm_gem.h"
>>>>>>    #include "phy/dsi_phy.h"
>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>        /* 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);
>>>>>
>>>>> This looks good
>>>>>
>>>>>>        /*
>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>
>>>>> And for these values the result is definitely changed. Separate patch
>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>> not a proper description for such changes.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Sure, I can put this into a separate patch.
>>>>
>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>> because slice count and slice per packet aren't always equivalent.
>>>> There can be cases where panel configures DSC to have multiple soft
>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>
>>> Please put this nice description into the commit message. It is exactly
>>> what I was looking for!
>>>
>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>
>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>> a slice_per_pkt value. (Now that I think about it, might be better to
>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
> 
> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
> with another way to pass this data from the panel or to deduce the
> value in our driver.

AFAIK we aren't developing on any panels that have a non-default 
slice-per-packet count right now, so I'm not sure if it would be worth 
the effort to add this if there's no panel we can validate it on.

FWIW, downstream reads slice_per_pkt from a custom DT entry [1]

[1] 
https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115

> 
>>
>> I don't expect it to change in the future, but it's a little more
>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>> and adding a comment, I'm also okay with that.
> 
> There is no need to divide by 1, the value doesn't change. So I'd
> probably prefer something like:
> 
> /* Default to 1 slice per packet */
> if (panel_slice_per_pkt)
>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
> else
>      pkt_per_line = slice_per_intf;
> 
> Or:
> 
> /* Default to 1 slice per packet */
> slice_per_pkt = 1;
> if (panel_slice_per_pkt)
>      slice_per_pkt = panel_slice_per_pkt;
> pkt_per_line = slice_per_intf / slice_per_pkt;

Acked.

> 
> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
> is a mixture of them through the code. If there is a difference
> between intf and line which is not yet posted, it's fine to keep the
> current code. WDYT?

AFAIK 'line' and 'intf' seem to be equivalent. I'll make this change 
once I confirm this.

Thanks,

Jessica Zhang

> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> Regarding eol_byte_num, probably the best explanation would be that is
>>> is a size of a padding rather than a size of a trailer bytes in a line
>>> (and thus original calculation was incorrect).
>>>
>>>>
>>>>>
>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>            reg =
>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>        DBG("");
>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>> +        /* Default widebus_en to false for now. */
>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>> mode->hdisplay,
>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>> +
>>>>>
>>>>> This is definitely something new and thus should probably go into a
>>>>> separate patch and be described. Also I'm not sure how does that
>>>>> interact with the hdisplay-related calculations below, under the
>>>>> if(dsc) clause.
>>>>
>>>> After double-checking the math here, I think this part of the change
>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>
>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>
>>> Thanks!
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>>>        /*
>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>         * the complete width of the panel. Since, the complete
>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>            return ret;
>>>>>>        }
>>>>>> -    dsc->initial_scale_value = 32;
>>>>>> +    dsc->initial_scale_value =
>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>
>>>>> This is fine, we only support 8bpp where these values match.
>>>>>
>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
> 
> 
> 
> -- 
> With best wishes
> Dmitry

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

[Konrad Dybcio]

On 31.03.2023 03:12, Jessica Zhang wrote:
> 
> 
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>    #include "msm_kms.h"
>>>>>>>    #include "msm_gem.h"
>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>        /* 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);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>>        /*
>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
> 
> AFAIK we aren't developing on any panels that have a non-default slice-per-packet count right now, so I'm not sure if it would be worth the effort to add this if there's no panel we can validate it on.
Some (but not all) SONY phones use panels with that set to 2, I'd
greatly appreciate fully implementing this.

Konrad
> 
> FWIW, downstream reads slice_per_pkt from a custom DT entry [1]
> 
> [1] https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115
> 
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>>      pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>>      slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
> 
> Acked.
> 
>>
>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>> is a mixture of them through the code. If there is a difference
>> between intf and line which is not yet posted, it's fine to keep the
>> current code. WDYT?
> 
> AFAIK 'line' and 'intf' seem to be equivalent. I'll make this change once I confirm this.
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>> (and thus original calculation was incorrect).
>>>>
>>>>>
>>>>>>
>>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>>            reg =
>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>        DBG("");
>>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>> +        /* Default widebus_en to false for now. */
>>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>> mode->hdisplay,
>>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>>> +
>>>>>>
>>>>>> This is definitely something new and thus should probably go into a
>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>> if(dsc) clause.
>>>>>
>>>>> After double-checking the math here, I think this part of the change
>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>
>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>>>
>>>>>>>        /*
>>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>>         * the complete width of the panel. Since, the complete
>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>>            return ret;
>>>>>>>        }
>>>>>>> -    dsc->initial_scale_value = 32;
>>>>>>> +    dsc->initial_scale_value =
>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>
>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>
>>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> With best wishes
>>>>>> Dmitry
>>>>>>
>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
>>>>
>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry

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

[Konrad Dybcio]

On 31.03.2023 03:12, Jessica Zhang wrote:
> 
> 
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>    #include "msm_kms.h"
>>>>>>>    #include "msm_gem.h"
>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>        /* 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);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>>        /*
>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
> 
> AFAIK we aren't developing on any panels that have a non-default slice-per-packet count right now, so I'm not sure if it would be worth the effort to add this if there's no panel we can validate it on.
Some (but not all) SONY phones use panels with that set to 2, I'd
greatly appreciate fully implementing this.

Konrad
> 
> FWIW, downstream reads slice_per_pkt from a custom DT entry [1]
> 
> [1] https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115
> 
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>>      pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>>      slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
> 
> Acked.
> 
>>
>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>> is a mixture of them through the code. If there is a difference
>> between intf and line which is not yet posted, it's fine to keep the
>> current code. WDYT?
> 
> AFAIK 'line' and 'intf' seem to be equivalent. I'll make this change once I confirm this.
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>> (and thus original calculation was incorrect).
>>>>
>>>>>
>>>>>>
>>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>>            reg =
>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>        DBG("");
>>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>> +        /* Default widebus_en to false for now. */
>>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>> mode->hdisplay,
>>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>>> +
>>>>>>
>>>>>> This is definitely something new and thus should probably go into a
>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>> if(dsc) clause.
>>>>>
>>>>> After double-checking the math here, I think this part of the change
>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>
>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>>>
>>>>>>>        /*
>>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>>         * the complete width of the panel. Since, the complete
>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>>            return ret;
>>>>>>>        }
>>>>>>> -    dsc->initial_scale_value = 32;
>>>>>>> +    dsc->initial_scale_value =
>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>
>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>
>>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> With best wishes
>>>>>> Dmitry
>>>>>>
>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
>>>>
>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry

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

[Dmitry Baryshkov]

On 31/03/2023 04:12, Jessica Zhang wrote:
> 
> 
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>    #include "msm_kms.h"
>>>>>>>    #include "msm_gem.h"
>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>        /* 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);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>>        /*
>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>> stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
> 
> AFAIK we aren't developing on any panels that have a non-default 
> slice-per-packet count right now, so I'm not sure if it would be worth 
> the effort to add this if there's no panel we can validate it on.
> 
> FWIW, downstream reads slice_per_pkt from a custom DT entry [1]

As you might guess, this approach is frowned upon in upstream kernel. 
This info should come from panel (e.g. extend drm_dsc_config or deduce 
the value in msm driver).

> 
> [1] 
> https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115
> 
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>>      pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>>      slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
> 
> Acked.
> 


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 31/03/2023 04:12, Jessica Zhang wrote:
> 
> 
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>    #include "msm_kms.h"
>>>>>>>    #include "msm_gem.h"
>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>        /* 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);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>>        /*
>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>> stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
> 
> AFAIK we aren't developing on any panels that have a non-default 
> slice-per-packet count right now, so I'm not sure if it would be worth 
> the effort to add this if there's no panel we can validate it on.
> 
> FWIW, downstream reads slice_per_pkt from a custom DT entry [1]

As you might guess, this approach is frowned upon in upstream kernel. 
This info should come from panel (e.g. extend drm_dsc_config or deduce 
the value in msm driver).

> 
> [1] 
> https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115
> 
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>>      pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>>      slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
> 
> Acked.
> 


-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 3/30/2023 6:25 PM, Dmitry Baryshkov wrote:
> On 31/03/2023 04:12, Jessica Zhang wrote:
>>
>>
>> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>>> <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>>    #include "msm_kms.h"
>>>>>>>>    #include "msm_gem.h"
>>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>>        /* 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);
>>>>>>>
>>>>>>> This looks good
>>>>>>>
>>>>>>>>        /*
>>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>>
>>>>>>> And for these values the result is definitely changed. Separate 
>>>>>>> patch
>>>>>>> & description please. Just in case, "values per downstream 
>>>>>>> kernel" is
>>>>>>> not a proper description for such changes.
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Sure, I can put this into a separate patch.
>>>>>>
>>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>>> because slice count and slice per packet aren't always equivalent.
>>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>>> slices per interface, but the panel only specifies 1 slice per 
>>>>>> packet.
>>>>>
>>>>> Please put this nice description into the commit message. It is 
>>>>> exactly
>>>>> what I was looking for!
>>>>>
>>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>>> stay
>>>>> at "1"? If so, it might be easier to drop it and instead add a 
>>>>> comment.
>>>>
>>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't 
>>>> specify
>>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>>
>>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>>> with another way to pass this data from the panel or to deduce the
>>> value in our driver.
>>
>> AFAIK we aren't developing on any panels that have a non-default 
>> slice-per-packet count right now, so I'm not sure if it would be worth 
>> the effort to add this if there's no panel we can validate it on.
>>
>> FWIW, downstream reads slice_per_pkt from a custom DT entry [1]
> 
> As you might guess, this approach is frowned upon in upstream kernel. 
> This info should come from panel (e.g. extend drm_dsc_config or deduce 
> the value in msm driver).
> 

Yes, I am already aware slice_per_pkt should come from the panel 
otherwise defaults to 1. Today MSM driver or even drm_panels for that 
matter dont support passing this.

We can extend adding it to the drm_panel and coming from there in 
another series. Let this series first add the support for DSC 1.2 for 
existing DSC 1.1 configurations. Even DSC 1.1 doesnt use custom 
slice_per_pkt today. So nothing broken.

>>
>> [1] 
>> https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115 
>>
>>
>>>
>>>>
>>>> I don't expect it to change in the future, but it's a little more
>>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>>> and adding a comment, I'm also okay with that.
>>>
>>> There is no need to divide by 1, the value doesn't change. So I'd
>>> probably prefer something like:
>>>
>>> /* Default to 1 slice per packet */
>>> if (panel_slice_per_pkt)
>>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>>> else
>>>      pkt_per_line = slice_per_intf;
>>>
>>> Or:
>>>
>>> /* Default to 1 slice per packet */
>>> slice_per_pkt = 1;
>>> if (panel_slice_per_pkt)
>>>      slice_per_pkt = panel_slice_per_pkt;
>>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>
>> Acked.
>>
> 
> 

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

[Abhinav Kumar]

On 3/30/2023 6:25 PM, Dmitry Baryshkov wrote:
> On 31/03/2023 04:12, Jessica Zhang wrote:
>>
>>
>> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>>> <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>>    #include "msm_kms.h"
>>>>>>>>    #include "msm_gem.h"
>>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>>        /* 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);
>>>>>>>
>>>>>>> This looks good
>>>>>>>
>>>>>>>>        /*
>>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>>
>>>>>>> And for these values the result is definitely changed. Separate 
>>>>>>> patch
>>>>>>> & description please. Just in case, "values per downstream 
>>>>>>> kernel" is
>>>>>>> not a proper description for such changes.
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Sure, I can put this into a separate patch.
>>>>>>
>>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>>> because slice count and slice per packet aren't always equivalent.
>>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>>> slices per interface, but the panel only specifies 1 slice per 
>>>>>> packet.
>>>>>
>>>>> Please put this nice description into the commit message. It is 
>>>>> exactly
>>>>> what I was looking for!
>>>>>
>>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>>> stay
>>>>> at "1"? If so, it might be easier to drop it and instead add a 
>>>>> comment.
>>>>
>>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't 
>>>> specify
>>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>>
>>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>>> with another way to pass this data from the panel or to deduce the
>>> value in our driver.
>>
>> AFAIK we aren't developing on any panels that have a non-default 
>> slice-per-packet count right now, so I'm not sure if it would be worth 
>> the effort to add this if there's no panel we can validate it on.
>>
>> FWIW, downstream reads slice_per_pkt from a custom DT entry [1]
> 
> As you might guess, this approach is frowned upon in upstream kernel. 
> This info should come from panel (e.g. extend drm_dsc_config or deduce 
> the value in msm driver).
> 

Yes, I am already aware slice_per_pkt should come from the panel 
otherwise defaults to 1. Today MSM driver or even drm_panels for that 
matter dont support passing this.

We can extend adding it to the drm_panel and coming from there in 
another series. Let this series first add the support for DSC 1.2 for 
existing DSC 1.1 configurations. Even DSC 1.1 doesnt use custom 
slice_per_pkt today. So nothing broken.

>>
>> [1] 
>> https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/dsi-panel-r66451-dsc-fhd-plus-120hz-cmd.dtsi#115 
>>
>>
>>>
>>>>
>>>> I don't expect it to change in the future, but it's a little more
>>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>>> and adding a comment, I'm also okay with that.
>>>
>>> There is no need to divide by 1, the value doesn't change. So I'd
>>> probably prefer something like:
>>>
>>> /* Default to 1 slice per packet */
>>> if (panel_slice_per_pkt)
>>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>>> else
>>>      pkt_per_line = slice_per_intf;
>>>
>>> Or:
>>>
>>> /* Default to 1 slice per packet */
>>> slice_per_pkt = 1;
>>> if (panel_slice_per_pkt)
>>>      slice_per_pkt = panel_slice_per_pkt;
>>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>
>> Acked.
>>
> 
> 

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

[Abhinav Kumar]

On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -31,6 +31,7 @@
>>>>>>    #include "msm_kms.h"
>>>>>>    #include "msm_gem.h"
>>>>>>    #include "phy/dsi_phy.h"
>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>        /* 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);
>>>>>
>>>>> This looks good
>>>>>
>>>>>>        /*
>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>
>>>>> And for these values the result is definitely changed. Separate patch
>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>> not a proper description for such changes.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Sure, I can put this into a separate patch.
>>>>
>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>> because slice count and slice per packet aren't always equivalent.
>>>> There can be cases where panel configures DSC to have multiple soft
>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>
>>> Please put this nice description into the commit message. It is exactly
>>> what I was looking for!
>>>
>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>
>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>> a slice_per_pkt value. (Now that I think about it, might be better to
>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
> 
> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
> with another way to pass this data from the panel or to deduce the
> value in our driver.
> 
>>
>> I don't expect it to change in the future, but it's a little more
>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>> and adding a comment, I'm also okay with that.
> 
> There is no need to divide by 1, the value doesn't change. So I'd
> probably prefer something like:
> 
> /* Default to 1 slice per packet */
> if (panel_slice_per_pkt)
>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
> else
>      pkt_per_line = slice_per_intf;
> 
> Or:
> 
> /* Default to 1 slice per packet */
> slice_per_pkt = 1;
> if (panel_slice_per_pkt)
>      slice_per_pkt = panel_slice_per_pkt;
> pkt_per_line = slice_per_intf / slice_per_pkt;
> 
> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
> is a mixture of them through the code. If there is a difference
> between intf and line which is not yet posted, it's fine to keep the
> current code. WDYT?
> 

No, I dont agree with the change from intf to line.

In case of dual DSI, intf is not equal to line.

2 intfs = 1 line

Hence that distinction is necessary.

>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> Regarding eol_byte_num, probably the best explanation would be that is
>>> is a size of a padding rather than a size of a trailer bytes in a line
>>> (and thus original calculation was incorrect).
>>>
>>>>
>>>>>
>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>            reg =
>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>        DBG("");
>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>> +        /* Default widebus_en to false for now. */
>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>> mode->hdisplay,
>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>> +
>>>>>
>>>>> This is definitely something new and thus should probably go into a
>>>>> separate patch and be described. Also I'm not sure how does that
>>>>> interact with the hdisplay-related calculations below, under the
>>>>> if(dsc) clause.
>>>>
>>>> After double-checking the math here, I think this part of the change
>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>
>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>
>>> Thanks!
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>>>        /*
>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>         * the complete width of the panel. Since, the complete
>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>            return ret;
>>>>>>        }
>>>>>> -    dsc->initial_scale_value = 32;
>>>>>> +    dsc->initial_scale_value =
>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>
>>>>> This is fine, we only support 8bpp where these values match.
>>>>>
>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
> 
> 
> 

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

[Abhinav Kumar]

On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -31,6 +31,7 @@
>>>>>>    #include "msm_kms.h"
>>>>>>    #include "msm_gem.h"
>>>>>>    #include "phy/dsi_phy.h"
>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>        /* 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);
>>>>>
>>>>> This looks good
>>>>>
>>>>>>        /*
>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>
>>>>> And for these values the result is definitely changed. Separate patch
>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>> not a proper description for such changes.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Sure, I can put this into a separate patch.
>>>>
>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>> because slice count and slice per packet aren't always equivalent.
>>>> There can be cases where panel configures DSC to have multiple soft
>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>
>>> Please put this nice description into the commit message. It is exactly
>>> what I was looking for!
>>>
>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will stay
>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>
>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>> a slice_per_pkt value. (Now that I think about it, might be better to
>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
> 
> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
> with another way to pass this data from the panel or to deduce the
> value in our driver.
> 
>>
>> I don't expect it to change in the future, but it's a little more
>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>> and adding a comment, I'm also okay with that.
> 
> There is no need to divide by 1, the value doesn't change. So I'd
> probably prefer something like:
> 
> /* Default to 1 slice per packet */
> if (panel_slice_per_pkt)
>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
> else
>      pkt_per_line = slice_per_intf;
> 
> Or:
> 
> /* Default to 1 slice per packet */
> slice_per_pkt = 1;
> if (panel_slice_per_pkt)
>      slice_per_pkt = panel_slice_per_pkt;
> pkt_per_line = slice_per_intf / slice_per_pkt;
> 
> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
> is a mixture of them through the code. If there is a difference
> between intf and line which is not yet posted, it's fine to keep the
> current code. WDYT?
> 

No, I dont agree with the change from intf to line.

In case of dual DSI, intf is not equal to line.

2 intfs = 1 line

Hence that distinction is necessary.

>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> Regarding eol_byte_num, probably the best explanation would be that is
>>> is a size of a padding rather than a size of a trailer bytes in a line
>>> (and thus original calculation was incorrect).
>>>
>>>>
>>>>>
>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>            reg =
>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>        DBG("");
>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>> +        /* Default widebus_en to false for now. */
>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>> mode->hdisplay,
>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>> +
>>>>>
>>>>> This is definitely something new and thus should probably go into a
>>>>> separate patch and be described. Also I'm not sure how does that
>>>>> interact with the hdisplay-related calculations below, under the
>>>>> if(dsc) clause.
>>>>
>>>> After double-checking the math here, I think this part of the change
>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>
>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>
>>> Thanks!
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>>>        /*
>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>         * the complete width of the panel. Since, the complete
>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>            return ret;
>>>>>>        }
>>>>>> -    dsc->initial_scale_value = 32;
>>>>>> +    dsc->initial_scale_value =
>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>
>>>>> This is fine, we only support 8bpp where these values match.
>>>>>
>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
> 
> 
> 

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

[Dmitry Baryshkov]

On 31/03/2023 04:33, Abhinav Kumar wrote:
> 
> 
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>    #include "msm_kms.h"
>>>>>>>    #include "msm_gem.h"
>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>        /* 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);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>>        /*
>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>> stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>>      pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>>      slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>
>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>> is a mixture of them through the code. If there is a difference
>> between intf and line which is not yet posted, it's fine to keep the
>> current code. WDYT?
>>
> 
> No, I dont agree with the change from intf to line.
> 
> In case of dual DSI, intf is not equal to line.
> 
> 2 intfs = 1 line
> 
> Hence that distinction is necessary.

Ack, this is what I was looking for!

so intf = line / num_intf?

Maybe I should explain the reason for my question:

msm_dsc_get_pclk_per_line() uses intf_width, calculates pclk_per_line 
(not per intf). msm_dsc_get_dce_bytes_per_line() does the same thing

In this patch we take slice_per_intf, divide it with slice_per_pkt and 
get pkt_per_line (rather than pkt_per_intf).

This is what prompted my question regarding intf vs line.

> 
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>> (and thus original calculation was incorrect).
>>>>
>>>>>
>>>>>>
>>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>>            reg =
>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>        DBG("");
>>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>> +        /* Default widebus_en to false for now. */
>>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>> mode->hdisplay,
>>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>>> +
>>>>>>
>>>>>> This is definitely something new and thus should probably go into a
>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>> if(dsc) clause.
>>>>>
>>>>> After double-checking the math here, I think this part of the change
>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>
>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>>>
>>>>>>>        /*
>>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>>         * the complete width of the panel. Since, the complete
>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>>            return ret;
>>>>>>>        }
>>>>>>> -    dsc->initial_scale_value = 32;
>>>>>>> +    dsc->initial_scale_value =
>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>
>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>
>>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> With best wishes
>>>>>> Dmitry
>>>>>>
>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
>>>>
>>
>>
>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 31/03/2023 04:33, Abhinav Kumar wrote:
> 
> 
> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>
>>>>>
>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>    #include "msm_kms.h"
>>>>>>>    #include "msm_gem.h"
>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>        /* 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);
>>>>>>
>>>>>> This looks good
>>>>>>
>>>>>>>        /*
>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>
>>>>>> And for these values the result is definitely changed. Separate patch
>>>>>> & description please. Just in case, "values per downstream kernel" is
>>>>>> not a proper description for such changes.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Sure, I can put this into a separate patch.
>>>>>
>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>> because slice count and slice per packet aren't always equivalent.
>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>> slices per interface, but the panel only specifies 1 slice per packet.
>>>>
>>>> Please put this nice description into the commit message. It is exactly
>>>> what I was looking for!
>>>>
>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>> stay
>>>> at "1"? If so, it might be easier to drop it and instead add a comment.
>>>
>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't specify
>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>
>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>> with another way to pass this data from the panel or to deduce the
>> value in our driver.
>>
>>>
>>> I don't expect it to change in the future, but it's a little more
>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>> and adding a comment, I'm also okay with that.
>>
>> There is no need to divide by 1, the value doesn't change. So I'd
>> probably prefer something like:
>>
>> /* Default to 1 slice per packet */
>> if (panel_slice_per_pkt)
>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>> else
>>      pkt_per_line = slice_per_intf;
>>
>> Or:
>>
>> /* Default to 1 slice per packet */
>> slice_per_pkt = 1;
>> if (panel_slice_per_pkt)
>>      slice_per_pkt = panel_slice_per_pkt;
>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>
>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>> is a mixture of them through the code. If there is a difference
>> between intf and line which is not yet posted, it's fine to keep the
>> current code. WDYT?
>>
> 
> No, I dont agree with the change from intf to line.
> 
> In case of dual DSI, intf is not equal to line.
> 
> 2 intfs = 1 line
> 
> Hence that distinction is necessary.

Ack, this is what I was looking for!

so intf = line / num_intf?

Maybe I should explain the reason for my question:

msm_dsc_get_pclk_per_line() uses intf_width, calculates pclk_per_line 
(not per intf). msm_dsc_get_dce_bytes_per_line() does the same thing

In this patch we take slice_per_intf, divide it with slice_per_pkt and 
get pkt_per_line (rather than pkt_per_intf).

This is what prompted my question regarding intf vs line.

> 
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>> (and thus original calculation was incorrect).
>>>>
>>>>>
>>>>>>
>>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>>            reg =
>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>        DBG("");
>>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>> +        /* Default widebus_en to false for now. */
>>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>> mode->hdisplay,
>>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>>> +
>>>>>>
>>>>>> This is definitely something new and thus should probably go into a
>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>> if(dsc) clause.
>>>>>
>>>>> After double-checking the math here, I think this part of the change
>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>
>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>>>
>>>>>>>        /*
>>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>>         * the complete width of the panel. Since, the complete
>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>>            return ret;
>>>>>>>        }
>>>>>>> -    dsc->initial_scale_value = 32;
>>>>>>> +    dsc->initial_scale_value =
>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>
>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>
>>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> With best wishes
>>>>>> Dmitry
>>>>>>
>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
>>>>
>>
>>
>>

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 3/30/2023 7:47 PM, Dmitry Baryshkov wrote:
> On 31/03/2023 04:33, Abhinav Kumar wrote:
>>
>>
>> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>>> <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>>    #include "msm_kms.h"
>>>>>>>>    #include "msm_gem.h"
>>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>>        /* 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);
>>>>>>>
>>>>>>> This looks good
>>>>>>>
>>>>>>>>        /*
>>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>>
>>>>>>> And for these values the result is definitely changed. Separate 
>>>>>>> patch
>>>>>>> & description please. Just in case, "values per downstream 
>>>>>>> kernel" is
>>>>>>> not a proper description for such changes.
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Sure, I can put this into a separate patch.
>>>>>>
>>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>>> because slice count and slice per packet aren't always equivalent.
>>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>>> slices per interface, but the panel only specifies 1 slice per 
>>>>>> packet.
>>>>>
>>>>> Please put this nice description into the commit message. It is 
>>>>> exactly
>>>>> what I was looking for!
>>>>>
>>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>>> stay
>>>>> at "1"? If so, it might be easier to drop it and instead add a 
>>>>> comment.
>>>>
>>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't 
>>>> specify
>>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>>
>>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>>> with another way to pass this data from the panel or to deduce the
>>> value in our driver.
>>>
>>>>
>>>> I don't expect it to change in the future, but it's a little more
>>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>>> and adding a comment, I'm also okay with that.
>>>
>>> There is no need to divide by 1, the value doesn't change. So I'd
>>> probably prefer something like:
>>>
>>> /* Default to 1 slice per packet */
>>> if (panel_slice_per_pkt)
>>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>>> else
>>>      pkt_per_line = slice_per_intf;
>>>
>>> Or:
>>>
>>> /* Default to 1 slice per packet */
>>> slice_per_pkt = 1;
>>> if (panel_slice_per_pkt)
>>>      slice_per_pkt = panel_slice_per_pkt;
>>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>>
>>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>>> is a mixture of them through the code. If there is a difference
>>> between intf and line which is not yet posted, it's fine to keep the
>>> current code. WDYT?
>>>
>>
>> No, I dont agree with the change from intf to line.
>>
>> In case of dual DSI, intf is not equal to line.
>>
>> 2 intfs = 1 line
>>
>> Hence that distinction is necessary.
> 
> Ack, this is what I was looking for!
> 
> so intf = line / num_intf?
> 

Yes by definition, "line" is one horizontal line of pixels for the panel.

So intf = h_active of panel / num_intf

But here "line" is one line of pixels pulled by the interface.

So for dual dsi cases its = h_active of panel / 2

> Maybe I should explain the reason for my question:
> 
> msm_dsc_get_pclk_per_line() uses intf_width, calculates pclk_per_line 
> (not per intf). msm_dsc_get_dce_bytes_per_line() does the same thing
> 
> In this patch we take slice_per_intf, divide it with slice_per_pkt and 
> get pkt_per_line (rather than pkt_per_intf).
> 
> This is what prompted my question regarding intf vs line.
> 

Valid question. The terminology gets a bit confusing because.

pclk_per_line can be only per interface.

Thats because each interface can pull the pixels at different pclks.

If it helps, I would say, this is pclk_per_line for each interface.

OR in other words pclks needed to pull one line of pixels for each 
interface.

But if i changed it to slice_per_line that would be wrong because then 
line becomes the full panel horizontal line.

>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>>> (and thus original calculation was incorrect).
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>>>            reg =
>>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE); 
>>>>>>>>
>>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>>        DBG("");
>>>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>>> +        /* Default widebus_en to false for now. */
>>>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>>> mode->hdisplay,
>>>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>>>> +
>>>>>>>
>>>>>>> This is definitely something new and thus should probably go into a
>>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>>> if(dsc) clause.
>>>>>>
>>>>>> After double-checking the math here, I think this part of the change
>>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>>
>>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Jessica Zhang
>>>>>>
>>>>>>>
>>>>>>>>        /*
>>>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>>>         * the complete width of the panel. Since, the complete
>>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>>>            return ret;
>>>>>>>>        }
>>>>>>>> -    dsc->initial_scale_value = 32;
>>>>>>>> +    dsc->initial_scale_value =
>>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>>
>>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>>
>>>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> With best wishes
>>>>>>> Dmitry
>>>>>>>
>>>>>
>>>>> -- 
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>>
>>>
> 

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

[Abhinav Kumar]

On 3/30/2023 7:47 PM, Dmitry Baryshkov wrote:
> On 31/03/2023 04:33, Abhinav Kumar wrote:
>>
>>
>> On 3/30/2023 5:16 PM, Dmitry Baryshkov wrote:
>>> On Fri, 31 Mar 2023 at 03:07, Jessica Zhang 
>>> <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/30/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>> On 31/03/2023 01:49, Jessica Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 3/29/2023 4:48 PM, Dmitry Baryshkov wrote:
>>>>>>> On 30/03/2023 02:18, Jessica Zhang wrote:
>>>>>>>> Use MSM and DRM DSC helper methods.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c | 18 ++++++++++++------
>>>>>>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> index 74d38f90398a..7419fe58a941 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>>    #include "msm_kms.h"
>>>>>>>>    #include "msm_gem.h"
>>>>>>>>    #include "phy/dsi_phy.h"
>>>>>>>> +#include "disp/msm_dsc_helper.h"
>>>>>>>>    #define DSI_RESET_TOGGLE_DELAY_MS 20
>>>>>>>> @@ -841,14 +842,14 @@ 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;
>>>>>>>>        /* 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);
>>>>>>>
>>>>>>> This looks good
>>>>>>>
>>>>>>>>        /*
>>>>>>>>         * If slice_count is greater than slice_per_intf
>>>>>>>> @@ -858,10 +859,10 @@ 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(msm_host->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;
>>>>>>>> +    pkt_per_line = slice_per_intf / MSM_DSC_SLICE_PER_PKT;
>>>>>>>
>>>>>>> And for these values the result is definitely changed. Separate 
>>>>>>> patch
>>>>>>> & description please. Just in case, "values per downstream 
>>>>>>> kernel" is
>>>>>>> not a proper description for such changes.
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Sure, I can put this into a separate patch.
>>>>>>
>>>>>> The reason this was changed from slice_count to SLICE_PER_PKT was
>>>>>> because slice count and slice per packet aren't always equivalent.
>>>>>> There can be cases where panel configures DSC to have multiple soft
>>>>>> slices per interface, but the panel only specifies 1 slice per 
>>>>>> packet.
>>>>>
>>>>> Please put this nice description into the commit message. It is 
>>>>> exactly
>>>>> what I was looking for!
>>>>>
>>>>> BTW: Do you expect to change MSM_DSC_SLICE_PER_PKT later or it will 
>>>>> stay
>>>>> at "1"? If so, it might be easier to drop it and instead add a 
>>>>> comment.
>>>>
>>>> MSM_DSC_SLICE_PER_PKT is the default value for panels that don't 
>>>> specify
>>>> a slice_per_pkt value. (Now that I think about it, might be better to
>>>> call it MSM_DSC_DEFAULT_SLICE_PER_PKT instead...)
>>>
>>> Note, there is no slice_per_pkt in drm_dsc_config, so we must come up
>>> with another way to pass this data from the panel or to deduce the
>>> value in our driver.
>>>
>>>>
>>>> I don't expect it to change in the future, but it's a little more
>>>> readable than just dividing by 1 IMO. If you prefer dropping the macro
>>>> and adding a comment, I'm also okay with that.
>>>
>>> There is no need to divide by 1, the value doesn't change. So I'd
>>> probably prefer something like:
>>>
>>> /* Default to 1 slice per packet */
>>> if (panel_slice_per_pkt)
>>>      pkt_per_line = slice_per_intf / panel_slice_per_pkt;
>>> else
>>>      pkt_per_line = slice_per_intf;
>>>
>>> Or:
>>>
>>> /* Default to 1 slice per packet */
>>> slice_per_pkt = 1;
>>> if (panel_slice_per_pkt)
>>>      slice_per_pkt = panel_slice_per_pkt;
>>> pkt_per_line = slice_per_intf / slice_per_pkt;
>>>
>>> BTW: could you possibly change 'intf' to 'line' to v2? It seems there
>>> is a mixture of them through the code. If there is a difference
>>> between intf and line which is not yet posted, it's fine to keep the
>>> current code. WDYT?
>>>
>>
>> No, I dont agree with the change from intf to line.
>>
>> In case of dual DSI, intf is not equal to line.
>>
>> 2 intfs = 1 line
>>
>> Hence that distinction is necessary.
> 
> Ack, this is what I was looking for!
> 
> so intf = line / num_intf?
> 

Yes by definition, "line" is one horizontal line of pixels for the panel.

So intf = h_active of panel / num_intf

But here "line" is one line of pixels pulled by the interface.

So for dual dsi cases its = h_active of panel / 2

> Maybe I should explain the reason for my question:
> 
> msm_dsc_get_pclk_per_line() uses intf_width, calculates pclk_per_line 
> (not per intf). msm_dsc_get_dce_bytes_per_line() does the same thing
> 
> In this patch we take slice_per_intf, divide it with slice_per_pkt and 
> get pkt_per_line (rather than pkt_per_intf).
> 
> This is what prompted my question regarding intf vs line.
> 

Valid question. The terminology gets a bit confusing because.

pclk_per_line can be only per interface.

Thats because each interface can pull the pixels at different pclks.

If it helps, I would say, this is pclk_per_line for each interface.

OR in other words pclks needed to pull one line of pixels for each 
interface.

But if i changed it to slice_per_line that would be wrong because then 
line becomes the full panel horizontal line.

>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>> Regarding eol_byte_num, probably the best explanation would be that is
>>>>> is a size of a padding rather than a size of a trailer bytes in a line
>>>>> (and thus original calculation was incorrect).
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>        if (is_cmd_mode) /* packet data type */
>>>>>>>>            reg =
>>>>>>>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE); 
>>>>>>>>
>>>>>>>> @@ -911,6 +912,11 @@ static void dsi_timing_setup(struct
>>>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>>>        DBG("");
>>>>>>>> +    if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)
>>>>>>>> +        /* Default widebus_en to false for now. */
>>>>>>>> +        hdisplay = msm_dsc_get_pclk_per_line(msm_host->dsc,
>>>>>>>> mode->hdisplay,
>>>>>>>> +                dsi_get_bpp(msm_host->format));
>>>>>>>> +
>>>>>>>
>>>>>>> This is definitely something new and thus should probably go into a
>>>>>>> separate patch and be described. Also I'm not sure how does that
>>>>>>> interact with the hdisplay-related calculations below, under the
>>>>>>> if(dsc) clause.
>>>>>>
>>>>>> After double-checking the math here, I think this part of the change
>>>>>> is actually wrong. pclk_per_line is essentially doing hdisplay / 3,
>>>>>> which is a repeat of what's being done in the `if (dsc)` block.
>>>>>>
>>>>>> Will replace `hdisplay /= 3` with the pclk_per_line calculation.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Jessica Zhang
>>>>>>
>>>>>>>
>>>>>>>>        /*
>>>>>>>>         * For bonded DSI mode, the current DRM mode has
>>>>>>>>         * the complete width of the panel. Since, the complete
>>>>>>>> @@ -1759,7 +1765,7 @@ static int dsi_populate_dsc_params(struct
>>>>>>>> msm_dsi_host *msm_host, struct drm_dsc
>>>>>>>>            return ret;
>>>>>>>>        }
>>>>>>>> -    dsc->initial_scale_value = 32;
>>>>>>>> +    dsc->initial_scale_value =
>>>>>>>> drm_dsc_calculate_initial_scale_value(dsc);
>>>>>>>
>>>>>>> This is fine, we only support 8bpp where these values match.
>>>>>>>
>>>>>>>>        dsc->line_buf_depth = dsc->bits_per_component + 1;
>>>>>>>>        return drm_dsc_compute_rc_parameters(dsc);
>>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> With best wishes
>>>>>>> Dmitry
>>>>>>>
>>>>>
>>>>> -- 
>>>>> With best wishes
>>>>> Dmitry
>>>>>
>>>
>>>
>>>
>