[PATCH v7 0/8] Introduce MSM-specific DSC helpers

[PATCH v7 0/8] Introduce MSM-specific DSC helpers

[Jessica Zhang]

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

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

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

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

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

---
Changes in v7:
- Renamed msm_dsc_get_pclk_per_intf to msm_dsc_get_bytes_per_line
- Removed duplicate msm_dsc_get_dce_bytes_per_line
- Reworded commit message for "drm/msm/dpu: Use DRM DSC helper for
  det_thresh_flatness"
- Dropped slice_per_pkt change (it will be included in the later
  "Add DSC v1.2 Support for DSI" series)
- Picked up "drm/display/dsc: Add flatness and initial scale value
  calculations" and "drm/display/dsc: add helper to set semi-const
  parameters", which were dropped from "drm/i915: move DSC RC tables to
  drm_dsc_helper.c" series
- Picked up "Reviewed-by" tags
- Removed changelog in individual patches
- Link to v6: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v6-0-cb7f59f0f7fb@quicinc.com

Changes in v6:
- Documented return values for MSM DSC helpers
- Fixed dependency issue in msm_dsc_helper.c
- Link to v5: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v5-0-0108401d7886@quicinc.com

Changes in v5:
- Added extra line at end of msm_dsc_helper.h
- Simplified msm_dsc_get_bytes_per_soft_slice() math
- Simplified and inlined msm_dsc_get_pclk_per_intf() math
- "Fix calculations pkt_per_line" --> "... Fix calculation for pkt_per_line"
- Split dsc->slice_width check into a separate patch
- Picked up Dmitry's msm/dsi patch ("drm/msm/dsi: use new helpers for
  DSC setup")
- Removed unused headers in MSM DSC helper files
- Picked up Reviewed-by tags
- Link to v4: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v4-0-1b79c78b30d7@quicinc.com

Changes in v4:
- Changed msm_dsc_get_uncompressed_pclk_per_intf to msm_dsc_get_pclk_per_intf
- Moved pclk_per_intf calculation for dsi_timing_setup to `if
  (msm_host->dsc)` block
- Link to v3: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v3-0-6bec0d277a83@quicinc.com

Changes in v3:
- Dropped src_bpp parameter from all methods -- src_bpp can be
  calculated as dsc->bits_per_component * 3- Cleaned up unused parameters
- Dropped intf_width parameter from get_bytes_per_soft_slice()
- Moved dsc->bits_per_component to numerator calculation in
  get_bytes_per_soft_slice()
- Made get_bytes_per_soft_slice() a public method (this will be called
  later to help calculate DP pclk params)- Added comment documentation to
  MSM DSC helpers
- Renamed msm_dsc_get_uncompressed_pclk_per_line to
  *_get_uncompressed_pclk_per_intf()
- Removed dsc->slice_width check from msm_dsc_get_uncompressed_pclk_per_intf()
- Added documentation in comments
- Moved extra_eol_bytes math out of msm_dsc_get_eol_byte_num()
- Renamed msm_dsc_get_eol_byte_num to *_get_bytes_per_intf.
- Reworded slice_last_group_size calculation to `(dsc->slice_width + 2) % 3`
- Used MSM DSC helper to calculate total_bytes_per_intf
- Initialized hdisplay as uncompressed pclk per line at the beginning of
  dsi_timing_setup as to not break dual DSI calculations
- Added slice_width check to dsi_timing_setup
- Dropped 78c8b81d57d8 ("drm/display/dsc: Add flatness and initial scale
  value calculations") patch as it was absorbed in Dmitry's DSC series [1]
- Split dsi_timing_setup() hdisplay calculation to a separate patch
- Link to v2: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v2-0-3c13ced536b2@quicinc.com

Changes in v2:
- Changed det_thresh_flatness to flatness_det_thresh
- Set initial_scale_value directly in helper
- Moved msm_dsc_helper files to msm/ directory
- Dropped get_comp_ratio() helper
- Used drm_int2fixp() to convert to integers to fp
- Fixed type mismatch issues in MSM DSC helpers
- Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method
- Style changes to improve readability
- Dropped last division step of msm_dsc_get_pclk_per_line() and changed
  method name accordingly
- Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line()
- Changed msm_dsc_get_slice_per_intf() to a static inline method
- Split eol_byte_num and pkt_per_line calculation into a separate patch
- Moved pclk_per_line calculation into `if (dsc)` block in
  dsi_timing_setup()
- *_calculate_initial_scale_value --> *_set_initial_scale_value
- Picked up Fixes tags for patches 3/5 and 4/5
- Picked up Reviewed-by for patch 4/5
- Link to v1: https://lore.kernel.org/r/20230329-rfc-msm-dsc-helper-v1-0-f3e479f59b6d@quicinc.com

---
Dmitry Baryshkov (2):
      drm/display/dsc: add helper to set semi-const parameters
      drm/msm/dsi: use DRM DSC helpers for DSC setup

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

 drivers/gpu/drm/display/drm_dsc_helper.c   | 22 ++++++++++
 drivers/gpu/drm/msm/Makefile               |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c |  9 ++--
 drivers/gpu/drm/msm/dsi/dsi_host.c         | 70 ++++++------------------------
 drivers/gpu/drm/msm/msm_dsc_helper.c       | 26 +++++++++++
 drivers/gpu/drm/msm/msm_dsc_helper.h       | 69 +++++++++++++++++++++++++++++
 include/drm/display/drm_dsc_helper.h       | 12 +++++
 7 files changed, 149 insertions(+), 60 deletions(-)
---
base-commit: 5526fe03b40ca1cc72c7b4e97f28d3bbfaa0ded9
change-id: 20230329-rfc-msm-dsc-helper-981a95edfbd0

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

[PATCH v7 1/8] 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: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 include/drm/display/drm_dsc_helper.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
index 0bb0c3afd740..422135a33d65 100644
--- a/include/drm/display/drm_dsc_helper.h
+++ b/include/drm/display/drm_dsc_helper.h
@@ -25,5 +25,16 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
 int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
 int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
 
+static inline void drm_dsc_set_initial_scale_value(struct drm_dsc_config *dsc)
+{
+	dsc->initial_scale_value = 8 * dsc->rc_model_size /
+		(dsc->rc_model_size - dsc->initial_offset);
+}
+
+static inline int drm_dsc_calculate_flatness_det_thresh(struct drm_dsc_config *dsc)
+{
+	return 2 << (dsc->bits_per_component - 8);
+}
+
 #endif /* _DRM_DSC_HELPER_H_ */
 

-- 
2.40.1

[PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Jessica Zhang]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Add a helper setting config values which are typically constant across
operating modes (table E-4 of the standard) and mux_word_size (which is
a const according to 3.5.2).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/display/drm_dsc_helper.c | 22 ++++++++++++++++++++++
 include/drm/display/drm_dsc_helper.h     |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
index 65e810a54257..b9c4e10ced41 100644
--- a/drivers/gpu/drm/display/drm_dsc_helper.c
+++ b/drivers/gpu/drm/display/drm_dsc_helper.c
@@ -270,6 +270,28 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
 }
 EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
 
+/**
+ * drm_dsc_set_const_params() - Set DSC parameters considered typically
+ * constant across operation modes
+ *
+ * @vdsc_cfg:
+ * DSC Configuration data partially filled by driver
+ */
+void drm_dsc_set_const_params(struct drm_dsc_config *vdsc_cfg)
+{
+	if (!vdsc_cfg->rc_model_size)
+		vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
+	vdsc_cfg->rc_edge_factor = DSC_RC_EDGE_FACTOR_CONST;
+	vdsc_cfg->rc_tgt_offset_high = DSC_RC_TGT_OFFSET_HI_CONST;
+	vdsc_cfg->rc_tgt_offset_low = DSC_RC_TGT_OFFSET_LO_CONST;
+
+	if (vdsc_cfg->bits_per_component <= 10)
+		vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
+	else
+		vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_12_BPC;
+}
+EXPORT_SYMBOL(drm_dsc_set_const_params);
+
 /* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
 static const u16 drm_dsc_rc_buf_thresh[] = {
 	896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
index 422135a33d65..bfa7f3acafcb 100644
--- a/include/drm/display/drm_dsc_helper.h
+++ b/include/drm/display/drm_dsc_helper.h
@@ -21,6 +21,7 @@ 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,
 			      const struct drm_dsc_config *dsc_cfg);
+void drm_dsc_set_const_params(struct drm_dsc_config *vdsc_cfg);
 void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
 int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
 int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);

-- 
2.40.1

[PATCH v7 3/8] drm/msm/dsi: use DRM DSC helpers for DSC setup

[Jessica Zhang]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Use new DRM DSC helpers to setup DSI DSC configuration. The
initial_scale_value needs to be adjusted according to the standard, but
this is a separate change.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 61 +++++---------------------------------
 1 file changed, 8 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 961689a255c4..74d38f90398a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1731,28 +1731,9 @@ static int dsi_host_parse_lane_data(struct msm_dsi_host *msm_host,
 	return -EINVAL;
 }
 
-static u32 dsi_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
-	0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54, 0x62,
-	0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
-};
-
-/* only 8bpc, 8bpp added */
-static char min_qp[DSC_NUM_BUF_RANGES] = {
-	0, 0, 1, 1, 3, 3, 3, 3, 3, 3, 5, 5, 5, 7, 13
-};
-
-static char max_qp[DSC_NUM_BUF_RANGES] = {
-	4, 4, 5, 6, 7, 7, 7, 8, 9, 10, 11, 12, 13, 13, 15
-};
-
-static char bpg_offset[DSC_NUM_BUF_RANGES] = {
-	2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12
-};
-
 static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc_config *dsc)
 {
-	int i;
-	u16 bpp = dsc->bits_per_pixel >> 4;
+	int ret;
 
 	if (dsc->bits_per_pixel & 0xf) {
 		DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support fractional bits_per_pixel\n");
@@ -1764,49 +1745,23 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 		return -EOPNOTSUPP;
 	}
 
-	dsc->rc_model_size = 8192;
-	dsc->first_line_bpg_offset = 12;
-	dsc->rc_edge_factor = 6;
-	dsc->rc_tgt_offset_high = 3;
-	dsc->rc_tgt_offset_low = 3;
 	dsc->simple_422 = 0;
 	dsc->convert_rgb = 1;
 	dsc->vbr_enable = 0;
 
-	/* handle only bpp = bpc = 8 */
-	for (i = 0; i < DSC_NUM_BUF_RANGES - 1 ; i++)
-		dsc->rc_buf_thresh[i] = dsi_dsc_rc_buf_thresh[i];
+	drm_dsc_set_const_params(dsc);
+	drm_dsc_set_rc_buf_thresh(dsc);
 
-	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
-		dsc->rc_range_params[i].range_min_qp = min_qp[i];
-		dsc->rc_range_params[i].range_max_qp = max_qp[i];
-		/*
-		 * Range BPG Offset contains two's-complement signed values that fill
-		 * 8 bits, yet the registers and DCS PPS field are only 6 bits wide.
-		 */
-		dsc->rc_range_params[i].range_bpg_offset = bpg_offset[i] & DSC_RANGE_BPG_OFFSET_MASK;
+	/* handle only bpp = bpc = 8, pre-SCR panels */
+	ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
+	if (ret) {
+		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n");
+		return ret;
 	}
 
-	dsc->initial_offset = 6144;		/* Not bpp 12 */
-	if (bpp != 8)
-		dsc->initial_offset = 2048;	/* bpp = 12 */
-
-	if (dsc->bits_per_component <= 10)
-		dsc->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
-	else
-		dsc->mux_word_size = DSC_MUX_WORD_SIZE_12_BPC;
-
-	dsc->initial_xmit_delay = 512;
 	dsc->initial_scale_value = 32;
-	dsc->first_line_bpg_offset = 12;
 	dsc->line_buf_depth = dsc->bits_per_component + 1;
 
-	/* bpc 8 */
-	dsc->flatness_min_qp = 3;
-	dsc->flatness_max_qp = 12;
-	dsc->rc_quant_incr_limit0 = 11;
-	dsc->rc_quant_incr_limit1 = 11;
-
 	return drm_dsc_compute_rc_parameters(dsc);
 }
 

-- 
2.40.1

[PATCH v7 4/8] 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.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/Makefile         |  1 +
 drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
 drivers/gpu/drm/msm/msm_dsc_helper.h | 69 ++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 7274c41228ed..b814fc80e2d5 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -94,6 +94,7 @@ msm-y += \
 	msm_atomic_tracepoints.o \
 	msm_debugfs.o \
 	msm_drv.o \
+	msm_dsc_helper.o \
 	msm_fb.o \
 	msm_fence.o \
 	msm_gem.o \
diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c
new file mode 100644
index 000000000000..29feb3e3b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
+ */
+
+#include <linux/kernel.h>
+#include <drm/drm_fixed.h>
+
+#include "msm_dsc_helper.h"
+
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
+{
+	return drm_fixp_from_fraction(dsc->slice_width * msm_dsc_get_bpp_int(dsc), 8);
+}
+
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
+{
+	u32 bytes_per_soft_slice;
+	s64 bytes_per_soft_slice_fp;
+	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
+
+	bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
+	bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
+
+	return bytes_per_soft_slice * slice_per_intf;
+}
diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
new file mode 100644
index 000000000000..38f3651d0b79
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -0,0 +1,69 @@
+/* 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 <linux/bug.h>
+#include <linux/math.h>
+#include <drm/display/drm_dsc_helper.h>
+
+/*
+ * Helper methods for MSM specific DSC calculations that are common between timing engine,
+ * DSI, and DP.
+ */
+
+/**
+ * msm_dsc_get_bpp_int - get bits per pixel integer value
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: BPP integer value
+ */
+static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
+{
+	WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
+	return dsc->bits_per_pixel >> 4;
+}
+
+/**
+ * msm_dsc_get_slice_per_intf - get number of slices per interface
+ * @dsc: Pointer to drm dsc config struct
+ * @intf_width: interface width
+ * Returns: Integer representing the slice per interface
+ */
+static inline int msm_dsc_get_slice_per_intf(struct drm_dsc_config *dsc, int intf_width)
+{
+	return DIV_ROUND_UP(intf_width, dsc->slice_width);
+}
+
+/**
+ * msm_dsc_get_bytes_per_line - Calculate bytes per line
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: Integer value representing pclk per interface
+ *
+ * Note: This value will then be passed along to DSI and DP for some more
+ * calculations. This is because DSI and DP divide the pclk_per_intf value
+ * by different values depending on if widebus is enabled.
+ */
+static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
+{
+	return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * msm_dsc_get_bpp_int(dsc), 8);
+}
+
+/**
+ * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: s31.32 fixed point value representing bytes per soft slice
+ */
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
+
+/**
+ * msm_dsc_get_bytes_per_intf - get total bytes per interface
+ * @dsc: Pointer to drm dsc config struct
+ * @intf_width: interface width
+ * Returns: u32 value representing bytes per interface
+ */
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
+
+#endif /* MSM_DSC_HELPER_H_ */

-- 
2.40.1

[PATCH v7 5/8] drm/msm/dpu: Use DRM DSC helper for det_thresh_flatness

[Jessica Zhang]

The current dpu_hw_dsc calculation for det_thresh_flatness does not
match the downstream calculation or the DSC spec.

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

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
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 4e1396575e6a..1e11c0fb0545 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2020-2022, Linaro Limited
  */
 
+#include <drm/display/drm_dsc_helper.h>
+
 #include "dpu_kms.h"
 #include "dpu_hw_catalog.h"
 #include "dpu_hwio.h"
@@ -102,7 +104,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 	data |= dsc->final_offset;
 	DPU_REG_WRITE(c, DSC_DSC_OFFSET, data);
 
-	det_thresh_flatness = 7 + 2 * (dsc->bits_per_component - 8);
+	det_thresh_flatness = drm_dsc_calculate_flatness_det_thresh(dsc);
 	data = det_thresh_flatness << 10;
 	data |= dsc->flatness_max_qp << 5;
 	data |= dsc->flatness_min_qp;

-- 
2.40.1

[PATCH v7 6/8] drm/msm/dpu: Fix slice_last_group_size calculation

[Jessica Zhang]

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

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

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 1e11c0fb0545..ddaec05151cd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -56,9 +56,10 @@ 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 + 2) % 3;
+
 	data = (initial_lines << 20);
-	data |= ((slice_last_group_size - 1) << 18);
+	data |= (slice_last_group_size << 18);
 	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
 	data |= (dsc->bits_per_pixel << 8);
 	data |= (dsc->block_pred_enable << 7);

-- 
2.40.1

[PATCH v7 7/8] drm/msm/dsi: Use MSM and DRM DSC helper methods

[Jessica Zhang]

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

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 74d38f90398a..508577c596ff 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -28,6 +28,7 @@
 #include "dsi.xml.h"
 #include "sfpb.xml.h"
 #include "dsi_cfg.h"
+#include "msm_dsc_helper.h"
 #include "msm_kms.h"
 #include "msm_gem.h"
 #include "phy/dsi_phy.h"
@@ -848,7 +849,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	/* first calculate dsc parameters and then program
 	 * compress mode registers
 	 */
-	slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
+	slice_per_intf = msm_dsc_get_slice_per_intf(dsc, hdisplay);
 
 	/*
 	 * If slice_count is greater than slice_per_intf
@@ -858,7 +859,7 @@ 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;
+	total_bytes_per_intf = msm_dsc_get_bytes_per_intf(dsc, hdisplay);
 
 	eol_byte_num = total_bytes_per_intf % 3;
 	pkt_per_line = slice_per_intf / dsc->slice_count;
@@ -1759,7 +1760,7 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 		return ret;
 	}
 
-	dsc->initial_scale_value = 32;
+	drm_dsc_set_initial_scale_value(dsc);
 	dsc->line_buf_depth = dsc->bits_per_component + 1;
 
 	return drm_dsc_compute_rc_parameters(dsc);

-- 
2.40.1

[PATCH v7 8/8] drm/msm/dsi: update hdisplay calculation for dsi_timing_setup

[Jessica Zhang]

hdisplay for compressed images should be calculated as bytes_per_slice *
slice_count. Thus, use MSM DSC helper to calculate hdisplay for
dsi_timing_setup instead of directly using mode->hdisplay.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 508577c596ff..d60403372514 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * pulse width same
 		 */
 		h_total -= hdisplay;
-		hdisplay /= 3;
+		hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}

-- 
2.40.1

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

[Marijn Suijten]

On 2023-05-09 15:06:47, 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: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

This ordering is odd: Jessica originally sent the patch, then Dmitry
seems to have sent it as part of his series and added his s-o-b [1], but
now Jessica is sending her original patch again but with Dmitry's
sign-off first.

[1]: https://lore.kernel.org/linux-arm-msm/20230403092313.235320-11-dmitry.baryshkov@linaro.org/

> ---
>  include/drm/display/drm_dsc_helper.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> index 0bb0c3afd740..422135a33d65 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -25,5 +25,16 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
>  int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
>  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>  
> +static inline void drm_dsc_set_initial_scale_value(struct drm_dsc_config *dsc)

Nit: the other functions are not static inline, and are called "compute"
or "setup" (or "calculate" below) for maths.

The math itself is:

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> +{
> +	dsc->initial_scale_value = 8 * dsc->rc_model_size /
> +		(dsc->rc_model_size - dsc->initial_offset);
> +}
> +
> +static inline int drm_dsc_calculate_flatness_det_thresh(struct drm_dsc_config *dsc)
> +{
> +	return 2 << (dsc->bits_per_component - 8);
> +}
> +
>  #endif /* _DRM_DSC_HELPER_H_ */
>  
> 
> -- 
> 2.40.1
> 

Re: [PATCH v7 8/8] drm/msm/dsi: update hdisplay calculation for dsi_timing_setup

[Marijn Suijten]

On 2023-05-09 15:06:54, Jessica Zhang wrote:
> hdisplay for compressed images should be calculated as bytes_per_slice *
> slice_count. Thus, use MSM DSC helper to calculate hdisplay for

For most intents and purposes, these values are the same when
bits_per_pixel=8.

We should find a place where we can assert that
slice_width*slice_count==hdisplay, though.

> dsi_timing_setup instead of directly using mode->hdisplay.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 508577c596ff..d60403372514 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		 * pulse width same
>  		 */
>  		h_total -= hdisplay;
> -		hdisplay /= 3;
> +		hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
>  		h_total += hdisplay;
>  		ha_end = ha_start + hdisplay;
>  	}
> 
> -- 
> 2.40.1
> 

Re: [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Marijn Suijten]

On 2023-05-09 15:06:48, Jessica Zhang wrote:
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Add a helper setting config values which are typically constant across
> operating modes (table E-4 of the standard) and mux_word_size (which is
> a const according to 3.5.2).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Same question about ordering.

Reviewed-by:  Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/display/drm_dsc_helper.c | 22 ++++++++++++++++++++++
>  include/drm/display/drm_dsc_helper.h     |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index 65e810a54257..b9c4e10ced41 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -270,6 +270,28 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>  }
>  EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>  
> +/**
> + * drm_dsc_set_const_params() - Set DSC parameters considered typically
> + * constant across operation modes
> + *
> + * @vdsc_cfg:
> + * DSC Configuration data partially filled by driver
> + */
> +void drm_dsc_set_const_params(struct drm_dsc_config *vdsc_cfg)
> +{
> +	if (!vdsc_cfg->rc_model_size)
> +		vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> +	vdsc_cfg->rc_edge_factor = DSC_RC_EDGE_FACTOR_CONST;
> +	vdsc_cfg->rc_tgt_offset_high = DSC_RC_TGT_OFFSET_HI_CONST;
> +	vdsc_cfg->rc_tgt_offset_low = DSC_RC_TGT_OFFSET_LO_CONST;
> +
> +	if (vdsc_cfg->bits_per_component <= 10)
> +		vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
> +	else
> +		vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_12_BPC;
> +}
> +EXPORT_SYMBOL(drm_dsc_set_const_params);
> +
>  /* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
>  static const u16 drm_dsc_rc_buf_thresh[] = {
>  	896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> index 422135a33d65..bfa7f3acafcb 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -21,6 +21,7 @@ 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,
>  			      const struct drm_dsc_config *dsc_cfg);
> +void drm_dsc_set_const_params(struct drm_dsc_config *vdsc_cfg);
>  void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
>  int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
>  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
> 
> -- 
> 2.40.1
> 

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Marijn Suijten]

On 2023-05-09 15:06:50, Jessica Zhang wrote:
> Introduce MSM-specific DSC helper methods, as some calculations are
> common between DP and DSC.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/Makefile         |  1 +
>  drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
>  drivers/gpu/drm/msm/msm_dsc_helper.h | 69 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 7274c41228ed..b814fc80e2d5 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -94,6 +94,7 @@ msm-y += \
>  	msm_atomic_tracepoints.o \
>  	msm_debugfs.o \
>  	msm_drv.o \
> +	msm_dsc_helper.o \
>  	msm_fb.o \
>  	msm_fence.o \
>  	msm_gem.o \
> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c
> new file mode 100644
> index 000000000000..29feb3e3b5a4
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> + */
> +
> +#include <linux/kernel.h>
> +#include <drm/drm_fixed.h>
> +
> +#include "msm_dsc_helper.h"
> +
> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
> +{
> +	return drm_fixp_from_fraction(dsc->slice_width * msm_dsc_get_bpp_int(dsc), 8);

How about using dsc->slice_chunk_size?

> +}
> +
> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
> +{
> +	u32 bytes_per_soft_slice;
> +	s64 bytes_per_soft_slice_fp;
> +	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
> +
> +	bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
> +	bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
> +
> +	return bytes_per_soft_slice * slice_per_intf;
> +}
> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
> new file mode 100644
> index 000000000000..38f3651d0b79
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
> @@ -0,0 +1,69 @@
> +/* 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 <linux/bug.h>
> +#include <linux/math.h>
> +#include <drm/display/drm_dsc_helper.h>
> +
> +/*
> + * Helper methods for MSM specific DSC calculations that are common between timing engine,
> + * DSI, and DP.
> + */
> +
> +/**
> + * msm_dsc_get_bpp_int - get bits per pixel integer value

For all function docs, don't forget the trailing parenthesis after the
function name: msm_dsc_get_bpp_int()

https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation

> + * @dsc: Pointer to drm dsc config struct
> + * Returns: BPP integer value
> + */
> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
> +{
> +	WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
> +	return dsc->bits_per_pixel >> 4;
> +}
> +
> +/**
> + * msm_dsc_get_slice_per_intf - get number of slices per interface
> + * @dsc: Pointer to drm dsc config struct
> + * @intf_width: interface width
> + * Returns: Integer representing the slice per interface
> + */
> +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);

Looks good.

> +}
> +
> +/**
> + * msm_dsc_get_bytes_per_line - Calculate bytes per line
> + * @dsc: Pointer to drm dsc config struct
> + * Returns: Integer value representing pclk per interface
> + *
> + * Note: This value will then be passed along to DSI and DP for some more
> + * calculations. This is because DSI and DP divide the pclk_per_intf value
> + * by different values depending on if widebus is enabled.
> + */
> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
> +{
> +	return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * msm_dsc_get_bpp_int(dsc), 8);

dsc->slice_chunk_size * dsc->slice_count?

> +}
> +
> +/**
> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc

Explain to the reader what a "soft" slice is?

- Marijn

> + * @dsc: Pointer to drm dsc config struct
> + * Returns: s31.32 fixed point value representing bytes per soft slice
> + */
> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
> +
> +/**
> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
> + * @dsc: Pointer to drm dsc config struct
> + * @intf_width: interface width
> + * Returns: u32 value representing bytes per interface
> + */
> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
> +
> +#endif /* MSM_DSC_HELPER_H_ */
> 
> -- 
> 2.40.1
> 

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

[Marijn Suijten]

Title suggestion: Use **fixed** MSM DSC helper...

To make it clear that this is a bugfix without having to read the commit
description first.

- Marijn

On 2023-05-09 15:06:51, Jessica Zhang wrote:
> The current dpu_hw_dsc calculation for det_thresh_flatness does not
> match the downstream calculation or the DSC spec.
> 
> Use the DRM DSC helper for det_thresh_flatness to match downstream
> implementation and the DSC spec.
> 
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 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 4e1396575e6a..1e11c0fb0545 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -3,6 +3,8 @@
>   * Copyright (c) 2020-2022, Linaro Limited
>   */
>  
> +#include <drm/display/drm_dsc_helper.h>
> +
>  #include "dpu_kms.h"
>  #include "dpu_hw_catalog.h"
>  #include "dpu_hwio.h"
> @@ -102,7 +104,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>  	data |= dsc->final_offset;
>  	DPU_REG_WRITE(c, DSC_DSC_OFFSET, data);
>  
> -	det_thresh_flatness = 7 + 2 * (dsc->bits_per_component - 8);
> +	det_thresh_flatness = drm_dsc_calculate_flatness_det_thresh(dsc);
>  	data = det_thresh_flatness << 10;
>  	data |= dsc->flatness_max_qp << 5;
>  	data |= dsc->flatness_min_qp;
> 
> -- 
> 2.40.1
> 

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Jessica Zhang]

On 5/9/2023 11:33 PM, Marijn Suijten wrote:
> On 2023-05-09 15:06:50, Jessica Zhang wrote:
>> Introduce MSM-specific DSC helper methods, as some calculations are
>> common between DP and DSC.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/Makefile         |  1 +
>>   drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
>>   drivers/gpu/drm/msm/msm_dsc_helper.h | 69 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 96 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index 7274c41228ed..b814fc80e2d5 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -94,6 +94,7 @@ msm-y += \
>>   	msm_atomic_tracepoints.o \
>>   	msm_debugfs.o \
>>   	msm_drv.o \
>> +	msm_dsc_helper.o \
>>   	msm_fb.o \
>>   	msm_fence.o \
>>   	msm_gem.o \
>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c
>> new file mode 100644
>> index 000000000000..29feb3e3b5a4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
>> @@ -0,0 +1,26 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <drm/drm_fixed.h>
>> +
>> +#include "msm_dsc_helper.h"
>> +
>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
>> +{
>> +	return drm_fixp_from_fraction(dsc->slice_width * msm_dsc_get_bpp_int(dsc), 8);
> 
> How about using dsc->slice_chunk_size?

Hi Marijn,

Thanks for pointing this out. However, I would prefer to keep this fixed 
point version of the slice_chunk_size math as the downstream DP math 
also uses fixed point [1].

If we are able to confirm that integer math also works for DP, we will 
make the change to use slice_chunk_size within the DP DSC series.

I also want to note that this math has stayed the same throughout all 7 
revisions. In the interest of making review more efficient, I think it 
would be helpful to point out important details like this early on in 
the process. That way we can address major concerns early on and keep 
the number of revisions per series low.

[1] 
https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335

> 
>> +}
>> +
>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
>> +{
>> +	u32 bytes_per_soft_slice;
>> +	s64 bytes_per_soft_slice_fp;
>> +	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
>> +
>> +	bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
>> +	bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
>> +
>> +	return bytes_per_soft_slice * slice_per_intf;
>> +}
>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
>> new file mode 100644
>> index 000000000000..38f3651d0b79
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
>> @@ -0,0 +1,69 @@
>> +/* 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 <linux/bug.h>
>> +#include <linux/math.h>
>> +#include <drm/display/drm_dsc_helper.h>
>> +
>> +/*
>> + * Helper methods for MSM specific DSC calculations that are common between timing engine,
>> + * DSI, and DP.
>> + */
>> +
>> +/**
>> + * msm_dsc_get_bpp_int - get bits per pixel integer value
> 
> For all function docs, don't forget the trailing parenthesis after the
> function name: msm_dsc_get_bpp_int()
> 
> https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation

Acked.

> 
>> + * @dsc: Pointer to drm dsc config struct
>> + * Returns: BPP integer value
>> + */
>> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
>> +{
>> +	WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
>> +	return dsc->bits_per_pixel >> 4;
>> +}
>> +
>> +/**
>> + * msm_dsc_get_slice_per_intf - get number of slices per interface
>> + * @dsc: Pointer to drm dsc config struct
>> + * @intf_width: interface width
>> + * Returns: Integer representing the slice per interface
>> + */
>> +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);
> 
> Looks good.
> 
>> +}
>> +
>> +/**
>> + * msm_dsc_get_bytes_per_line - Calculate bytes per line
>> + * @dsc: Pointer to drm dsc config struct
>> + * Returns: Integer value representing pclk per interface
>> + *
>> + * Note: This value will then be passed along to DSI and DP for some more
>> + * calculations. This is because DSI and DP divide the pclk_per_intf value
>> + * by different values depending on if widebus is enabled.
>> + */
>> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
>> +{
>> +	return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * msm_dsc_get_bpp_int(dsc), 8);
> 
> dsc->slice_chunk_size * dsc->slice_count?

Acked.

> 
>> +}
>> +
>> +/**
>> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
> 
> Explain to the reader what a "soft" slice is?

A soft slice is a slice defined in software as opposed to "hard slices" 
that are defined by hardware.

Since the slice-related variables in drm_dsc_config hold information 
related to soft slices and there is no separate variable for hard 
slices, I'll change this name to *_get_bytes_per_slice instead.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>> + * @dsc: Pointer to drm dsc config struct
>> + * Returns: s31.32 fixed point value representing bytes per soft slice
>> + */
>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
>> +
>> +/**
>> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
>> + * @dsc: Pointer to drm dsc config struct
>> + * @intf_width: interface width
>> + * Returns: u32 value representing bytes per interface
>> + */
>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
>> +
>> +#endif /* MSM_DSC_HELPER_H_ */
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Jessica Zhang]

On 5/9/2023 11:29 PM, Marijn Suijten wrote:
> On 2023-05-09 15:06:48, Jessica Zhang wrote:
>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Add a helper setting config values which are typically constant across
>> operating modes (table E-4 of the standard) and mux_word_size (which is
>> a const according to 3.5.2).
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Same question about ordering.

Hi Marijn,

This patch was authored by Dmitry and originally part of his DRM DSC 
helpers series [1], but was removed from that series for mergeability 
reasons.

Looking over the kernel documentation, the last Signed-off-by should be 
from the patch submitter [2], so I think my s-o-b tag should be at the 
bottom.

As for the order in the previous patch, I can add a duplicate s-o-b 
before Dmitry's so that it reflects the history of the patch.

Thanks,

Jessica Zhang

[1] https://patchwork.freedesktop.org/patch/530512/?series=114472&rev=4
[2] 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> 
> Reviewed-by:  Marijn Suijten <marijn.suijten@somainline.org>
> 
>> ---
>>   drivers/gpu/drm/display/drm_dsc_helper.c | 22 ++++++++++++++++++++++
>>   include/drm/display/drm_dsc_helper.h     |  1 +
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
>> index 65e810a54257..b9c4e10ced41 100644
>> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>> @@ -270,6 +270,28 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>>   }
>>   EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>>   
>> +/**
>> + * drm_dsc_set_const_params() - Set DSC parameters considered typically
>> + * constant across operation modes
>> + *
>> + * @vdsc_cfg:
>> + * DSC Configuration data partially filled by driver
>> + */
>> +void drm_dsc_set_const_params(struct drm_dsc_config *vdsc_cfg)
>> +{
>> +	if (!vdsc_cfg->rc_model_size)
>> +		vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
>> +	vdsc_cfg->rc_edge_factor = DSC_RC_EDGE_FACTOR_CONST;
>> +	vdsc_cfg->rc_tgt_offset_high = DSC_RC_TGT_OFFSET_HI_CONST;
>> +	vdsc_cfg->rc_tgt_offset_low = DSC_RC_TGT_OFFSET_LO_CONST;
>> +
>> +	if (vdsc_cfg->bits_per_component <= 10)
>> +		vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
>> +	else
>> +		vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_12_BPC;
>> +}
>> +EXPORT_SYMBOL(drm_dsc_set_const_params);
>> +
>>   /* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
>>   static const u16 drm_dsc_rc_buf_thresh[] = {
>>   	896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
>> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
>> index 422135a33d65..bfa7f3acafcb 100644
>> --- a/include/drm/display/drm_dsc_helper.h
>> +++ b/include/drm/display/drm_dsc_helper.h
>> @@ -21,6 +21,7 @@ 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,
>>   			      const struct drm_dsc_config *dsc_cfg);
>> +void drm_dsc_set_const_params(struct drm_dsc_config *vdsc_cfg);
>>   void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
>>   int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
>>   int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Dmitry Baryshkov]

On 11/05/2023 01:35, Jessica Zhang wrote:
> 
> 
> On 5/9/2023 11:29 PM, Marijn Suijten wrote:
>> On 2023-05-09 15:06:48, Jessica Zhang wrote:
>>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> Add a helper setting config values which are typically constant across
>>> operating modes (table E-4 of the standard) and mux_word_size (which is
>>> a const according to 3.5.2).
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Same question about ordering.
> 
> Hi Marijn,
> 
> This patch was authored by Dmitry and originally part of his DRM DSC 
> helpers series [1], but was removed from that series for mergeability 
> reasons.
> 
> Looking over the kernel documentation, the last Signed-off-by should be 
> from the patch submitter [2], so I think my s-o-b tag should be at the 
> bottom.
> 
> As for the order in the previous patch, I can add a duplicate s-o-b 
> before Dmitry's so that it reflects the history of the patch.

I think this is an overkill. Instead you can drop my SOB from the patch 
1. We do not need this level of detail.

For this patch the ordering of tags is correct.

-- 
With best wishes
Dmitry

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Marijn Suijten]

On 2023-05-10 14:03:14, Jessica Zhang wrote:
> 
> 
> On 5/9/2023 11:33 PM, Marijn Suijten wrote:
> > On 2023-05-09 15:06:50, Jessica Zhang wrote:
> >> Introduce MSM-specific DSC helper methods, as some calculations are
> >> common between DP and DSC.
> >>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/Makefile         |  1 +
> >>   drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
> >>   drivers/gpu/drm/msm/msm_dsc_helper.h | 69 ++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 96 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> >> index 7274c41228ed..b814fc80e2d5 100644
> >> --- a/drivers/gpu/drm/msm/Makefile
> >> +++ b/drivers/gpu/drm/msm/Makefile
> >> @@ -94,6 +94,7 @@ msm-y += \
> >>   	msm_atomic_tracepoints.o \
> >>   	msm_debugfs.o \
> >>   	msm_drv.o \
> >> +	msm_dsc_helper.o \
> >>   	msm_fb.o \
> >>   	msm_fence.o \
> >>   	msm_gem.o \
> >> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c
> >> new file mode 100644
> >> index 000000000000..29feb3e3b5a4
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
> >> @@ -0,0 +1,26 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <drm/drm_fixed.h>
> >> +
> >> +#include "msm_dsc_helper.h"
> >> +
> >> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
> >> +{
> >> +	return drm_fixp_from_fraction(dsc->slice_width * msm_dsc_get_bpp_int(dsc), 8);
> > 
> > How about using dsc->slice_chunk_size?
> 
> Hi Marijn,
> 
> Thanks for pointing this out. However, I would prefer to keep this fixed 
> point version of the slice_chunk_size math as the downstream DP math 
> also uses fixed point [1].

That looks like a totally different computation.  For this specific
value it appears drm_fixp_from_fraction does rounding whereas
DIV_ROUND_UP always rounds up when used for slice_chunk_size, but these
values are generally multiples of 8.  slice_chunk_size will also take
fractional bpp into account (even if unsupported by DPU otherwise).

> If we are able to confirm that integer math also works for DP, we will 
> make the change to use slice_chunk_size within the DP DSC series.

Don't think the DP series alone should point that out, as it heavily
depends on the relation between slice size and bpp parameters, and
whether those end up with a fractional part or not (are you able to test
and confirm all those combinations?).  Regardless it seems more natural
to use slice_chunk_size which is used in various other ways and sent in
the PPS: it wouldn't make sense to adhere to a different slice byte
count elsewhere.

> I also want to note that this math has stayed the same throughout all 7 
> revisions. In the interest of making review more efficient, I think it 
> would be helpful to point out important details like this early on in 
> the process. That way we can address major concerns early on and keep 
> the number of revisions per series low.

I've already expressed to Abhinav and you that the revisions for all
these series were coming in way too hot, leaving no time for review and
discussions before the next revision hits the list.  If you want to keep
the number of revisions low, v8 shouln't have already been sent.

I desire to review *and test* all these patches (which I believe is in
everyone's best interest) and have not initially been able to do so as
all these efforts come out of my free time, which is sometimes limited
of has been used to finalize the INTF TE series (which is a dependency
of these series).

It should be pretty obvious to see that this patch has not had a reply
from me on any of the previous revisions, and there are more patches
that I have not been able to comment on yet.

The alternative to that is of course not seeing enough value in my
review and testing (or ""late"" comments...) and merging it regardless
at some point, but that's not up to me to decide.

- Marijn

> 
> [1] 
> https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
> 
> > 
> >> +}
> >> +
> >> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
> >> +{
> >> +	u32 bytes_per_soft_slice;
> >> +	s64 bytes_per_soft_slice_fp;
> >> +	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
> >> +
> >> +	bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
> >> +	bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
> >> +
> >> +	return bytes_per_soft_slice * slice_per_intf;
> >> +}
> >> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
> >> new file mode 100644
> >> index 000000000000..38f3651d0b79
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
> >> @@ -0,0 +1,69 @@
> >> +/* 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 <linux/bug.h>
> >> +#include <linux/math.h>
> >> +#include <drm/display/drm_dsc_helper.h>
> >> +
> >> +/*
> >> + * Helper methods for MSM specific DSC calculations that are common between timing engine,
> >> + * DSI, and DP.
> >> + */
> >> +
> >> +/**
> >> + * msm_dsc_get_bpp_int - get bits per pixel integer value
> > 
> > For all function docs, don't forget the trailing parenthesis after the
> > function name: msm_dsc_get_bpp_int()
> > 
> > https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
> 
> Acked.
> 
> > 
> >> + * @dsc: Pointer to drm dsc config struct
> >> + * Returns: BPP integer value
> >> + */
> >> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
> >> +{
> >> +	WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
> >> +	return dsc->bits_per_pixel >> 4;
> >> +}
> >> +
> >> +/**
> >> + * msm_dsc_get_slice_per_intf - get number of slices per interface
> >> + * @dsc: Pointer to drm dsc config struct
> >> + * @intf_width: interface width
> >> + * Returns: Integer representing the slice per interface
> >> + */
> >> +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);
> > 
> > Looks good.
> > 
> >> +}
> >> +
> >> +/**
> >> + * msm_dsc_get_bytes_per_line - Calculate bytes per line
> >> + * @dsc: Pointer to drm dsc config struct
> >> + * Returns: Integer value representing pclk per interface
> >> + *
> >> + * Note: This value will then be passed along to DSI and DP for some more
> >> + * calculations. This is because DSI and DP divide the pclk_per_intf value
> >> + * by different values depending on if widebus is enabled.
> >> + */
> >> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
> >> +{
> >> +	return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * msm_dsc_get_bpp_int(dsc), 8);
> > 
> > dsc->slice_chunk_size * dsc->slice_count?
> 
> Acked.
> 
> > 
> >> +}
> >> +
> >> +/**
> >> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
> > 
> > Explain to the reader what a "soft" slice is?
> 
> A soft slice is a slice defined in software as opposed to "hard slices" 
> that are defined by hardware.
> 
> Since the slice-related variables in drm_dsc_config hold information 
> related to soft slices and there is no separate variable for hard 
> slices, I'll change this name to *_get_bytes_per_slice instead.
> 
> Thanks,
> 
> Jessica Zhang
> 
> > 
> > - Marijn
> > 
> >> + * @dsc: Pointer to drm dsc config struct
> >> + * Returns: s31.32 fixed point value representing bytes per soft slice
> >> + */
> >> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
> >> +
> >> +/**
> >> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
> >> + * @dsc: Pointer to drm dsc config struct
> >> + * @intf_width: interface width
> >> + * Returns: u32 value representing bytes per interface
> >> + */
> >> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
> >> +
> >> +#endif /* MSM_DSC_HELPER_H_ */
> >>
> >> -- 
> >> 2.40.1
> >>

Re: [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Marijn Suijten]

On 2023-05-11 07:26:28, Dmitry Baryshkov wrote:
> On 11/05/2023 01:35, Jessica Zhang wrote:
> > 
> > 
> > On 5/9/2023 11:29 PM, Marijn Suijten wrote:
> >> On 2023-05-09 15:06:48, Jessica Zhang wrote:
> >>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>
> >>> Add a helper setting config values which are typically constant across
> >>> operating modes (table E-4 of the standard) and mux_word_size (which is
> >>> a const according to 3.5.2).
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>
> >> Same question about ordering.
> > 
> > Hi Marijn,
> > 
> > This patch was authored by Dmitry and originally part of his DRM DSC 
> > helpers series [1], but was removed from that series for mergeability 
> > reasons.
> > 
> > Looking over the kernel documentation, the last Signed-off-by should be 
> > from the patch submitter [2], so I think my s-o-b tag should be at the 
> > bottom.

That's true, but I also think the S-o-B at the top should match the
 From: author.

> > As for the order in the previous patch, I can add a duplicate s-o-b 
> > before Dmitry's so that it reflects the history of the patch.
> 
> I think this is an overkill. Instead you can drop my SOB from the patch 
> 1. We do not need this level of detail.
> 
> For this patch the ordering of tags is correct.

So indeed, that either means duplicating the S-o-B or dropping it
entirely as we do not care that it was part of that series earlier.
Dmitry will likely sign this off once again when picking the patches.

- Marijn

Re: [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Dmitry Baryshkov]

On 11/05/2023 09:18, Marijn Suijten wrote:
> On 2023-05-11 07:26:28, Dmitry Baryshkov wrote:
>> On 11/05/2023 01:35, Jessica Zhang wrote:
>>>
>>>
>>> On 5/9/2023 11:29 PM, Marijn Suijten wrote:
>>>> On 2023-05-09 15:06:48, Jessica Zhang wrote:
>>>>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>
>>>>> Add a helper setting config values which are typically constant across
>>>>> operating modes (table E-4 of the standard) and mux_word_size (which is
>>>>> a const according to 3.5.2).
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>
>>>> Same question about ordering.
>>>
>>> Hi Marijn,
>>>
>>> This patch was authored by Dmitry and originally part of his DRM DSC
>>> helpers series [1], but was removed from that series for mergeability
>>> reasons.
>>>
>>> Looking over the kernel documentation, the last Signed-off-by should be
>>> from the patch submitter [2], so I think my s-o-b tag should be at the
>>> bottom.
> 
> That's true, but I also think the S-o-B at the top should match the
>   From: author.

I'd say, this is usual, but not the required order of tags.

> 
>>> As for the order in the previous patch, I can add a duplicate s-o-b
>>> before Dmitry's so that it reflects the history of the patch.
>>
>> I think this is an overkill. Instead you can drop my SOB from the patch
>> 1. We do not need this level of detail.
>>
>> For this patch the ordering of tags is correct.
> 
> So indeed, that either means duplicating the S-o-B or dropping it
> entirely as we do not care that it was part of that series earlier.
> Dmitry will likely sign this off once again when picking the patches.

Yes.

I'd suggest the following tags:

Patch 1 (Add flatness...):
From: Jessica
SOB: Jessica

Patches 2 (add helper to set) and 3 (use DRM DSC helpers):
From: Dmitry
SOB: Dmitry
SOB: Jessica


> 
> - Marijn

-- 
With best wishes
Dmitry

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Dmitry Baryshkov]

On 11/05/2023 00:03, Jessica Zhang wrote:
> 
> 
> On 5/9/2023 11:33 PM, Marijn Suijten wrote:
>> On 2023-05-09 15:06:50, Jessica Zhang wrote:
>>> Introduce MSM-specific DSC helper methods, as some calculations are
>>> common between DP and DSC.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/Makefile         |  1 +
>>>   drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
>>>   drivers/gpu/drm/msm/msm_dsc_helper.h | 69 
>>> ++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 96 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>>> index 7274c41228ed..b814fc80e2d5 100644
>>> --- a/drivers/gpu/drm/msm/Makefile
>>> +++ b/drivers/gpu/drm/msm/Makefile
>>> @@ -94,6 +94,7 @@ msm-y += \
>>>       msm_atomic_tracepoints.o \
>>>       msm_debugfs.o \
>>>       msm_drv.o \
>>> +    msm_dsc_helper.o \
>>>       msm_fb.o \
>>>       msm_fence.o \
>>>       msm_gem.o \
>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c 
>>> b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>> new file mode 100644
>>> index 000000000000..29feb3e3b5a4
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>> @@ -0,0 +1,26 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>> reserved
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <drm/drm_fixed.h>
>>> +
>>> +#include "msm_dsc_helper.h"
>>> +
>>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
>>> +{
>>> +    return drm_fixp_from_fraction(dsc->slice_width * 
>>> msm_dsc_get_bpp_int(dsc), 8);
>>
>> How about using dsc->slice_chunk_size?
> 
> Hi Marijn,
> 
> Thanks for pointing this out. However, I would prefer to keep this fixed 
> point version of the slice_chunk_size math as the downstream DP math 
> also uses fixed point [1].

This is pretty weak argument. Especially since this particular piece of 
code does lots of wrong or inefficient things.

> 
> If we are able to confirm that integer math also works for DP, we will 
> make the change to use slice_chunk_size within the DP DSC series.

This is why we usually do not accept API-only series. It is next to 
imposible to judge if the API is good enough without the actual users.

> 
> I also want to note that this math has stayed the same throughout all 7 
> revisions. In the interest of making review more efficient, I think it 
> would be helpful to point out important details like this early on in 
> the process. That way we can address major concerns early on and keep 
> the number of revisions per series low.

This is not always possible. We grasp the details of the patchset as we 
review and dive into the patchset under the review and other close 
enough patches/commits. So it is infrequent but still valid when at some 
point a reviewer (or the author) would come up with the comments 
demanding significant changes to the patch.

> 
> [1] 
> https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
> 
>>
>>> +}
>>> +
>>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int 
>>> intf_width)
>>> +{
>>> +    u32 bytes_per_soft_slice;
>>> +    s64 bytes_per_soft_slice_fp;
>>> +    int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
>>> +
>>> +    bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
>>> +    bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
>>> +
>>> +    return bytes_per_soft_slice * slice_per_intf;
>>> +}
>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h 
>>> b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>> new file mode 100644
>>> index 000000000000..38f3651d0b79
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>> @@ -0,0 +1,69 @@
>>> +/* 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 <linux/bug.h>
>>> +#include <linux/math.h>
>>> +#include <drm/display/drm_dsc_helper.h>
>>> +
>>> +/*
>>> + * Helper methods for MSM specific DSC calculations that are common 
>>> between timing engine,
>>> + * DSI, and DP.
>>> + */
>>> +
>>> +/**
>>> + * msm_dsc_get_bpp_int - get bits per pixel integer value
>>
>> For all function docs, don't forget the trailing parenthesis after the
>> function name: msm_dsc_get_bpp_int()
>>
>> https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
> 
> Acked.
> 
>>
>>> + * @dsc: Pointer to drm dsc config struct
>>> + * Returns: BPP integer value
>>> + */
>>> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
>>> +{
>>> +    WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
>>> +    return dsc->bits_per_pixel >> 4;
>>> +}
>>> +
>>> +/**
>>> + * msm_dsc_get_slice_per_intf - get number of slices per interface
>>> + * @dsc: Pointer to drm dsc config struct
>>> + * @intf_width: interface width
>>> + * Returns: Integer representing the slice per interface
>>> + */
>>> +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);
>>
>> Looks good.
>>
>>> +}
>>> +
>>> +/**
>>> + * msm_dsc_get_bytes_per_line - Calculate bytes per line
>>> + * @dsc: Pointer to drm dsc config struct
>>> + * Returns: Integer value representing pclk per interface
>>> + *
>>> + * Note: This value will then be passed along to DSI and DP for some 
>>> more
>>> + * calculations. This is because DSI and DP divide the pclk_per_intf 
>>> value
>>> + * by different values depending on if widebus is enabled.
>>> + */
>>> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config 
>>> *dsc)
>>> +{
>>> +    return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * 
>>> msm_dsc_get_bpp_int(dsc), 8);
>>
>> dsc->slice_chunk_size * dsc->slice_count?
> 
> Acked.
> 
>>
>>> +}
>>> +
>>> +/**
>>> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice 
>>> for dsc
>>
>> Explain to the reader what a "soft" slice is?
> 
> A soft slice is a slice defined in software as opposed to "hard slices" 
> that are defined by hardware.
> 
> Since the slice-related variables in drm_dsc_config hold information 
> related to soft slices and there is no separate variable for hard 
> slices, I'll change this name to *_get_bytes_per_slice instead.
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>> - Marijn
>>
>>> + * @dsc: Pointer to drm dsc config struct
>>> + * Returns: s31.32 fixed point value representing bytes per soft slice
>>> + */
>>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
>>> +
>>> +/**
>>> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
>>> + * @dsc: Pointer to drm dsc config struct
>>> + * @intf_width: interface width
>>> + * Returns: u32 value representing bytes per interface
>>> + */
>>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int 
>>> intf_width);
>>> +
>>> +#endif /* MSM_DSC_HELPER_H_ */
>>>
>>> -- 
>>> 2.40.1
>>>

-- 
With best wishes
Dmitry

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Abhinav Kumar]

On 5/10/2023 11:28 PM, Dmitry Baryshkov wrote:
> On 11/05/2023 00:03, Jessica Zhang wrote:
>>
>>
>> On 5/9/2023 11:33 PM, Marijn Suijten wrote:
>>> On 2023-05-09 15:06:50, Jessica Zhang wrote:
>>>> Introduce MSM-specific DSC helper methods, as some calculations are
>>>> common between DP and DSC.
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/Makefile         |  1 +
>>>>   drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
>>>>   drivers/gpu/drm/msm/msm_dsc_helper.h | 69 
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 96 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/Makefile 
>>>> b/drivers/gpu/drm/msm/Makefile
>>>> index 7274c41228ed..b814fc80e2d5 100644
>>>> --- a/drivers/gpu/drm/msm/Makefile
>>>> +++ b/drivers/gpu/drm/msm/Makefile
>>>> @@ -94,6 +94,7 @@ msm-y += \
>>>>       msm_atomic_tracepoints.o \
>>>>       msm_debugfs.o \
>>>>       msm_drv.o \
>>>> +    msm_dsc_helper.o \
>>>>       msm_fb.o \
>>>>       msm_fence.o \
>>>>       msm_gem.o \
>>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c 
>>>> b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>>> new file mode 100644
>>>> index 000000000000..29feb3e3b5a4
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>>> @@ -0,0 +1,26 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights 
>>>> reserved
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <drm/drm_fixed.h>
>>>> +
>>>> +#include "msm_dsc_helper.h"
>>>> +
>>>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
>>>> +{
>>>> +    return drm_fixp_from_fraction(dsc->slice_width * 
>>>> msm_dsc_get_bpp_int(dsc), 8);
>>>
>>> How about using dsc->slice_chunk_size?
>>
>> Hi Marijn,
>>
>> Thanks for pointing this out. However, I would prefer to keep this 
>> fixed point version of the slice_chunk_size math as the downstream DP 
>> math also uses fixed point [1].
> 
> This is pretty weak argument. Especially since this particular piece of 
> code does lots of wrong or inefficient things.
> 

I guess Jessica's concern was we will have to revalidate some bpc/bpp 
and slice combinations again for DP. So its a question of keeping this 
helper for now and dropping it if unnecessary later or dropping it now 
itself. If the consensus is going to be to drop this, we will just 
re-test the monitors we have and do it.

>>
>> If we are able to confirm that integer math also works for DP, we will 
>> make the change to use slice_chunk_size within the DP DSC series.
> 
> This is why we usually do not accept API-only series. It is next to 
> imposible to judge if the API is good enough without the actual users.
> 
>>
>> I also want to note that this math has stayed the same throughout all 
>> 7 revisions. In the interest of making review more efficient, I think 
>> it would be helpful to point out important details like this early on 
>> in the process. That way we can address major concerns early on and 
>> keep the number of revisions per series low.
> 
> This is not always possible. We grasp the details of the patchset as we 
> review and dive into the patchset under the review and other close 
> enough patches/commits. So it is infrequent but still valid when at some 
> point a reviewer (or the author) would come up with the comments 
> demanding significant changes to the patch.
> 

I dont think Jessica is really complaining about multiple reviewers. Its 
a question of when those reviews are coming. Not reviewing revisions 1-7 
and starting the reviews at revision 8 (that essentially becomes 
revision 1 now) , is going to make this go on forever.

So its a bit frustrating from the developers point of view to just 
ignore revisions 1-7 and start at rev 8.

>>
>> [1] 
>> https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335 
>>
>>
>>>
>>>> +}
>>>> +
>>>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int 
>>>> intf_width)
>>>> +{
>>>> +    u32 bytes_per_soft_slice;
>>>> +    s64 bytes_per_soft_slice_fp;
>>>> +    int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
>>>> +
>>>> +    bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
>>>> +    bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
>>>> +
>>>> +    return bytes_per_soft_slice * slice_per_intf;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h 
>>>> b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>>> new file mode 100644
>>>> index 000000000000..38f3651d0b79
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>>> @@ -0,0 +1,69 @@
>>>> +/* 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 <linux/bug.h>
>>>> +#include <linux/math.h>
>>>> +#include <drm/display/drm_dsc_helper.h>
>>>> +
>>>> +/*
>>>> + * Helper methods for MSM specific DSC calculations that are common 
>>>> between timing engine,
>>>> + * DSI, and DP.
>>>> + */
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bpp_int - get bits per pixel integer value
>>>
>>> For all function docs, don't forget the trailing parenthesis after the
>>> function name: msm_dsc_get_bpp_int()
>>>
>>> https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation 
>>>
>>
>> Acked.
>>
>>>
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * Returns: BPP integer value
>>>> + */
>>>> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
>>>> +{
>>>> +    WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
>>>> +    return dsc->bits_per_pixel >> 4;
>>>> +}
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_slice_per_intf - get number of slices per interface
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * @intf_width: interface width
>>>> + * Returns: Integer representing the slice per interface
>>>> + */
>>>> +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);
>>>
>>> Looks good.
>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bytes_per_line - Calculate bytes per line
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * Returns: Integer value representing pclk per interface
>>>> + *
>>>> + * Note: This value will then be passed along to DSI and DP for 
>>>> some more
>>>> + * calculations. This is because DSI and DP divide the 
>>>> pclk_per_intf value
>>>> + * by different values depending on if widebus is enabled.
>>>> + */
>>>> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config 
>>>> *dsc)
>>>> +{
>>>> +    return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * 
>>>> msm_dsc_get_bpp_int(dsc), 8);
>>>
>>> dsc->slice_chunk_size * dsc->slice_count?
>>
>> Acked.
>>
>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice 
>>>> for dsc
>>>
>>> Explain to the reader what a "soft" slice is?
>>
>> A soft slice is a slice defined in software as opposed to "hard 
>> slices" that are defined by hardware.
>>
>> Since the slice-related variables in drm_dsc_config hold information 
>> related to soft slices and there is no separate variable for hard 
>> slices, I'll change this name to *_get_bytes_per_slice instead.
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> - Marijn
>>>
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * Returns: s31.32 fixed point value representing bytes per soft slice
>>>> + */
>>>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * @intf_width: interface width
>>>> + * Returns: u32 value representing bytes per interface
>>>> + */
>>>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int 
>>>> intf_width);
>>>> +
>>>> +#endif /* MSM_DSC_HELPER_H_ */
>>>>
>>>> -- 
>>>> 2.40.1
>>>>
> 

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Abhinav Kumar]

On 5/10/2023 11:15 PM, Marijn Suijten wrote:
> On 2023-05-10 14:03:14, Jessica Zhang wrote:
>>
>>
>> On 5/9/2023 11:33 PM, Marijn Suijten wrote:
>>> On 2023-05-09 15:06:50, Jessica Zhang wrote:
>>>> Introduce MSM-specific DSC helper methods, as some calculations are
>>>> common between DP and DSC.
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/Makefile         |  1 +
>>>>    drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++++++++++++++
>>>>    drivers/gpu/drm/msm/msm_dsc_helper.h | 69 ++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 96 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>>>> index 7274c41228ed..b814fc80e2d5 100644
>>>> --- a/drivers/gpu/drm/msm/Makefile
>>>> +++ b/drivers/gpu/drm/msm/Makefile
>>>> @@ -94,6 +94,7 @@ msm-y += \
>>>>    	msm_atomic_tracepoints.o \
>>>>    	msm_debugfs.o \
>>>>    	msm_drv.o \
>>>> +	msm_dsc_helper.o \
>>>>    	msm_fb.o \
>>>>    	msm_fence.o \
>>>>    	msm_gem.o \
>>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>>> new file mode 100644
>>>> index 000000000000..29feb3e3b5a4
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
>>>> @@ -0,0 +1,26 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <drm/drm_fixed.h>
>>>> +
>>>> +#include "msm_dsc_helper.h"
>>>> +
>>>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
>>>> +{
>>>> +	return drm_fixp_from_fraction(dsc->slice_width * msm_dsc_get_bpp_int(dsc), 8);
>>>
>>> How about using dsc->slice_chunk_size?
>>
>> Hi Marijn,
>>
>> Thanks for pointing this out. However, I would prefer to keep this fixed
>> point version of the slice_chunk_size math as the downstream DP math
>> also uses fixed point [1].
> 
> That looks like a totally different computation.  For this specific
> value it appears drm_fixp_from_fraction does rounding whereas
> DIV_ROUND_UP always rounds up when used for slice_chunk_size, but these
> values are generally multiples of 8.  slice_chunk_size will also take
> fractional bpp into account (even if unsupported by DPU otherwise).
> 
>> If we are able to confirm that integer math also works for DP, we will
>> make the change to use slice_chunk_size within the DP DSC series.
> 
> Don't think the DP series alone should point that out, as it heavily
> depends on the relation between slice size and bpp parameters, and
> whether those end up with a fractional part or not (are you able to test
> and confirm all those combinations?).  Regardless it seems more natural
> to use slice_chunk_size which is used in various other ways and sent in
> the PPS: it wouldn't make sense to adhere to a different slice byte
> count elsewhere.
> 

They are not totally different.

I guess what Jessica is trying to say here is we will have to do more 
validation with slice/bpp combinations for DP to conclude whether there 
is a difference in math. We can go with two approaches:

1) Either go with slice_chunk_size for now and re-validate with the 
monitors we have and drop this particular helper
2) Keep this particular helper for now and upon more validation if we 
see it's same across more combinations, drop this later.

I just have a slight preference for (2), but looks like your preference 
is for (1) but this is just more validation work for us which is fine I 
guess this time since we will revalidate DP again anyway.

>> I also want to note that this math has stayed the same throughout all 7
>> revisions. In the interest of making review more efficient, I think it
>> would be helpful to point out important details like this early on in
>> the process. That way we can address major concerns early on and keep
>> the number of revisions per series low.
> 
> I've already expressed to Abhinav and you that the revisions for all
> these series were coming in way too hot, leaving no time for review and
> discussions before the next revision hits the list.  If you want to keep
> the number of revisions low, v8 shouln't have already been sent.
> 

You had the concern for DSC 1.2 DPU series from kuogee. So to respect 
that we held that back the entire end of last week and early this week 
(~6 days) and posted today.

Now, you have the same concern with this series from jessica. Sorry, as 
much as i would like to get your feedback on every series, I cannot hold 
back every QC display developer to wait for your reviews on patch rev 1 
before posting patch rev 2. They all work mostly independently. So if 
one reviewer say dmitry has reviewed one revision and we agreed on the 
comments, in the absence of another comment from another reviewer, the 
developer is going to post the next revision for more reviews as its 
more efficient to manage their time as they are in the same context. 
This is nothing unusual from normal upstream development.

If by the time you start reviewing its going to be revision 7 or 8, then 
start it as a fresh review which it is for you, folks are obviously 
going to question why you didnt review revisions 1-7 so far. So I cannot 
take sides on this.

I wish I could help more to accommodate your schedules but its hard to 
control who pushes when with distributed development and tell each of 
them to hold back.

> I desire to review *and test* all these patches (which I believe is in
> everyone's best interest) and have not initially been able to do so as
> all these efforts come out of my free time, which is sometimes limited
> of has been used to finalize the INTF TE series (which is a dependency
> of these series).
> 
> It should be pretty obvious to see that this patch has not had a reply
> from me on any of the previous revisions, and there are more patches
> that I have not been able to comment on yet.
> 
> The alternative to that is of course not seeing enough value in my
> review and testing (or ""late"" comments...) and merging it regardless
> at some point, but that's not up to me to decide.
> 
> - Marijn

Thats true even for mine or other's reviews :) that if i am not able to 
get to some patches on time and other reviewers are happy with it they 
do get landed in the tree. Has happened many times and I have not 
complained. I just fix the issues on top of them (which again happens a 
lot) and move on.

So there doesnt seem to be any rule as such today that wait for this 
particular developer to review and validate.

> 
>>
>> [1]
>> https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
>>
>>>
>>>> +}
>>>> +
>>>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
>>>> +{
>>>> +	u32 bytes_per_soft_slice;
>>>> +	s64 bytes_per_soft_slice_fp;
>>>> +	int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
>>>> +
>>>> +	bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
>>>> +	bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
>>>> +
>>>> +	return bytes_per_soft_slice * slice_per_intf;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>>> new file mode 100644
>>>> index 000000000000..38f3651d0b79
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
>>>> @@ -0,0 +1,69 @@
>>>> +/* 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 <linux/bug.h>
>>>> +#include <linux/math.h>
>>>> +#include <drm/display/drm_dsc_helper.h>
>>>> +
>>>> +/*
>>>> + * Helper methods for MSM specific DSC calculations that are common between timing engine,
>>>> + * DSI, and DP.
>>>> + */
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bpp_int - get bits per pixel integer value
>>>
>>> For all function docs, don't forget the trailing parenthesis after the
>>> function name: msm_dsc_get_bpp_int()
>>>
>>> https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
>>
>> Acked.
>>
>>>
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * Returns: BPP integer value
>>>> + */
>>>> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
>>>> +{
>>>> +	WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
>>>> +	return dsc->bits_per_pixel >> 4;
>>>> +}
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_slice_per_intf - get number of slices per interface
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * @intf_width: interface width
>>>> + * Returns: Integer representing the slice per interface
>>>> + */
>>>> +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);
>>>
>>> Looks good.
>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bytes_per_line - Calculate bytes per line
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * Returns: Integer value representing pclk per interface
>>>> + *
>>>> + * Note: This value will then be passed along to DSI and DP for some more
>>>> + * calculations. This is because DSI and DP divide the pclk_per_intf value
>>>> + * by different values depending on if widebus is enabled.
>>>> + */
>>>> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
>>>> +{
>>>> +	return DIV_ROUND_UP(dsc->slice_width * dsc->slice_count * msm_dsc_get_bpp_int(dsc), 8);
>>>
>>> dsc->slice_chunk_size * dsc->slice_count?
>>
>> Acked.
>>
>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
>>>
>>> Explain to the reader what a "soft" slice is?
>>
>> A soft slice is a slice defined in software as opposed to "hard slices"
>> that are defined by hardware.
>>
>> Since the slice-related variables in drm_dsc_config hold information
>> related to soft slices and there is no separate variable for hard
>> slices, I'll change this name to *_get_bytes_per_slice instead.
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> - Marijn
>>>
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * Returns: s31.32 fixed point value representing bytes per soft slice
>>>> + */
>>>> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
>>>> +
>>>> +/**
>>>> + * msm_dsc_get_bytes_per_intf - get total bytes per interface
>>>> + * @dsc: Pointer to drm dsc config struct
>>>> + * @intf_width: interface width
>>>> + * Returns: u32 value representing bytes per interface
>>>> + */
>>>> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
>>>> +
>>>> +#endif /* MSM_DSC_HELPER_H_ */
>>>>
>>>> -- 
>>>> 2.40.1
>>>>

Re: [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods

[Marijn Suijten]

On 2023-05-11 13:05:15, Abhinav Kumar wrote:
<snip>
> > Don't think the DP series alone should point that out, as it heavily
> > depends on the relation between slice size and bpp parameters, and
> > whether those end up with a fractional part or not (are you able to test
> > and confirm all those combinations?).  Regardless it seems more natural
> > to use slice_chunk_size which is used in various other ways and sent in
> > the PPS: it wouldn't make sense to adhere to a different slice byte
> > count elsewhere.
> > 
> 
> They are not totally different.
> 
> I guess what Jessica is trying to say here is we will have to do more 
> validation with slice/bpp combinations for DP to conclude whether there 
> is a difference in math. We can go with two approaches:
> 
> 1) Either go with slice_chunk_size for now and re-validate with the 
> monitors we have and drop this particular helper
> 2) Keep this particular helper for now and upon more validation if we 
> see it's same across more combinations, drop this later.

I would argue that if we need a different rounding strategy on the
number of bytes within a slice, shouldn't the DRM helper calculating
slice_chunk_size be updated as well?  This sounds/feels like a thing
that is specified in the DSC docs, as all the parameters involved are
part of the DSC spec (but I am not currently in a position to review
that within the coming few days).

> I just have a slight preference for (2), but looks like your preference 
> is for (1) but this is just more validation work for us which is fine I 
> guess this time since we will revalidate DP again anyway.

Thanks, that's appreciated, but do make sure to test the cases where the
rounding method actually makes a difference in the resulting value.

> >> I also want to note that this math has stayed the same throughout all 7
> >> revisions. In the interest of making review more efficient, I think it
> >> would be helpful to point out important details like this early on in
> >> the process. That way we can address major concerns early on and keep
> >> the number of revisions per series low.
> > 
> > I've already expressed to Abhinav and you that the revisions for all
> > these series were coming in way too hot, leaving no time for review and
> > discussions before the next revision hits the list.  If you want to keep
> > the number of revisions low, v8 shouln't have already been sent.
> > 
> 
> You had the concern for DSC 1.2 DPU series from kuogee. So to respect 
> that we held that back the entire end of last week and early this week 
> (~6 days) and posted today.
> 
> Now, you have the same concern with this series from jessica. Sorry, as 
> much as i would like to get your feedback on every series, I cannot hold 
> back every QC display developer to wait for your reviews on patch rev 1 
> before posting patch rev 2. They all work mostly independently. So if 
> one reviewer say dmitry has reviewed one revision and we agreed on the 
> comments, in the absence of another comment from another reviewer, the 
> developer is going to post the next revision for more reviews as its 
> more efficient to manage their time as they are in the same context. 
> This is nothing unusual from normal upstream development.

I am not saying that it is unusual to send many revisions (though in
"quick succession", which is only loosely defined).  Just that it is
unusual to "demand" reviews (which are seemingly perceived as requesting
a lot, even though I don't think I'm asking anything outrageous here) to
stop when the revisions reach a certain number, or to have happened
early on in the series.  I understand it's annoying from a developer
perspective, but sometimes it is what it is.

This whole process would be better if there's a more explicit list of
"these specific people have to test, review and sign off on every patch"
before this series is ready to be merged, but I understand that people
do not want to commit to that from both sides.

> If by the time you start reviewing its going to be revision 7 or 8, then 
> start it as a fresh review which it is for you, folks are obviously 
> going to question why you didnt review revisions 1-7 so far. So I cannot 
> take sides on this.

Fine to question it, and my answer is: besides having a hard time
finding enough free time to look at this, I also want to test it by
piecing together all the various series, and besides regression-testing
on sdm845 even managed to use these series in a very positive way to
finally (albeit with some additional fixes) get working DSC and panels
on my SM8150 and SM8250 boards.  Meaning I have even more hardware and
configurations to test, valudate, **and provide feedback** on, taking
away from the review time initially.

> I wish I could help more to accommodate your schedules but its hard to 
> control who pushes when with distributed development and tell each of 
> them to hold back.
> 
> > I desire to review *and test* all these patches (which I believe is in
> > everyone's best interest) and have not initially been able to do so as
> > all these efforts come out of my free time, which is sometimes limited
> > of has been used to finalize the INTF TE series (which is a dependency
> > of these series).
> > 
> > It should be pretty obvious to see that this patch has not had a reply
> > from me on any of the previous revisions, and there are more patches
> > that I have not been able to comment on yet.
> > 
> > The alternative to that is of course not seeing enough value in my
> > review and testing (or ""late"" comments...) and merging it regardless
> > at some point, but that's not up to me to decide.
> > 
> > - Marijn
> 
> Thats true even for mine or other's reviews :) that if i am not able to 
> get to some patches on time and other reviewers are happy with it they 
> do get landed in the tree. Has happened many times and I have not 
> complained. I just fix the issues on top of them (which again happens a 
> lot) and move on.

That is totally fine, but in this case the series is not merged yet (nor
seems to happen within the next couple of hours) so I rather share
findings rather than keeping them on a private list to send
cleanups/fixes days after it lands.  Especially in the event that this
takes a few more revisions.  In other words, I understand that my review
gets rightfully ignored/dropped if the maintainer has already picked or
is about to pick it.

> So there doesnt seem to be any rule as such today that wait for this 
> particular developer to review and validate.

Exactly.  As mentioned briefly above this process would involve
committing to a fixed list of reviewers to at least have r-b'd or
replied to every patch, but (as also said above) I can see from both
sides that might not be desirable to commit to.

Thanks for the patience and cooperation thus far!  I'll try to give this
another review- and testing round over the weekend.

- Marijn

<snip>

Re: [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters

[Marijn Suijten]

On 2023-05-11 09:22:47, Dmitry Baryshkov wrote:
> On 11/05/2023 09:18, Marijn Suijten wrote:
> > On 2023-05-11 07:26:28, Dmitry Baryshkov wrote:
> >> On 11/05/2023 01:35, Jessica Zhang wrote:
> >>>
> >>>
> >>> On 5/9/2023 11:29 PM, Marijn Suijten wrote:
> >>>> On 2023-05-09 15:06:48, Jessica Zhang wrote:
> >>>>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>
> >>>>> Add a helper setting config values which are typically constant across
> >>>>> operating modes (table E-4 of the standard) and mux_word_size (which is
> >>>>> a const according to 3.5.2).
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>>
> >>>> Same question about ordering.
> >>>
> >>> Hi Marijn,
> >>>
> >>> This patch was authored by Dmitry and originally part of his DRM DSC
> >>> helpers series [1], but was removed from that series for mergeability
> >>> reasons.
> >>>
> >>> Looking over the kernel documentation, the last Signed-off-by should be
> >>> from the patch submitter [2], so I think my s-o-b tag should be at the
> >>> bottom.
> > 
> > That's true, but I also think the S-o-B at the top should match the
> >   From: author.
> 
> I'd say, this is usual, but not the required order of tags.
> 
> > 
> >>> As for the order in the previous patch, I can add a duplicate s-o-b
> >>> before Dmitry's so that it reflects the history of the patch.
> >>
> >> I think this is an overkill. Instead you can drop my SOB from the patch
> >> 1. We do not need this level of detail.
> >>
> >> For this patch the ordering of tags is correct.
> > 
> > So indeed, that either means duplicating the S-o-B or dropping it
> > entirely as we do not care that it was part of that series earlier.
> > Dmitry will likely sign this off once again when picking the patches.
> 
> Yes.
> 
> I'd suggest the following tags:
> 
> Patch 1 (Add flatness...):
> From: Jessica
> SOB: Jessica
> 
> Patches 2 (add helper to set) and 3 (use DRM DSC helpers):
> From: Dmitry
> SOB: Dmitry
> SOB: Jessica

Both sound exactly right, as we do not care about that fact that the
first patch was temporarily picked up by you in another series, and then
dropped again.

- Marijn