[PATCH 00/13] DSC misc fixes

[PATCH 00/13] DSC misc fixes

[Ankit Nautiyal]

This series is an attempt to address multiple issues with DSC,
scattered in separate existing series.

Patches 1-3 are DSC fixes from series to Handle BPC for HDMI2.1 PCON
https://patchwork.freedesktop.org/series/107550/

Patches 4-5 are from series DSC fixes for Bigjoiner:
https://patchwork.freedesktop.org/series/115773/

Patches 6-12 are from series to add DSC fractional BPP support:
https://patchwork.freedesktop.org/series/111391/

Patch 13 is to fix compressed bpc for MST DSC, from Stan's series :
https://patchwork.freedesktop.org/series/116179/

Ankit Nautiyal (12):
  drm/i915/dp: Consider output_format while computing dsc bpp
  drm/i915/dp_mst: Use output_format to get the final link bpp
  drm/i915/dp: Use consistent name for link bpp and compressed bpp
  drm/i915/dp: Update Bigjoiner interface bits for computing compressed
    bpp
  drm/i915/intel_cdclk: Add vdsc with bigjoiner constraints on min_cdlck
  drm/i915/dp: Remove extra logs for printing DSC info
  drm/display/dp: Fix the DP DSC Receiver cap size
  drm/i915/dp: Avoid forcing DSC BPC for MST case
  drm/i915/dp: Check min bpc DSC limits for dsc_force_bpc also
  drm/i915/dp: Avoid left shift of DSC output bpp by 4
  drm/i915/dp: Rename helpers to get DSC max pipe_bpp/output_bpp
  drm/i915/dp: Get optimal link config to have best compressed bpp

Stanislav Lisovskiy (1):
  drm/i915: Query compressed bpp properly using correct DPCD and DP Spec
    info

 drivers/gpu/drm/i915/display/intel_cdclk.c  |  49 ++-
 drivers/gpu/drm/i915/display/intel_dp.c     | 408 +++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_dp.h     |  20 +-
 drivers/gpu/drm/i915/display/intel_dp_mst.c |  76 ++--
 include/drm/display/drm_dp.h                |   2 +-
 5 files changed, 401 insertions(+), 154 deletions(-)

-- 
2.25.1

[PATCH 01/13] drm/i915/dp: Consider output_format while computing dsc bpp

[Ankit Nautiyal]

While using DSC the compressed bpp is computed assuming RGB output
format. Consider the output_format and compute the compressed bpp
during mode valid and compute config steps.

For DP-MST we currently use RGB output format only, so continue
using RGB while computing compressed bpp for MST case.

v2: Use output_bpp instead for pipe_bpp to clamp compressed_bpp. (Ville)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 19 +++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp.h     |  1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c |  1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 0cc57681dc4d..263c30948117 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -734,6 +734,7 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 				u32 link_clock, u32 lane_count,
 				u32 mode_clock, u32 mode_hdisplay,
 				bool bigjoiner,
+				enum intel_output_format output_format,
 				u32 pipe_bpp,
 				u32 timeslots)
 {
@@ -758,6 +759,10 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 	bits_per_pixel = ((link_clock * lane_count) * timeslots) /
 			 (intel_dp_mode_to_fec_clock(mode_clock) * 8);
 
+	/* Bandwidth required for 420 is half, that of 444 format */
+	if (output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
+		bits_per_pixel *= 2;
+
 	drm_dbg_kms(&i915->drm, "Max link bpp is %u for %u timeslots "
 				"total bw %u pixel clock %u\n",
 				bits_per_pixel, timeslots,
@@ -1151,11 +1156,16 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 
 	if (HAS_DSC(dev_priv) &&
 	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
+		enum intel_output_format sink_format, output_format;
+		int pipe_bpp;
+
+		sink_format = intel_dp_sink_format(connector, mode);
+		output_format = intel_dp_output_format(connector, sink_format);
 		/*
 		 * TBD pass the connector BPC,
 		 * for now U8_MAX so that max BPC on that platform would be picked
 		 */
-		int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
+		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
 
 		/*
 		 * Output bpp is stored in 6.4 format so right shift by 4 to get the
@@ -1175,6 +1185,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 							    target_clock,
 							    mode->hdisplay,
 							    bigjoiner,
+							    output_format,
 							    pipe_bpp, 64) >> 4;
 			dsc_slice_count =
 				intel_dp_dsc_get_slice_count(intel_dp,
@@ -1707,6 +1718,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 							    adjusted_mode->crtc_clock,
 							    adjusted_mode->crtc_hdisplay,
 							    pipe_config->bigjoiner_pipes,
+							    pipe_config->output_format,
 							    pipe_bpp,
 							    timeslots);
 			/*
@@ -1742,9 +1754,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 		 * calculation procedure is bit different for MST case.
 		 */
 		if (compute_pipe_bpp) {
+			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
+							     pipe_config->pipe_bpp);
+
 			pipe_config->dsc.compressed_bpp = min_t(u16,
 								dsc_max_output_bpp >> 4,
-								pipe_config->pipe_bpp);
+								output_bpp);
 		}
 		pipe_config->dsc.slice_count = dsc_dp_slice_count;
 		drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index ef39e4f7a329..db86c2b71c1f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -107,6 +107,7 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 				u32 link_clock, u32 lane_count,
 				u32 mode_clock, u32 mode_hdisplay,
 				bool bigjoiner,
+				enum intel_output_format output_format,
 				u32 pipe_bpp,
 				u32 timeslots);
 u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 63d61e610210..ee28bb89bffe 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -973,6 +973,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 							    target_clock,
 							    mode->hdisplay,
 							    bigjoiner,
+							    INTEL_OUTPUT_FORMAT_RGB,
 							    pipe_bpp, 64) >> 4;
 			dsc_slice_count =
 				intel_dp_dsc_get_slice_count(intel_dp,
-- 
2.25.1

[PATCH 02/13] drm/i915/dp_mst: Use output_format to get the final link bpp

[Ankit Nautiyal]

The final link bpp used to calculate the m_n values depend on the
output_format. Though the output_format is set to RGB for MST case and
the link bpp will be same as the pipe bpp, for the sake of semantics,
lets calculate the m_n values with the link bpp, instead of pipe_bpp.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 2 +-
 drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 ++++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 263c30948117..67d09c49de6f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -946,7 +946,7 @@ int intel_dp_min_bpp(enum intel_output_format output_format)
 		return 8 * 3;
 }
 
-static int intel_dp_output_bpp(enum intel_output_format output_format, int bpp)
+int intel_dp_output_bpp(enum intel_output_format output_format, int bpp)
 {
 	/*
 	 * bpp value was assumed to RGB format. And YCbCr 4:2:0 output
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index db86c2b71c1f..856172bfa13e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -140,5 +140,6 @@ void intel_dp_pcon_dsc_configure(struct intel_dp *intel_dp,
 void intel_dp_phy_test(struct intel_encoder *encoder);
 
 void intel_dp_wait_source_oui(struct intel_dp *intel_dp);
+int intel_dp_output_bpp(enum intel_output_format output_format, int bpp);
 
 #endif /* __INTEL_DP_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index ee28bb89bffe..2c2d258a3a77 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -155,6 +155,7 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
 	int slots = -EINVAL;
+	int link_bpp;
 
 	slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp,
 						     limits->min_bpp, limits,
@@ -163,7 +164,9 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	if (slots < 0)
 		return slots;
 
-	intel_link_compute_m_n(crtc_state->pipe_bpp,
+	link_bpp = intel_dp_output_bpp(crtc_state->output_format, crtc_state->pipe_bpp);
+
+	intel_link_compute_m_n(link_bpp,
 			       crtc_state->lane_count,
 			       adjusted_mode->crtc_clock,
 			       crtc_state->port_clock,
-- 
2.25.1

[PATCH 03/13] drm/i915/dp: Use consistent name for link bpp and compressed bpp

[Ankit Nautiyal]

Currently there are many places where we use output_bpp for link bpp and
compressed bpp.
Lets use consistent naming:
output_bpp : The intermediate value taking into account the
output_format chroma subsampling.
compressed_bpp : target bpp for the DSC encoder.
link_bpp : final bpp used in the link.

For 444 sampling without DSC:
link_bpp = output_bpp = pipe_bpp

For 420 sampling without DSC:
output_bpp = pipe_bpp / 2
link_bpp = output_bpp

For 444 sampling with DSC:
output_bpp = pipe_bpp
link_bpp = compressed_bpp, computed with output_bpp (i.e. pipe_bpp in
this case)

For 420 sampling with DSC:
output_bpp = pipe_bpp/2
link_bpp = compressed_bpp, computed with output_bpp

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 88 +++++++++++----------
 drivers/gpu/drm/i915/display/intel_dp.h     | 14 ++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++---
 3 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 67d09c49de6f..24de25551a49 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -730,13 +730,13 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
 	return bits_per_pixel;
 }
 
-u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
-				u32 link_clock, u32 lane_count,
-				u32 mode_clock, u32 mode_hdisplay,
-				bool bigjoiner,
-				enum intel_output_format output_format,
-				u32 pipe_bpp,
-				u32 timeslots)
+u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
+					u32 link_clock, u32 lane_count,
+					u32 mode_clock, u32 mode_hdisplay,
+					bool bigjoiner,
+					enum intel_output_format output_format,
+					u32 pipe_bpp,
+					u32 timeslots)
 {
 	u32 bits_per_pixel, max_bpp_small_joiner_ram;
 
@@ -1117,7 +1117,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 	int target_clock = mode->clock;
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 	int max_dotclk = dev_priv->max_dotclk_freq;
-	u16 dsc_max_output_bpp = 0;
+	u16 dsc_max_compressed_bpp = 0;
 	u8 dsc_slice_count = 0;
 	enum drm_mode_status status;
 	bool dsc = false, bigjoiner = false;
@@ -1172,21 +1172,21 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 		 * integer value since we support only integer values of bpp.
 		 */
 		if (intel_dp_is_edp(intel_dp)) {
-			dsc_max_output_bpp =
+			dsc_max_compressed_bpp =
 				drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
 			dsc_slice_count =
 				drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
 								true);
 		} else if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) {
-			dsc_max_output_bpp =
-				intel_dp_dsc_get_output_bpp(dev_priv,
-							    max_link_clock,
-							    max_lanes,
-							    target_clock,
-							    mode->hdisplay,
-							    bigjoiner,
-							    output_format,
-							    pipe_bpp, 64) >> 4;
+			dsc_max_compressed_bpp =
+				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
+								    max_link_clock,
+								    max_lanes,
+								    target_clock,
+								    mode->hdisplay,
+								    bigjoiner,
+								    output_format,
+								    pipe_bpp, 64) >> 4;
 			dsc_slice_count =
 				intel_dp_dsc_get_slice_count(intel_dp,
 							     target_clock,
@@ -1194,7 +1194,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 							     bigjoiner);
 		}
 
-		dsc = dsc_max_output_bpp && dsc_slice_count;
+		dsc = dsc_max_compressed_bpp && dsc_slice_count;
 	}
 
 	/*
@@ -1476,9 +1476,9 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
 	int mode_rate, link_rate, link_avail;
 
 	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
-		int output_bpp = intel_dp_output_bpp(pipe_config->output_format, bpp);
+		int link_bpp = intel_dp_output_bpp(pipe_config->output_format, bpp);
 
-		mode_rate = intel_dp_link_required(clock, output_bpp);
+		mode_rate = intel_dp_link_required(clock, link_bpp);
 
 		for (i = 0; i < intel_dp->num_common_rates; i++) {
 			link_rate = intel_dp_common_rate(intel_dp, i);
@@ -1707,20 +1707,20 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			return -EINVAL;
 		}
 	} else {
-		u16 dsc_max_output_bpp = 0;
+		u16 dsc_max_compressed_bpp = 0;
 		u8 dsc_dp_slice_count;
 
 		if (compute_pipe_bpp) {
-			dsc_max_output_bpp =
-				intel_dp_dsc_get_output_bpp(dev_priv,
-							    pipe_config->port_clock,
-							    pipe_config->lane_count,
-							    adjusted_mode->crtc_clock,
-							    adjusted_mode->crtc_hdisplay,
-							    pipe_config->bigjoiner_pipes,
-							    pipe_config->output_format,
-							    pipe_bpp,
-							    timeslots);
+			dsc_max_compressed_bpp =
+				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
+								    pipe_config->port_clock,
+								    pipe_config->lane_count,
+								    adjusted_mode->crtc_clock,
+								    adjusted_mode->crtc_hdisplay,
+								    pipe_config->bigjoiner_pipes,
+								    pipe_config->output_format,
+								    pipe_bpp,
+								    timeslots);
 			/*
 			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
 			 * supported PPS value can be 63.9375 and with the further
@@ -1728,9 +1728,11 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			 * restricting our target bpp to be 31.9375 at max
 			 */
 			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
-				dsc_max_output_bpp = min_t(u16, dsc_max_output_bpp, 31 << 4);
+				dsc_max_compressed_bpp = min_t(u16,
+							       dsc_max_compressed_bpp,
+							       31 << 4);
 
-			if (!dsc_max_output_bpp) {
+			if (!dsc_max_compressed_bpp) {
 				drm_dbg_kms(&dev_priv->drm,
 					    "Compressed BPP not supported\n");
 				return -EINVAL;
@@ -1758,7 +1760,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 							     pipe_config->pipe_bpp);
 
 			pipe_config->dsc.compressed_bpp = min_t(u16,
-								dsc_max_output_bpp >> 4,
+								dsc_max_compressed_bpp >> 4,
 								output_bpp);
 		}
 		pipe_config->dsc.slice_count = dsc_dp_slice_count;
@@ -2134,7 +2136,7 @@ static bool can_enable_drrs(struct intel_connector *connector,
 static void
 intel_dp_drrs_compute_config(struct intel_connector *connector,
 			     struct intel_crtc_state *pipe_config,
-			     int output_bpp)
+			     int link_bpp)
 {
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 	const struct drm_display_mode *downclock_mode =
@@ -2159,7 +2161,7 @@ intel_dp_drrs_compute_config(struct intel_connector *connector,
 	if (pipe_config->splitter.enable)
 		pixel_clock /= pipe_config->splitter.link_count;
 
-	intel_link_compute_m_n(output_bpp, pipe_config->lane_count, pixel_clock,
+	intel_link_compute_m_n(link_bpp, pipe_config->lane_count, pixel_clock,
 			       pipe_config->port_clock, &pipe_config->dp_m2_n2,
 			       pipe_config->fec_enable);
 
@@ -2256,7 +2258,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	const struct drm_display_mode *fixed_mode;
 	struct intel_connector *connector = intel_dp->attached_connector;
-	int ret = 0, output_bpp;
+	int ret = 0, link_bpp;
 
 	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && encoder->port != PORT_A)
 		pipe_config->has_pch_encoder = true;
@@ -2306,10 +2308,10 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		intel_dp_limited_color_range(pipe_config, conn_state);
 
 	if (pipe_config->dsc.compression_enable)
-		output_bpp = pipe_config->dsc.compressed_bpp;
+		link_bpp = pipe_config->dsc.compressed_bpp;
 	else
-		output_bpp = intel_dp_output_bpp(pipe_config->output_format,
-						 pipe_config->pipe_bpp);
+		link_bpp = intel_dp_output_bpp(pipe_config->output_format,
+					       pipe_config->pipe_bpp);
 
 	if (intel_dp->mso_link_count) {
 		int n = intel_dp->mso_link_count;
@@ -2333,7 +2335,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 	intel_dp_audio_compute_config(encoder, pipe_config, conn_state);
 
-	intel_link_compute_m_n(output_bpp,
+	intel_link_compute_m_n(link_bpp,
 			       pipe_config->lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
@@ -2349,7 +2351,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 	intel_vrr_compute_config(pipe_config, conn_state);
 	intel_psr_compute_config(intel_dp, pipe_config, conn_state);
-	intel_dp_drrs_compute_config(connector, pipe_config, output_bpp);
+	intel_dp_drrs_compute_config(connector, pipe_config, link_bpp);
 	intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state);
 	intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config, conn_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 856172bfa13e..42b98546c140 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -103,13 +103,13 @@ void intel_read_dp_sdp(struct intel_encoder *encoder,
 		       unsigned int type);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
 int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
-u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
-				u32 link_clock, u32 lane_count,
-				u32 mode_clock, u32 mode_hdisplay,
-				bool bigjoiner,
-				enum intel_output_format output_format,
-				u32 pipe_bpp,
-				u32 timeslots);
+u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
+					u32 link_clock, u32 lane_count,
+					u32 mode_clock, u32 mode_hdisplay,
+					bool bigjoiner,
+					enum intel_output_format output_format,
+					u32 pipe_bpp,
+					u32 timeslots);
 u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
 				int mode_clock, int mode_hdisplay,
 				bool bigjoiner);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2c2d258a3a77..d92af6122763 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -915,7 +915,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 	int ret;
 	bool dsc = false, bigjoiner = false;
-	u16 dsc_max_output_bpp = 0;
+	u16 dsc_max_compressed_bpp = 0;
 	u8 dsc_slice_count = 0;
 	int target_clock = mode->clock;
 
@@ -969,15 +969,15 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 		int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
 
 		if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) {
-			dsc_max_output_bpp =
-				intel_dp_dsc_get_output_bpp(dev_priv,
-							    max_link_clock,
-							    max_lanes,
-							    target_clock,
-							    mode->hdisplay,
-							    bigjoiner,
-							    INTEL_OUTPUT_FORMAT_RGB,
-							    pipe_bpp, 64) >> 4;
+			dsc_max_compressed_bpp =
+				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
+								    max_link_clock,
+								    max_lanes,
+								    target_clock,
+								    mode->hdisplay,
+								    bigjoiner,
+								    INTEL_OUTPUT_FORMAT_RGB,
+								    pipe_bpp, 64) >> 4;
 			dsc_slice_count =
 				intel_dp_dsc_get_slice_count(intel_dp,
 							     target_clock,
@@ -985,7 +985,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 							     bigjoiner);
 		}
 
-		dsc = dsc_max_output_bpp && dsc_slice_count;
+		dsc = dsc_max_compressed_bpp && dsc_slice_count;
 	}
 
 	/*
-- 
2.25.1

[PATCH 04/13] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Ankit Nautiyal]

In Bigjoiner check for DSC, bigjoiner interface bits for DP for
DISPLAY > 13 is 36 (Bspec: 49259).

v2: Corrected Display ver to 13.

v3: Follow convention for conditional statement. (Ville)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 24de25551a49..bca80c0793e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -783,8 +783,9 @@ u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
 	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
 
 	if (bigjoiner) {
+		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
 		u32 max_bpp_bigjoiner =
-			i915->display.cdclk.max_cdclk_freq * 48 /
+			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
 			intel_dp_mode_to_fec_clock(mode_clock);
 
 		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
-- 
2.25.1

[PATCH 05/13] drm/i915/intel_cdclk: Add vdsc with bigjoiner constraints on min_cdlck

[Ankit Nautiyal]

As per Bsepc:49259, Bigjoiner BW check puts restriction on the
compressed bpp for a given CDCLK, pixelclock in cases where
Bigjoiner + DSC are used.

Currently compressed bpp is computed first, and it is ensured that
the bpp will work at least with the max CDCLK freq.

Since the CDCLK is computed later, lets account for Bigjoiner BW
check while calculating Min CDCLK.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 49 ++++++++++++++++++----
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 6bed75f1541a..3532640c5027 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2520,6 +2520,46 @@ static int intel_planes_min_cdclk(const struct intel_crtc_state *crtc_state)
 	return min_cdclk;
 }
 
+static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	int min_cdclk = 0;
+
+	/*
+	 * When we decide to use only one VDSC engine, since
+	 * each VDSC operates with 1 ppc throughput, pixel clock
+	 * cannot be higher than the VDSC clock (cdclk)
+	 */
+	if (!crtc_state->dsc.dsc_split)
+		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
+
+	if (crtc_state->bigjoiner_pipes) {
+		/*
+		 * According to Bigjoiner bw check:
+		 * compressed_bpp <= PPC * CDCLK * Big joiner Interface bits / Pixel clock
+		 *
+		 * We have already computed compressed_bpp, so now compute the min CDCLK that
+		 * is required to support this compressed_bpp.
+		 *
+		 * => CDCLK >= compressed_bpp * Pixel clock / (PPC * Bigjoiner Interface bits)
+		 *
+		 * Since Num of pipes joined = 2, and PPC = 2 with bigjoiner
+		 * => CDCLK >= compressed_bpp * pixel_rate  / Bigjoiner Interface bits
+		 *
+		 * #TODO Bspec mentions to account for FEC overhead while using pixel clock.
+		 * Check if we need to use FEC overhead in the above calculations.
+		 */
+		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
+		int min_cdclk_bj = crtc_state->dsc.compressed_bpp * crtc_state->pixel_rate /
+				   bigjoiner_interface_bits;
+
+		min_cdclk = max(min_cdclk, min_cdclk_bj);
+	}
+
+	return min_cdclk;
+}
+
 int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv =
@@ -2591,13 +2631,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
 	/* Account for additional needs from the planes */
 	min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
 
-	/*
-	 * When we decide to use only one VDSC engine, since
-	 * each VDSC operates with 1 ppc throughput, pixel clock
-	 * cannot be higher than the VDSC clock (cdclk)
-	 */
-	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
-		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
+	if (crtc_state->dsc.compression_enable)
+		min_cdclk = max(min_cdclk, intel_vdsc_min_cdclk(crtc_state));
 
 	/*
 	 * HACK. Currently for TGL/DG2 platforms we calculate
-- 
2.25.1

[PATCH 06/13] drm/i915/dp: Remove extra logs for printing DSC info

[Ankit Nautiyal]

DSC compressed bpp and slice counts are already getting printed at the
end of dsc compute config. Remove extra logs.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bca80c0793e9..a7cc9d418f22 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1765,9 +1765,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 								output_bpp);
 		}
 		pipe_config->dsc.slice_count = dsc_dp_slice_count;
-		drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
-			    pipe_config->dsc.compressed_bpp,
-			    pipe_config->dsc.slice_count);
 	}
 	/*
 	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
-- 
2.25.1

[PATCH 07/13] drm/display/dp: Fix the DP DSC Receiver cap size

[Ankit Nautiyal]

DP DSC Receiver Capabilities are exposed via DPCD 60h-6Fh.
Fix the DSC RECEIVER CAP SIZE accordingly.

Fixes: ffddc4363c28 ("drm/dp: Add DP DSC DPCD receiver capability size define and missing SHIFT")
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: <stable@vger.kernel.org> # v5.0+

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 include/drm/display/drm_dp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index b046f79f4744..e6e86a134ddd 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -1537,7 +1537,7 @@ enum drm_dp_phy {
 
 #define DP_BRANCH_OUI_HEADER_SIZE	0xc
 #define DP_RECEIVER_CAP_SIZE		0xf
-#define DP_DSC_RECEIVER_CAP_SIZE        0xf
+#define DP_DSC_RECEIVER_CAP_SIZE        0x10 /* DSC Capabilities 0x60 through 0x6F */
 #define EDP_PSR_RECEIVER_CAP_SIZE	2
 #define EDP_DISPLAY_CTL_CAP_SIZE	3
 #define DP_LTTPR_COMMON_CAP_SIZE	8
-- 
2.25.1

[PATCH 08/13] drm/i915/dp: Avoid forcing DSC BPC for MST case

[Ankit Nautiyal]

For MST the bpc is hardcoded to 8, and pipe bpp to 24.
So avoid forcing DSC bpc for MST case.

v2: Warn and ignore the debug flag than to bail out. (Jani)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 11 +++++------
 drivers/gpu/drm/i915/display/intel_dp_mst.c |  5 +++++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a7cc9d418f22..7ea4f27a4bf5 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1669,14 +1669,13 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
 		return -EINVAL;
 
-	if (compute_pipe_bpp)
+	if (intel_dp->force_dsc_bpc && compute_pipe_bpp) {
+		pipe_bpp = intel_dp->force_dsc_bpc * 3;
+		drm_dbg_kms(&dev_priv->drm, "Input DSC BPP forced to %d\n", pipe_bpp);
+	} else if (compute_pipe_bpp) {
 		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
-	else
+	} else {
 		pipe_bpp = pipe_config->pipe_bpp;
-
-	if (intel_dp->force_dsc_bpc) {
-		pipe_bpp = intel_dp->force_dsc_bpc * 3;
-		drm_dbg_kms(&dev_priv->drm, "Input DSC BPP forced to %d", pipe_bpp);
 	}
 
 	/* Min Input BPC for ICL+ is 8 */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index d92af6122763..636e3e8e57e8 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -361,6 +361,11 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	/* enable compression if the mode doesn't fit available BW */
 	drm_dbg_kms(&dev_priv->drm, "Force DSC en = %d\n", intel_dp->force_dsc_en);
 	if (ret || intel_dp->force_dsc_en) {
+		/*
+		 * FIXME: As bpc is hardcoded to 8, as mentioned above,
+		 * WARN and ignore the debug flag force_dsc_bpc for now.
+		 */
+		drm_WARN(&dev_priv->drm, intel_dp->force_dsc_bpc, "Cannot Force BPC for MST\n");
 		/*
 		 * Try to get at least some timeslots and then see, if
 		 * we can fit there with DSC.
-- 
2.25.1

[PATCH 09/13] drm/i915/dp: Check min bpc DSC limits for dsc_force_bpc also

[Ankit Nautiyal]

For DSC the min BPC is 8 for ICL+ and so the min pipe_bpp is 24.
Check this condition for cases where bpc is forced by debugfs flag
dsc_force_bpc. If the check fails, then WARN and ignore the debugfs
flag.

For MST case the pipe_bpp is already computed (hardcoded to be 24),
and this check is not required.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 48 ++++++++++++++++---------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7ea4f27a4bf5..83fb198fcdae 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1646,6 +1646,13 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
 	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
 }
 
+static
+bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
+{
+	/* Min Input BPC for ICL+ is 8 */
+	return (pipe_bpp < 8 * 3);
+}
+
 int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				struct intel_crtc_state *pipe_config,
 				struct drm_connector_state *conn_state,
@@ -1657,7 +1664,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
 	const struct drm_display_mode *adjusted_mode =
 		&pipe_config->hw.adjusted_mode;
-	int pipe_bpp;
 	int ret;
 
 	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
@@ -1669,28 +1675,36 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
 		return -EINVAL;
 
-	if (intel_dp->force_dsc_bpc && compute_pipe_bpp) {
-		pipe_bpp = intel_dp->force_dsc_bpc * 3;
-		drm_dbg_kms(&dev_priv->drm, "Input DSC BPP forced to %d\n", pipe_bpp);
-	} else if (compute_pipe_bpp) {
-		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
-	} else {
-		pipe_bpp = pipe_config->pipe_bpp;
-	}
+	if (compute_pipe_bpp) {
+		int pipe_bpp;
+		int forced_bpp = intel_dp->force_dsc_bpc * 3;
 
-	/* Min Input BPC for ICL+ is 8 */
-	if (pipe_bpp < 8 * 3) {
-		drm_dbg_kms(&dev_priv->drm,
-			    "No DSC support for less than 8bpc\n");
-		return -EINVAL;
+		if (forced_bpp && is_dsc_pipe_bpp_sufficient(forced_bpp)) {
+			pipe_bpp = forced_bpp;
+			drm_dbg_kms(&dev_priv->drm, "Input DSC BPP forced to %d\n", pipe_bpp);
+		} else {
+			drm_WARN(&dev_priv->drm,
+				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
+				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
+				 intel_dp->force_dsc_bpc);
+
+			pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp,
+							    conn_state->max_requested_bpc);
+
+			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
+				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
+				return -EINVAL;
+			}
+		}
+
+		pipe_config->pipe_bpp = pipe_bpp;
 	}
 
 	/*
-	 * For now enable DSC for max bpp, max link rate, max lane count.
+	 * For now enable DSC for max link rate, max lane count.
 	 * Optimize this later for the minimum possible link rate/lane count
 	 * with DSC enabled for the requested mode.
 	 */
-	pipe_config->pipe_bpp = pipe_bpp;
 	pipe_config->port_clock = limits->max_rate;
 	pipe_config->lane_count = limits->max_lane_count;
 
@@ -1719,7 +1733,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 								    adjusted_mode->crtc_hdisplay,
 								    pipe_config->bigjoiner_pipes,
 								    pipe_config->output_format,
-								    pipe_bpp,
+								    pipe_config->pipe_bpp,
 								    timeslots);
 			/*
 			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
-- 
2.25.1

[PATCH 10/13] drm/i915/dp: Avoid left shift of DSC output bpp by 4

[Ankit Nautiyal]

To make way for fractional bpp support, avoid left shifting the
output_bpp by 4 in helper intel_dp_dsc_get_output_bpp.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 12 ++++--------
 drivers/gpu/drm/i915/display/intel_dp_mst.c |  2 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 83fb198fcdae..c5f1bc7620e2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -793,11 +793,7 @@ u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
 
 	bits_per_pixel = intel_dp_dsc_nearest_valid_bpp(i915, bits_per_pixel, pipe_bpp);
 
-	/*
-	 * Compressed BPP in U6.4 format so multiply by 16, for Gen 11,
-	 * fractional part is 0
-	 */
-	return bits_per_pixel << 4;
+	return bits_per_pixel;
 }
 
 u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
@@ -1187,7 +1183,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 								    mode->hdisplay,
 								    bigjoiner,
 								    output_format,
-								    pipe_bpp, 64) >> 4;
+								    pipe_bpp, 64);
 			dsc_slice_count =
 				intel_dp_dsc_get_slice_count(intel_dp,
 							     target_clock,
@@ -1744,7 +1740,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
 				dsc_max_compressed_bpp = min_t(u16,
 							       dsc_max_compressed_bpp,
-							       31 << 4);
+							       31);
 
 			if (!dsc_max_compressed_bpp) {
 				drm_dbg_kms(&dev_priv->drm,
@@ -1774,7 +1770,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 							     pipe_config->pipe_bpp);
 
 			pipe_config->dsc.compressed_bpp = min_t(u16,
-								dsc_max_compressed_bpp >> 4,
+								dsc_max_compressed_bpp,
 								output_bpp);
 		}
 		pipe_config->dsc.slice_count = dsc_dp_slice_count;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 636e3e8e57e8..daae3c9402a9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -982,7 +982,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 								    mode->hdisplay,
 								    bigjoiner,
 								    INTEL_OUTPUT_FORMAT_RGB,
-								    pipe_bpp, 64) >> 4;
+								    pipe_bpp, 64);
 			dsc_slice_count =
 				intel_dp_dsc_get_slice_count(intel_dp,
 							     target_clock,
-- 
2.25.1

[PATCH 11/13] drm/i915/dp: Rename helpers to get DSC max pipe_bpp/output_bpp

[Ankit Nautiyal]

Currently the required dsc output bpp is set to be the largest
compressed bpp supported for max, lane, rate, and bpp.
The helper intel_dp_dsc_get_output_bpp gets the maximum supported
compressed bpp taking into account link configuration, input bpp,
bigjoiner considerations etc.

Similarly, the helper intel_dp_dsc_compute_bpp gives the maximum
pipe bpp that is allowed with DSC.

Rename the functions to reflect that these return max DSC input and
output bpps.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 8 ++++----
 drivers/gpu/drm/i915/display/intel_dp.h     | 2 +-
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index c5f1bc7620e2..39e2bf3d738d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1162,7 +1162,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 		 * TBD pass the connector BPC,
 		 * for now U8_MAX so that max BPC on that platform would be picked
 		 */
-		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
+		pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp, U8_MAX);
 
 		/*
 		 * Output bpp is stored in 6.4 format so right shift by 4 to get the
@@ -1503,7 +1503,7 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
 	return -EINVAL;
 }
 
-int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 max_req_bpc)
+int intel_dp_dsc_compute_max_bpp(struct intel_dp *intel_dp, u8 max_req_bpc)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	int i, num_bpc;
@@ -1684,8 +1684,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
 				 intel_dp->force_dsc_bpc);
 
-			pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp,
-							    conn_state->max_requested_bpc);
+			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
+								conn_state->max_requested_bpc);
 
 			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
 				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 42b98546c140..295b967de3cc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -102,7 +102,7 @@ void intel_read_dp_sdp(struct intel_encoder *encoder,
 		       struct intel_crtc_state *crtc_state,
 		       unsigned int type);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
-int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
+int intel_dp_dsc_compute_max_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
 u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
 					u32 link_clock, u32 lane_count,
 					u32 mode_clock, u32 mode_hdisplay,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index daae3c9402a9..f02ae68a9877 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -971,7 +971,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 		 * TBD pass the connector BPC,
 		 * for now U8_MAX so that max BPC on that platform would be picked
 		 */
-		int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
+		int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp, U8_MAX);
 
 		if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) {
 			dsc_max_compressed_bpp =
-- 
2.25.1

[PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Ankit Nautiyal]

Currently, we take the max lane, rate and pipe bpp, to get the maximum
compressed bpp possible. We then set the output bpp to this value.
This patch provides support to have max bpp, min rate and min lanes,
that can support the min compressed bpp.

v2:
-Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
-Fix the checks for limits->max/min_bpp while iterating over list of
 valid DSC bpcs. (Stan)

v3:
-Refactor the code to have pipe bpp/compressed bpp computation and slice
count calculation separately for different cases.

v4:
-Separate the pipe_bpp calculation for eDP and DP.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
 1 file changed, 245 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 39e2bf3d738d..578320220c9a 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
 	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
 }
 
+static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
+				 const struct drm_display_mode *adjusted_mode)
+{
+	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
+	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
+
+	return mode_rate <= link_avail;
+}
+
+static int dsc_compute_link_config(struct intel_dp *intel_dp,
+				   struct intel_crtc_state *pipe_config,
+				   struct link_config_limits *limits,
+				   int pipe_bpp,
+				   u16 compressed_bpp,
+				   int timeslots)
+{
+	const struct drm_display_mode *adjusted_mode =
+		&pipe_config->hw.adjusted_mode;
+	int link_rate, lane_count;
+	int dsc_max_bpp;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	int i;
+
+	for (i = 0; i < intel_dp->num_common_rates; i++) {
+		link_rate = intel_dp_common_rate(intel_dp, i);
+		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
+			continue;
+
+		for (lane_count = limits->min_lane_count;
+		     lane_count <= limits->max_lane_count;
+		     lane_count <<= 1) {
+			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
+									  link_rate,
+									  lane_count,
+									  adjusted_mode->crtc_clock,
+									  adjusted_mode->crtc_hdisplay,
+									  pipe_config->bigjoiner_pipes,
+									  pipe_config->output_format,
+									  pipe_bpp, timeslots);
+			/*
+			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
+			 * supported PPS value can be 63.9375 and with the further
+			 * mention that bpp should be programmed double the target bpp
+			 * restricting our target bpp to be 31.9375 at max
+			 */
+			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
+				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
+
+			if (compressed_bpp > dsc_max_bpp)
+				continue;
+
+			if (!is_dsc_bw_sufficient(link_rate, lane_count,
+						  compressed_bpp, adjusted_mode))
+				continue;
+
+			pipe_config->lane_count = lane_count;
+			pipe_config->port_clock = link_rate;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static
+u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
+					    struct intel_crtc_state *pipe_config,
+					    int bpc)
+{
+	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
+
+	if (max_bppx16)
+		return max_bppx16;
+	/*
+	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
+	 * values as given in spec Table 2-157 DP v2.0
+	 */
+	switch (pipe_config->output_format) {
+	case INTEL_OUTPUT_FORMAT_RGB:
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return (3 * bpc) << 4;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return (3 * (bpc / 2)) << 4;
+	default:
+		MISSING_CASE(pipe_config->output_format);
+		break;
+	}
+
+	return 0;
+}
+
+static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
+{
+	switch (pipe_config->output_format) {
+	case INTEL_OUTPUT_FORMAT_RGB:
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return 8 << 4;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return 6 << 4;
+	default:
+		MISSING_CASE(pipe_config->output_format);
+		break;
+	}
+
+	return 0;
+}
+
+static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
+				      struct intel_crtc_state *pipe_config,
+				      struct link_config_limits *limits,
+				      int pipe_bpp,
+				      int timeslots)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	u16 compressed_bpp;
+	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
+	int ret;
+
+	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
+	if (DISPLAY_VER(dev_priv) <= 12)
+		dsc_src_max_bpp = 23;
+	else
+		dsc_src_max_bpp = 27;
+	dsc_sink_max_bpp =
+		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
+							pipe_config, pipe_bpp / 3) >> 4;
+
+	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
+
+	/* Compressed BPP should be less than the Input DSC bpp */
+	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
+
+	for (compressed_bpp = dsc_max_bpp;
+	     compressed_bpp >= dsc_min_bpp;
+	     compressed_bpp--) {
+		ret = dsc_compute_link_config(intel_dp,
+					      pipe_config,
+					      limits,
+					      pipe_bpp,
+					      compressed_bpp,
+					      timeslots);
+		if (ret == 0) {
+			pipe_config->dsc.compressed_bpp = compressed_bpp;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
+					 struct intel_crtc_state *pipe_config,
+					 struct drm_connector_state *conn_state,
+					 struct link_config_limits *limits,
+					 int timeslots)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	u8 dsc_bpc[3] = {0};
+	u8 dsc_max_bpc, dsc_max_bpp;
+	u8 dsc_min_bpc, dsc_min_bpp;
+	u8 max_req_bpc = conn_state->max_requested_bpc;
+	int i, bpp, ret;
+	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
+							   dsc_bpc);
+
+	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
+	if (DISPLAY_VER(i915) >= 12)
+		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
+	else
+		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
+
+	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
+
+	/* Min DSC Input BPC for ICL+ is 8 */
+	dsc_min_bpc = 8;
+	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
+
+	/*
+	 * Get the maximum DSC bpc that will be supported by any valid
+	 * link configuration and compressed bpp.
+	 */
+	for (i = 0; i < num_bpc; i++) {
+		bpp = dsc_bpc[i] * 3;
+
+		if (bpp < dsc_min_bpp)
+			break;
+
+		if (bpp > dsc_max_bpp)
+			continue;
+
+		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
+						 limits, bpp, timeslots);
+		if (ret == 0) {
+			pipe_config->pipe_bpp = bpp;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static
 bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
 {
@@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
 	return (pipe_bpp < 8 * 3);
 }
 
+static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
+					  struct intel_crtc_state *pipe_config,
+					  struct drm_connector_state *conn_state,
+					  struct link_config_limits *limits)
+{
+	/* For eDP use max bpp that can be supported with DSC. */
+	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
+						    conn_state->max_requested_bpc);
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
+		drm_dbg_kms(&i915->drm,
+			    "No DSC support for less than 8bpc\n");
+		return -EINVAL;
+	}
+	pipe_config->pipe_bpp = pipe_bpp;
+	pipe_config->port_clock = limits->max_rate;
+	pipe_config->lane_count = limits->max_lane_count;
+	pipe_config->dsc.compressed_bpp =
+		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
+		      pipe_config->pipe_bpp);
+
+	return 0;
+}
+
 int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				struct intel_crtc_state *pipe_config,
 				struct drm_connector_state *conn_state,
@@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
 		return -EINVAL;
 
+	/*
+	 * compute pipe bpp is set to false for DP MST DSC case
+	 * and compressed_bpp is calculated same time once
+	 * vpci timeslots are allocated, because overall bpp
+	 * calculation procedure is bit different for MST case.
+	 */
 	if (compute_pipe_bpp) {
 		int pipe_bpp;
 		int forced_bpp = intel_dp->force_dsc_bpc * 3;
@@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
 				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
 				 intel_dp->force_dsc_bpc);
-
-			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
-								conn_state->max_requested_bpc);
-
-			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
-				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
-				return -EINVAL;
+			if (intel_dp_is_edp(intel_dp))
+				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
+								     conn_state, limits);
+			else
+				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
+								    conn_state, limits, timeslots);
+			if (ret) {
+				drm_dbg_kms(&dev_priv->drm,
+					    "No Valid pipe bpp for given mode ret = %d\n", ret);
+				return ret;
 			}
 		}
-
-		pipe_config->pipe_bpp = pipe_bpp;
 	}
 
-	/*
-	 * For now enable DSC for max link rate, max lane count.
-	 * Optimize this later for the minimum possible link rate/lane count
-	 * with DSC enabled for the requested mode.
-	 */
 	pipe_config->port_clock = limits->max_rate;
 	pipe_config->lane_count = limits->max_lane_count;
 
+	/* Calculate Slice count */
 	if (intel_dp_is_edp(intel_dp)) {
-		pipe_config->dsc.compressed_bpp =
-			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
-			      pipe_config->pipe_bpp);
 		pipe_config->dsc.slice_count =
 			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
 							true);
@@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			return -EINVAL;
 		}
 	} else {
-		u16 dsc_max_compressed_bpp = 0;
 		u8 dsc_dp_slice_count;
 
-		if (compute_pipe_bpp) {
-			dsc_max_compressed_bpp =
-				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
-								    pipe_config->port_clock,
-								    pipe_config->lane_count,
-								    adjusted_mode->crtc_clock,
-								    adjusted_mode->crtc_hdisplay,
-								    pipe_config->bigjoiner_pipes,
-								    pipe_config->output_format,
-								    pipe_config->pipe_bpp,
-								    timeslots);
-			/*
-			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
-			 * supported PPS value can be 63.9375 and with the further
-			 * mention that bpp should be programmed double the target bpp
-			 * restricting our target bpp to be 31.9375 at max
-			 */
-			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
-				dsc_max_compressed_bpp = min_t(u16,
-							       dsc_max_compressed_bpp,
-							       31);
-
-			if (!dsc_max_compressed_bpp) {
-				drm_dbg_kms(&dev_priv->drm,
-					    "Compressed BPP not supported\n");
-				return -EINVAL;
-			}
-		}
 		dsc_dp_slice_count =
 			intel_dp_dsc_get_slice_count(intel_dp,
 						     adjusted_mode->crtc_clock,
@@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			return -EINVAL;
 		}
 
-		/*
-		 * compute pipe bpp is set to false for DP MST DSC case
-		 * and compressed_bpp is calculated same time once
-		 * vpci timeslots are allocated, because overall bpp
-		 * calculation procedure is bit different for MST case.
-		 */
-		if (compute_pipe_bpp) {
-			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
-							     pipe_config->pipe_bpp);
-
-			pipe_config->dsc.compressed_bpp = min_t(u16,
-								dsc_max_compressed_bpp,
-								output_bpp);
-		}
 		pipe_config->dsc.slice_count = dsc_dp_slice_count;
 	}
 	/*
-- 
2.25.1

[PATCH 13/13] drm/i915: Query compressed bpp properly using correct DPCD and DP Spec info

[Ankit Nautiyal]

From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Currently we seem to be using wrong DPCD register for reading compressed bpps,
reading min/max input bpc instead of compressed bpp.
Fix that, so that we now apply min/max compressed bpp limitations we get
from DP Spec Table 2-157 DP v2.0 and/or correspondent DPCD register
DP_DSC_MAX_BITS_PER_PIXEL_LOW/HIGH.

This might also allow us to get rid of an ugly compressed bpp recalculation,
which we had to add to make some MST hubs usable.

v2: - Fix operator precedence
v3: - Added debug info about compressed bpps
v4: - Don't try to intersect Sink input bpp and compressed bpps.
v5: - Decrease step while looking for suitable compressed bpp to
      accommodate.
v6: - Use helper for getting min and max compressed_bpp (Ankit)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     |  3 +-
 drivers/gpu/drm/i915/display/intel_dp.h     |  4 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 43 ++++++++-------------
 3 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 578320220c9a..3cb442516eb9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1707,7 +1707,6 @@ static int dsc_compute_link_config(struct intel_dp *intel_dp,
 	return -EINVAL;
 }
 
-static
 u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
 					    struct intel_crtc_state *pipe_config,
 					    int bpc)
@@ -1734,7 +1733,7 @@ u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
 	return 0;
 }
 
-static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
+u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
 {
 	switch (pipe_config->output_format) {
 	case INTEL_OUTPUT_FORMAT_RGB:
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 295b967de3cc..dcb32bd0c656 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -123,6 +123,10 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 
 u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
 u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 pipe_bpp);
+u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
+					    struct intel_crtc_state *pipe_config,
+					    int bpc);
+u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config);
 
 void intel_ddi_update_pipe(struct intel_atomic_state *state,
 			   struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index f02ae68a9877..461f588aab8d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -101,6 +101,9 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 							      crtc_state->lane_count);
 	}
 
+	drm_dbg_kms(&i915->drm, "Looking for slots in range min bpp %d max bpp %d\n",
+		    min_bpp, max_bpp);
+
 	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
 		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
 
@@ -194,8 +197,7 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder,
 	u8 dsc_bpc[3] = {0};
 	int min_bpp, max_bpp, sink_min_bpp, sink_max_bpp;
 	u8 dsc_max_bpc;
-	bool need_timeslot_recalc = false;
-	u32 last_compressed_bpp;
+	int min_compressed_bpp, max_compressed_bpp;
 
 	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
 	if (DISPLAY_VER(i915) >= 12)
@@ -231,34 +233,21 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder,
 	if (max_bpp > sink_max_bpp)
 		max_bpp = sink_max_bpp;
 
-	slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, max_bpp,
-						     min_bpp, limits,
-						     conn_state, 2 * 3, true);
-
-	if (slots < 0)
-		return slots;
-
-	last_compressed_bpp = crtc_state->dsc.compressed_bpp;
+	max_compressed_bpp = intel_dp_dsc_max_sink_compressed_bppx16(intel_dp, crtc_state, max_bpp / 3) >> 4;
+	min_compressed_bpp = intel_dp_dsc_min_compressed_bppx16(crtc_state) >> 4;
+	drm_dbg_kms(&i915->drm, "DSC Sink supported compressed min bpp %d compressed max bpp %d\n",
+		    min_compressed_bpp, max_compressed_bpp);
 
-	crtc_state->dsc.compressed_bpp = intel_dp_dsc_nearest_valid_bpp(i915,
-									last_compressed_bpp,
-									crtc_state->pipe_bpp);
+	/* Align compressed bpps according to our own constraints */
+	max_compressed_bpp = intel_dp_dsc_nearest_valid_bpp(i915, max_compressed_bpp, crtc_state->pipe_bpp);
+	min_compressed_bpp = intel_dp_dsc_nearest_valid_bpp(i915, min_compressed_bpp, crtc_state->pipe_bpp);
 
-	if (crtc_state->dsc.compressed_bpp != last_compressed_bpp)
-		need_timeslot_recalc = true;
+	slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, max_compressed_bpp,
+						     min_compressed_bpp, limits,
+						     conn_state, 1, true);
 
-	/*
-	 * Apparently some MST hubs dislike if vcpi slots are not matching precisely
-	 * the actual compressed bpp we use.
-	 */
-	if (need_timeslot_recalc) {
-		slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state,
-							     crtc_state->dsc.compressed_bpp,
-							     crtc_state->dsc.compressed_bpp,
-							     limits, conn_state, 2 * 3, true);
-		if (slots < 0)
-			return slots;
-	}
+	if (slots < 0)
+		return slots;
 
 	intel_link_compute_m_n(crtc_state->dsc.compressed_bpp,
 			       crtc_state->lane_count,
-- 
2.25.1

Re: [PATCH 01/13] drm/i915/dp: Consider output_format while computing dsc bpp

[Ville Syrjälä]

On Fri, May 12, 2023 at 11:54:05AM +0530, Ankit Nautiyal wrote:
> While using DSC the compressed bpp is computed assuming RGB output
> format. Consider the output_format and compute the compressed bpp
> during mode valid and compute config steps.
> 
> For DP-MST we currently use RGB output format only, so continue
> using RGB while computing compressed bpp for MST case.
> 
> v2: Use output_bpp instead for pipe_bpp to clamp compressed_bpp. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 19 +++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dp.h     |  1 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c |  1 +
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0cc57681dc4d..263c30948117 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -734,6 +734,7 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  				u32 link_clock, u32 lane_count,
>  				u32 mode_clock, u32 mode_hdisplay,
>  				bool bigjoiner,
> +				enum intel_output_format output_format,
>  				u32 pipe_bpp,
>  				u32 timeslots)
>  {
> @@ -758,6 +759,10 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  	bits_per_pixel = ((link_clock * lane_count) * timeslots) /
>  			 (intel_dp_mode_to_fec_clock(mode_clock) * 8);
>  
> +	/* Bandwidth required for 420 is half, that of 444 format */
> +	if (output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> +		bits_per_pixel *= 2;
> +
>  	drm_dbg_kms(&i915->drm, "Max link bpp is %u for %u timeslots "
>  				"total bw %u pixel clock %u\n",
>  				bits_per_pixel, timeslots,
> @@ -1151,11 +1156,16 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  
>  	if (HAS_DSC(dev_priv) &&
>  	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> +		enum intel_output_format sink_format, output_format;
> +		int pipe_bpp;
> +
> +		sink_format = intel_dp_sink_format(connector, mode);
> +		output_format = intel_dp_output_format(connector, sink_format);
>  		/*
>  		 * TBD pass the connector BPC,
>  		 * for now U8_MAX so that max BPC on that platform would be picked
>  		 */
> -		int pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
> +		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, U8_MAX);
>  
>  		/*
>  		 * Output bpp is stored in 6.4 format so right shift by 4 to get the
> @@ -1175,6 +1185,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  							    target_clock,
>  							    mode->hdisplay,
>  							    bigjoiner,
> +							    output_format,
>  							    pipe_bpp, 64) >> 4;
>  			dsc_slice_count =
>  				intel_dp_dsc_get_slice_count(intel_dp,
> @@ -1707,6 +1718,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  							    adjusted_mode->crtc_clock,
>  							    adjusted_mode->crtc_hdisplay,
>  							    pipe_config->bigjoiner_pipes,
> +							    pipe_config->output_format,
>  							    pipe_bpp,
>  							    timeslots);
>  			/*
> @@ -1742,9 +1754,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  		 * calculation procedure is bit different for MST case.
>  		 */
>  		if (compute_pipe_bpp) {
> +			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> +							     pipe_config->pipe_bpp);
> +
>  			pipe_config->dsc.compressed_bpp = min_t(u16,
>  								dsc_max_output_bpp >> 4,
> -								pipe_config->pipe_bpp);
> +								output_bpp);
>  		}
>  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
>  		drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index ef39e4f7a329..db86c2b71c1f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -107,6 +107,7 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  				u32 link_clock, u32 lane_count,
>  				u32 mode_clock, u32 mode_hdisplay,
>  				bool bigjoiner,
> +				enum intel_output_format output_format,
>  				u32 pipe_bpp,
>  				u32 timeslots);
>  u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 63d61e610210..ee28bb89bffe 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -973,6 +973,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  							    target_clock,
>  							    mode->hdisplay,
>  							    bigjoiner,
> +							    INTEL_OUTPUT_FORMAT_RGB,
>  							    pipe_bpp, 64) >> 4;
>  			dsc_slice_count =
>  				intel_dp_dsc_get_slice_count(intel_dp,
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 04/13] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Ville Syrjälä]

On Fri, May 12, 2023 at 11:54:08AM +0530, Ankit Nautiyal wrote:
> In Bigjoiner check for DSC, bigjoiner interface bits for DP for
> DISPLAY > 13 is 36 (Bspec: 49259).
> 
> v2: Corrected Display ver to 13.
> 
> v3: Follow convention for conditional statement. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 24de25551a49..bca80c0793e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -783,8 +783,9 @@ u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
>  	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
>  
>  	if (bigjoiner) {
> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;

'x >= 14' is the usual convention.

with that
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  		u32 max_bpp_bigjoiner =
> -			i915->display.cdclk.max_cdclk_freq * 48 /
> +			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>  			intel_dp_mode_to_fec_clock(mode_clock);
>  
>  		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 05/13] drm/i915/intel_cdclk: Add vdsc with bigjoiner constraints on min_cdlck

[Ville Syrjälä]

On Fri, May 12, 2023 at 11:54:09AM +0530, Ankit Nautiyal wrote:
> As per Bsepc:49259, Bigjoiner BW check puts restriction on the
> compressed bpp for a given CDCLK, pixelclock in cases where
> Bigjoiner + DSC are used.
> 
> Currently compressed bpp is computed first, and it is ensured that
> the bpp will work at least with the max CDCLK freq.
> 
> Since the CDCLK is computed later, lets account for Bigjoiner BW
> check while calculating Min CDCLK.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 49 ++++++++++++++++++----
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 6bed75f1541a..3532640c5027 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2520,6 +2520,46 @@ static int intel_planes_min_cdclk(const struct intel_crtc_state *crtc_state)
>  	return min_cdclk;
>  }
>  
> +static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	int min_cdclk = 0;
> +
> +	/*
> +	 * When we decide to use only one VDSC engine, since
> +	 * each VDSC operates with 1 ppc throughput, pixel clock
> +	 * cannot be higher than the VDSC clock (cdclk)
> +	 */
> +	if (!crtc_state->dsc.dsc_split)
> +		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
> +
> +	if (crtc_state->bigjoiner_pipes) {
> +		/*
> +		 * According to Bigjoiner bw check:
> +		 * compressed_bpp <= PPC * CDCLK * Big joiner Interface bits / Pixel clock
> +		 *
> +		 * We have already computed compressed_bpp, so now compute the min CDCLK that
> +		 * is required to support this compressed_bpp.
> +		 *
> +		 * => CDCLK >= compressed_bpp * Pixel clock / (PPC * Bigjoiner Interface bits)
> +		 *
> +		 * Since Num of pipes joined = 2, and PPC = 2 with bigjoiner
> +		 * => CDCLK >= compressed_bpp * pixel_rate  / Bigjoiner Interface bits
> +		 *
> +		 * #TODO Bspec mentions to account for FEC overhead while using pixel clock.
> +		 * Check if we need to use FEC overhead in the above calculations.
> +		 */
> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
> +		int min_cdclk_bj = crtc_state->dsc.compressed_bpp * crtc_state->pixel_rate /
> +				   bigjoiner_interface_bits;

pixel_rate is the downscale adjusted thing, so it doesn't seem
like the correct thing to use here.

Hmm. Assuming that the single VDSC engine really throttles the entire
pipe to 1 PPC then we should probably account for the 1 vs. 2 PPC
difference in *_plane_min_cdclk() and intel_pixel_rate_to_cdclk()
directly. Currently all of those assume 2 PPC.

> +
> +		min_cdclk = max(min_cdclk, min_cdclk_bj);
> +	}
> +
> +	return min_cdclk;
> +}
> +
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -2591,13 +2631,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>  	/* Account for additional needs from the planes */
>  	min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
>  
> -	/*
> -	 * When we decide to use only one VDSC engine, since
> -	 * each VDSC operates with 1 ppc throughput, pixel clock
> -	 * cannot be higher than the VDSC clock (cdclk)
> -	 */
> -	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
> -		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
> +	if (crtc_state->dsc.compression_enable)
> +		min_cdclk = max(min_cdclk, intel_vdsc_min_cdclk(crtc_state));
>  
>  	/*
>  	 * HACK. Currently for TGL/DG2 platforms we calculate
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [PATCH 05/13] drm/i915/intel_cdclk: Add vdsc with bigjoiner constraints on min_cdlck

[Lisovskiy, Stanislav]

On Mon, May 15, 2023 at 05:44:51PM +0300, Ville Syrjälä wrote:
> On Fri, May 12, 2023 at 11:54:09AM +0530, Ankit Nautiyal wrote:
> > As per Bsepc:49259, Bigjoiner BW check puts restriction on the
> > compressed bpp for a given CDCLK, pixelclock in cases where
> > Bigjoiner + DSC are used.
> > 
> > Currently compressed bpp is computed first, and it is ensured that
> > the bpp will work at least with the max CDCLK freq.
> > 
> > Since the CDCLK is computed later, lets account for Bigjoiner BW
> > check while calculating Min CDCLK.
> > 
> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 49 ++++++++++++++++++----
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 6bed75f1541a..3532640c5027 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2520,6 +2520,46 @@ static int intel_planes_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  	return min_cdclk;
> >  }
> >  
> > +static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +	int min_cdclk = 0;
> > +
> > +	/*
> > +	 * When we decide to use only one VDSC engine, since
> > +	 * each VDSC operates with 1 ppc throughput, pixel clock
> > +	 * cannot be higher than the VDSC clock (cdclk)
> > +	 */
> > +	if (!crtc_state->dsc.dsc_split)
> > +		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
> > +
> > +	if (crtc_state->bigjoiner_pipes) {
> > +		/*
> > +		 * According to Bigjoiner bw check:
> > +		 * compressed_bpp <= PPC * CDCLK * Big joiner Interface bits / Pixel clock
> > +		 *
> > +		 * We have already computed compressed_bpp, so now compute the min CDCLK that
> > +		 * is required to support this compressed_bpp.
> > +		 *
> > +		 * => CDCLK >= compressed_bpp * Pixel clock / (PPC * Bigjoiner Interface bits)
> > +		 *
> > +		 * Since Num of pipes joined = 2, and PPC = 2 with bigjoiner
> > +		 * => CDCLK >= compressed_bpp * pixel_rate  / Bigjoiner Interface bits
> > +		 *
> > +		 * #TODO Bspec mentions to account for FEC overhead while using pixel clock.
> > +		 * Check if we need to use FEC overhead in the above calculations.
> > +		 */
> > +		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
> > +		int min_cdclk_bj = crtc_state->dsc.compressed_bpp * crtc_state->pixel_rate /
> > +				   bigjoiner_interface_bits;
> 
> pixel_rate is the downscale adjusted thing, so it doesn't seem
> like the correct thing to use here.
> 
> Hmm. Assuming that the single VDSC engine really throttles the entire
> pipe to 1 PPC then we should probably account for the 1 vs. 2 PPC
> difference in *_plane_min_cdclk() and intel_pixel_rate_to_cdclk()
> directly. Currently all of those assume 2 PPC.

Main thing is to properly align that one you propose above with that check,
where we decide how many VDSC engines to use:

        /*
         * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
         * is greater than the maximum Cdclock and if slice count is even
         * then we need to use 2 VDSC instances.
         */
        if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
                if (pipe_config->dsc.slice_count > 1) {
                        pipe_config->dsc.dsc_split = true;
                } else {
                        drm_dbg_kms(&dev_priv->drm,
                                    "Cannot split stream to use 2 VDSC instances\n");
                        return -EINVAL;
                }
        }

Otherwise I agree that we should do that check preferrably in *_plane_min_cdclk
and use plane data rate which is adjusted after scaling is applied(I think we even have correspondent function there)
It is strange that scaling wasn't mentioned in BSpec formula.
I would also say that we should account for number of slices(i.e VDSC engines) now only in Bigjoiner case, but always, as I understand that number can be different not only for Bigjoiner cases.

Stan


> 
> > +
> > +		min_cdclk = max(min_cdclk, min_cdclk_bj);
> > +	}
> > +
> > +	return min_cdclk;
> > +}
> > +
> >  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > @@ -2591,13 +2631,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  	/* Account for additional needs from the planes */
> >  	min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
> >  
> > -	/*
> > -	 * When we decide to use only one VDSC engine, since
> > -	 * each VDSC operates with 1 ppc throughput, pixel clock
> > -	 * cannot be higher than the VDSC clock (cdclk)
> > -	 */
> > -	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
> > -		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
> > +	if (crtc_state->dsc.compression_enable)
> > +		min_cdclk = max(min_cdclk, intel_vdsc_min_cdclk(crtc_state));
> >  
> >  	/*
> >  	 * HACK. Currently for TGL/DG2 platforms we calculate
> > -- 
> > 2.25.1
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Lisovskiy, Stanislav]

On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> Currently, we take the max lane, rate and pipe bpp, to get the maximum
> compressed bpp possible. We then set the output bpp to this value.
> This patch provides support to have max bpp, min rate and min lanes,
> that can support the min compressed bpp.
> 
> v2:
> -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> -Fix the checks for limits->max/min_bpp while iterating over list of
>  valid DSC bpcs. (Stan)
> 
> v3:
> -Refactor the code to have pipe bpp/compressed bpp computation and slice
> count calculation separately for different cases.
> 
> v4:
> -Separate the pipe_bpp calculation for eDP and DP.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
>  1 file changed, 245 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 39e2bf3d738d..578320220c9a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
>  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
>  }
>  
> +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> +				 const struct drm_display_mode *adjusted_mode)
> +{
> +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> +
> +	return mode_rate <= link_avail;
> +}
> +
> +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> +				   struct intel_crtc_state *pipe_config,
> +				   struct link_config_limits *limits,
> +				   int pipe_bpp,
> +				   u16 compressed_bpp,
> +				   int timeslots)
> +{
> +	const struct drm_display_mode *adjusted_mode =
> +		&pipe_config->hw.adjusted_mode;
> +	int link_rate, lane_count;
> +	int dsc_max_bpp;
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	int i;
> +
> +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> +		link_rate = intel_dp_common_rate(intel_dp, i);
> +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> +			continue;
> +
> +		for (lane_count = limits->min_lane_count;
> +		     lane_count <= limits->max_lane_count;
> +		     lane_count <<= 1) {
> +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> +									  link_rate,
> +									  lane_count,
> +									  adjusted_mode->crtc_clock,
> +									  adjusted_mode->crtc_hdisplay,
> +									  pipe_config->bigjoiner_pipes,
> +									  pipe_config->output_format,
> +									  pipe_bpp, timeslots);
> +			/*
> +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> +			 * supported PPS value can be 63.9375 and with the further
> +			 * mention that bpp should be programmed double the target bpp
> +			 * restricting our target bpp to be 31.9375 at max
> +			 */
> +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> +
> +			if (compressed_bpp > dsc_max_bpp)
> +				continue;
> +
> +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> +						  compressed_bpp, adjusted_mode))
> +				continue;
> +
> +			pipe_config->lane_count = lane_count;
> +			pipe_config->port_clock = link_rate;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static
> +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> +					    struct intel_crtc_state *pipe_config,
> +					    int bpc)
> +{
> +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> +
> +	if (max_bppx16)
> +		return max_bppx16;
> +	/*
> +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> +	 * values as given in spec Table 2-157 DP v2.0
> +	 */
> +	switch (pipe_config->output_format) {
> +	case INTEL_OUTPUT_FORMAT_RGB:
> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> +		return (3 * bpc) << 4;
> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> +		return (3 * (bpc / 2)) << 4;
> +	default:
> +		MISSING_CASE(pipe_config->output_format);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> +{
> +	switch (pipe_config->output_format) {
> +	case INTEL_OUTPUT_FORMAT_RGB:
> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> +		return 8 << 4;
> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> +		return 6 << 4;
> +	default:
> +		MISSING_CASE(pipe_config->output_format);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> +				      struct intel_crtc_state *pipe_config,
> +				      struct link_config_limits *limits,
> +				      int pipe_bpp,
> +				      int timeslots)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	u16 compressed_bpp;
> +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> +	int ret;
> +
> +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> +	if (DISPLAY_VER(dev_priv) <= 12)
> +		dsc_src_max_bpp = 23;
> +	else
> +		dsc_src_max_bpp = 27;

I would may be added some comment about what are those "23/27" numbers or
may be even created some self-explanatory #define constants for those.
Otherwise in a couple of years, even we ourselves might not be able to recall
immediately where those numbers are taken from.

Otherwise looks good to me,

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> +	dsc_sink_max_bpp =
> +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
> +							pipe_config, pipe_bpp / 3) >> 4;
> +
> +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
> +
> +	/* Compressed BPP should be less than the Input DSC bpp */
> +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
> +
> +	for (compressed_bpp = dsc_max_bpp;
> +	     compressed_bpp >= dsc_min_bpp;
> +	     compressed_bpp--) {
> +		ret = dsc_compute_link_config(intel_dp,
> +					      pipe_config,
> +					      limits,
> +					      pipe_bpp,
> +					      compressed_bpp,
> +					      timeslots);
> +		if (ret == 0) {
> +			pipe_config->dsc.compressed_bpp = compressed_bpp;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> +					 struct intel_crtc_state *pipe_config,
> +					 struct drm_connector_state *conn_state,
> +					 struct link_config_limits *limits,
> +					 int timeslots)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u8 dsc_bpc[3] = {0};
> +	u8 dsc_max_bpc, dsc_max_bpp;
> +	u8 dsc_min_bpc, dsc_min_bpp;
> +	u8 max_req_bpc = conn_state->max_requested_bpc;
> +	int i, bpp, ret;
> +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> +							   dsc_bpc);
> +
> +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
> +	if (DISPLAY_VER(i915) >= 12)
> +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
> +	else
> +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
> +
> +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
> +
> +	/* Min DSC Input BPC for ICL+ is 8 */
> +	dsc_min_bpc = 8;
> +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
> +
> +	/*
> +	 * Get the maximum DSC bpc that will be supported by any valid
> +	 * link configuration and compressed bpp.
> +	 */
> +	for (i = 0; i < num_bpc; i++) {
> +		bpp = dsc_bpc[i] * 3;
> +
> +		if (bpp < dsc_min_bpp)
> +			break;
> +
> +		if (bpp > dsc_max_bpp)
> +			continue;
> +
> +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
> +						 limits, bpp, timeslots);
> +		if (ret == 0) {
> +			pipe_config->pipe_bpp = bpp;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static
>  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>  {
> @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>  	return (pipe_bpp < 8 * 3);
>  }
>  
> +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> +					  struct intel_crtc_state *pipe_config,
> +					  struct drm_connector_state *conn_state,
> +					  struct link_config_limits *limits)
> +{
> +	/* For eDP use max bpp that can be supported with DSC. */
> +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> +						    conn_state->max_requested_bpc);
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> +		drm_dbg_kms(&i915->drm,
> +			    "No DSC support for less than 8bpc\n");
> +		return -EINVAL;
> +	}
> +	pipe_config->pipe_bpp = pipe_bpp;
> +	pipe_config->port_clock = limits->max_rate;
> +	pipe_config->lane_count = limits->max_lane_count;
> +	pipe_config->dsc.compressed_bpp =
> +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> +		      pipe_config->pipe_bpp);
> +
> +	return 0;
> +}
> +
>  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  				struct intel_crtc_state *pipe_config,
>  				struct drm_connector_state *conn_state,
> @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
>  		return -EINVAL;
>  
> +	/*
> +	 * compute pipe bpp is set to false for DP MST DSC case
> +	 * and compressed_bpp is calculated same time once
> +	 * vpci timeslots are allocated, because overall bpp
> +	 * calculation procedure is bit different for MST case.
> +	 */
>  	if (compute_pipe_bpp) {
>  		int pipe_bpp;
>  		int forced_bpp = intel_dp->force_dsc_bpc * 3;
> @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
>  				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
>  				 intel_dp->force_dsc_bpc);
> -
> -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> -								conn_state->max_requested_bpc);
> -
> -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
> -				return -EINVAL;
> +			if (intel_dp_is_edp(intel_dp))
> +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> +								     conn_state, limits);
> +			else
> +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> +								    conn_state, limits, timeslots);
> +			if (ret) {
> +				drm_dbg_kms(&dev_priv->drm,
> +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
> +				return ret;
>  			}
>  		}
> -
> -		pipe_config->pipe_bpp = pipe_bpp;
>  	}
>  
> -	/*
> -	 * For now enable DSC for max link rate, max lane count.
> -	 * Optimize this later for the minimum possible link rate/lane count
> -	 * with DSC enabled for the requested mode.
> -	 */
>  	pipe_config->port_clock = limits->max_rate;
>  	pipe_config->lane_count = limits->max_lane_count;
>  
> +	/* Calculate Slice count */
>  	if (intel_dp_is_edp(intel_dp)) {
> -		pipe_config->dsc.compressed_bpp =
> -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> -			      pipe_config->pipe_bpp);
>  		pipe_config->dsc.slice_count =
>  			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>  							true);
> @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  			return -EINVAL;
>  		}
>  	} else {
> -		u16 dsc_max_compressed_bpp = 0;
>  		u8 dsc_dp_slice_count;
>  
> -		if (compute_pipe_bpp) {
> -			dsc_max_compressed_bpp =
> -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> -								    pipe_config->port_clock,
> -								    pipe_config->lane_count,
> -								    adjusted_mode->crtc_clock,
> -								    adjusted_mode->crtc_hdisplay,
> -								    pipe_config->bigjoiner_pipes,
> -								    pipe_config->output_format,
> -								    pipe_config->pipe_bpp,
> -								    timeslots);
> -			/*
> -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> -			 * supported PPS value can be 63.9375 and with the further
> -			 * mention that bpp should be programmed double the target bpp
> -			 * restricting our target bpp to be 31.9375 at max
> -			 */
> -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> -				dsc_max_compressed_bpp = min_t(u16,
> -							       dsc_max_compressed_bpp,
> -							       31);
> -
> -			if (!dsc_max_compressed_bpp) {
> -				drm_dbg_kms(&dev_priv->drm,
> -					    "Compressed BPP not supported\n");
> -				return -EINVAL;
> -			}
> -		}
>  		dsc_dp_slice_count =
>  			intel_dp_dsc_get_slice_count(intel_dp,
>  						     adjusted_mode->crtc_clock,
> @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  			return -EINVAL;
>  		}
>  
> -		/*
> -		 * compute pipe bpp is set to false for DP MST DSC case
> -		 * and compressed_bpp is calculated same time once
> -		 * vpci timeslots are allocated, because overall bpp
> -		 * calculation procedure is bit different for MST case.
> -		 */
> -		if (compute_pipe_bpp) {
> -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> -							     pipe_config->pipe_bpp);
> -
> -			pipe_config->dsc.compressed_bpp = min_t(u16,
> -								dsc_max_compressed_bpp,
> -								output_bpp);
> -		}
>  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
>  	}
>  	/*
> -- 
> 2.25.1
> 

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Ville Syrjälä]

On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > compressed bpp possible. We then set the output bpp to this value.
> > This patch provides support to have max bpp, min rate and min lanes,
> > that can support the min compressed bpp.
> > 
> > v2:
> > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > -Fix the checks for limits->max/min_bpp while iterating over list of
> >  valid DSC bpcs. (Stan)
> > 
> > v3:
> > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > count calculation separately for different cases.
> > 
> > v4:
> > -Separate the pipe_bpp calculation for eDP and DP.
> > 
> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> >  1 file changed, 245 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 39e2bf3d738d..578320220c9a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> >  }
> >  
> > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > +				 const struct drm_display_mode *adjusted_mode)
> > +{
> > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > +
> > +	return mode_rate <= link_avail;
> > +}
> > +
> > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > +				   struct intel_crtc_state *pipe_config,
> > +				   struct link_config_limits *limits,
> > +				   int pipe_bpp,
> > +				   u16 compressed_bpp,
> > +				   int timeslots)
> > +{
> > +	const struct drm_display_mode *adjusted_mode =
> > +		&pipe_config->hw.adjusted_mode;
> > +	int link_rate, lane_count;
> > +	int dsc_max_bpp;
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	int i;
> > +
> > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > +			continue;
> > +
> > +		for (lane_count = limits->min_lane_count;
> > +		     lane_count <= limits->max_lane_count;
> > +		     lane_count <<= 1) {
> > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > +									  link_rate,
> > +									  lane_count,
> > +									  adjusted_mode->crtc_clock,
> > +									  adjusted_mode->crtc_hdisplay,
> > +									  pipe_config->bigjoiner_pipes,
> > +									  pipe_config->output_format,
> > +									  pipe_bpp, timeslots);
> > +			/*
> > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > +			 * supported PPS value can be 63.9375 and with the further
> > +			 * mention that bpp should be programmed double the target bpp
> > +			 * restricting our target bpp to be 31.9375 at max
> > +			 */
> > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > +
> > +			if (compressed_bpp > dsc_max_bpp)
> > +				continue;
> > +
> > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > +						  compressed_bpp, adjusted_mode))
> > +				continue;
> > +
> > +			pipe_config->lane_count = lane_count;
> > +			pipe_config->port_clock = link_rate;
> > +
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static
> > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > +					    struct intel_crtc_state *pipe_config,
> > +					    int bpc)
> > +{
> > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > +
> > +	if (max_bppx16)
> > +		return max_bppx16;
> > +	/*
> > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > +	 * values as given in spec Table 2-157 DP v2.0
> > +	 */
> > +	switch (pipe_config->output_format) {
> > +	case INTEL_OUTPUT_FORMAT_RGB:
> > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > +		return (3 * bpc) << 4;
> > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > +		return (3 * (bpc / 2)) << 4;
> > +	default:
> > +		MISSING_CASE(pipe_config->output_format);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > +{
> > +	switch (pipe_config->output_format) {
> > +	case INTEL_OUTPUT_FORMAT_RGB:
> > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > +		return 8 << 4;
> > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > +		return 6 << 4;
> > +	default:
> > +		MISSING_CASE(pipe_config->output_format);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > +				      struct intel_crtc_state *pipe_config,
> > +				      struct link_config_limits *limits,
> > +				      int pipe_bpp,
> > +				      int timeslots)
> > +{
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	u16 compressed_bpp;
> > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > +	int ret;
> > +
> > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > +	if (DISPLAY_VER(dev_priv) <= 12)
> > +		dsc_src_max_bpp = 23;
> > +	else
> > +		dsc_src_max_bpp = 27;
> 
> I would may be added some comment about what are those "23/27" numbers or
> may be even created some self-explanatory #define constants for those.

I dislike defines like that. They are single use so don't actually
do anything in terms of avoiding typoes and other accidental
mismatches, and people always seem put them in some random place
(eg. top of file) so then it takes extra work to find them.

The best approach IMO is to just use functions with good names.
Eg. in this case we could just have a full set of clear functions:
dsc_{sink,source}_{min,max}_bpp() or something along those line.

> Otherwise in a couple of years, even we ourselves might not be able to recall
> immediately where those numbers are taken from.
> 
> Otherwise looks good to me,
> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> > +	dsc_sink_max_bpp =
> > +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
> > +							pipe_config, pipe_bpp / 3) >> 4;
> > +
> > +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
> > +
> > +	/* Compressed BPP should be less than the Input DSC bpp */
> > +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
> > +
> > +	for (compressed_bpp = dsc_max_bpp;
> > +	     compressed_bpp >= dsc_min_bpp;
> > +	     compressed_bpp--) {
> > +		ret = dsc_compute_link_config(intel_dp,
> > +					      pipe_config,
> > +					      limits,
> > +					      pipe_bpp,
> > +					      compressed_bpp,
> > +					      timeslots);
> > +		if (ret == 0) {
> > +			pipe_config->dsc.compressed_bpp = compressed_bpp;
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > +					 struct intel_crtc_state *pipe_config,
> > +					 struct drm_connector_state *conn_state,
> > +					 struct link_config_limits *limits,
> > +					 int timeslots)
> > +{
> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +	u8 dsc_bpc[3] = {0};
> > +	u8 dsc_max_bpc, dsc_max_bpp;
> > +	u8 dsc_min_bpc, dsc_min_bpp;
> > +	u8 max_req_bpc = conn_state->max_requested_bpc;
> > +	int i, bpp, ret;
> > +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> > +							   dsc_bpc);
> > +
> > +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
> > +	if (DISPLAY_VER(i915) >= 12)
> > +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
> > +	else
> > +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
> > +
> > +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
> > +
> > +	/* Min DSC Input BPC for ICL+ is 8 */
> > +	dsc_min_bpc = 8;
> > +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
> > +
> > +	/*
> > +	 * Get the maximum DSC bpc that will be supported by any valid
> > +	 * link configuration and compressed bpp.
> > +	 */
> > +	for (i = 0; i < num_bpc; i++) {
> > +		bpp = dsc_bpc[i] * 3;
> > +
> > +		if (bpp < dsc_min_bpp)
> > +			break;
> > +
> > +		if (bpp > dsc_max_bpp)
> > +			continue;
> > +
> > +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
> > +						 limits, bpp, timeslots);
> > +		if (ret == 0) {
> > +			pipe_config->pipe_bpp = bpp;
> > +
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static
> >  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> >  {
> > @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> >  	return (pipe_bpp < 8 * 3);
> >  }
> >  
> > +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > +					  struct intel_crtc_state *pipe_config,
> > +					  struct drm_connector_state *conn_state,
> > +					  struct link_config_limits *limits)
> > +{
> > +	/* For eDP use max bpp that can be supported with DSC. */
> > +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > +						    conn_state->max_requested_bpc);
> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +
> > +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > +		drm_dbg_kms(&i915->drm,
> > +			    "No DSC support for less than 8bpc\n");
> > +		return -EINVAL;
> > +	}
> > +	pipe_config->pipe_bpp = pipe_bpp;
> > +	pipe_config->port_clock = limits->max_rate;
> > +	pipe_config->lane_count = limits->max_lane_count;
> > +	pipe_config->dsc.compressed_bpp =
> > +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > +		      pipe_config->pipe_bpp);
> > +
> > +	return 0;
> > +}
> > +
> >  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  				struct intel_crtc_state *pipe_config,
> >  				struct drm_connector_state *conn_state,
> > @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> >  		return -EINVAL;
> >  
> > +	/*
> > +	 * compute pipe bpp is set to false for DP MST DSC case
> > +	 * and compressed_bpp is calculated same time once
> > +	 * vpci timeslots are allocated, because overall bpp
> > +	 * calculation procedure is bit different for MST case.
> > +	 */
> >  	if (compute_pipe_bpp) {
> >  		int pipe_bpp;
> >  		int forced_bpp = intel_dp->force_dsc_bpc * 3;
> > @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
> >  				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
> >  				 intel_dp->force_dsc_bpc);
> > -
> > -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > -								conn_state->max_requested_bpc);
> > -
> > -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
> > -				return -EINVAL;
> > +			if (intel_dp_is_edp(intel_dp))
> > +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > +								     conn_state, limits);
> > +			else
> > +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > +								    conn_state, limits, timeslots);
> > +			if (ret) {
> > +				drm_dbg_kms(&dev_priv->drm,
> > +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
> > +				return ret;
> >  			}
> >  		}
> > -
> > -		pipe_config->pipe_bpp = pipe_bpp;
> >  	}
> >  
> > -	/*
> > -	 * For now enable DSC for max link rate, max lane count.
> > -	 * Optimize this later for the minimum possible link rate/lane count
> > -	 * with DSC enabled for the requested mode.
> > -	 */
> >  	pipe_config->port_clock = limits->max_rate;
> >  	pipe_config->lane_count = limits->max_lane_count;
> >  
> > +	/* Calculate Slice count */
> >  	if (intel_dp_is_edp(intel_dp)) {
> > -		pipe_config->dsc.compressed_bpp =
> > -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > -			      pipe_config->pipe_bpp);
> >  		pipe_config->dsc.slice_count =
> >  			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> >  							true);
> > @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  			return -EINVAL;
> >  		}
> >  	} else {
> > -		u16 dsc_max_compressed_bpp = 0;
> >  		u8 dsc_dp_slice_count;
> >  
> > -		if (compute_pipe_bpp) {
> > -			dsc_max_compressed_bpp =
> > -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > -								    pipe_config->port_clock,
> > -								    pipe_config->lane_count,
> > -								    adjusted_mode->crtc_clock,
> > -								    adjusted_mode->crtc_hdisplay,
> > -								    pipe_config->bigjoiner_pipes,
> > -								    pipe_config->output_format,
> > -								    pipe_config->pipe_bpp,
> > -								    timeslots);
> > -			/*
> > -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > -			 * supported PPS value can be 63.9375 and with the further
> > -			 * mention that bpp should be programmed double the target bpp
> > -			 * restricting our target bpp to be 31.9375 at max
> > -			 */
> > -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > -				dsc_max_compressed_bpp = min_t(u16,
> > -							       dsc_max_compressed_bpp,
> > -							       31);
> > -
> > -			if (!dsc_max_compressed_bpp) {
> > -				drm_dbg_kms(&dev_priv->drm,
> > -					    "Compressed BPP not supported\n");
> > -				return -EINVAL;
> > -			}
> > -		}
> >  		dsc_dp_slice_count =
> >  			intel_dp_dsc_get_slice_count(intel_dp,
> >  						     adjusted_mode->crtc_clock,
> > @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  			return -EINVAL;
> >  		}
> >  
> > -		/*
> > -		 * compute pipe bpp is set to false for DP MST DSC case
> > -		 * and compressed_bpp is calculated same time once
> > -		 * vpci timeslots are allocated, because overall bpp
> > -		 * calculation procedure is bit different for MST case.
> > -		 */
> > -		if (compute_pipe_bpp) {
> > -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> > -							     pipe_config->pipe_bpp);
> > -
> > -			pipe_config->dsc.compressed_bpp = min_t(u16,
> > -								dsc_max_compressed_bpp,
> > -								output_bpp);
> > -		}
> >  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
> >  	}
> >  	/*
> > -- 
> > 2.25.1
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 05/13] drm/i915/intel_cdclk: Add vdsc with bigjoiner constraints on min_cdlck

[Nautiyal, Ankit K]

Thanks Ville and Stan for the comments.

I agree with the changes in _plane_min_cdclk and 
intel_pixel_rate_to_cdclk regarding PPC.

But I am a little confused for about the pixel clock.

Please find my comments inline:


On 5/16/2023 3:41 PM, Lisovskiy, Stanislav wrote:
> On Mon, May 15, 2023 at 05:44:51PM +0300, Ville Syrjälä wrote:
>> On Fri, May 12, 2023 at 11:54:09AM +0530, Ankit Nautiyal wrote:
>>> As per Bsepc:49259, Bigjoiner BW check puts restriction on the
>>> compressed bpp for a given CDCLK, pixelclock in cases where
>>> Bigjoiner + DSC are used.
>>>
>>> Currently compressed bpp is computed first, and it is ensured that
>>> the bpp will work at least with the max CDCLK freq.
>>>
>>> Since the CDCLK is computed later, lets account for Bigjoiner BW
>>> check while calculating Min CDCLK.
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_cdclk.c | 49 ++++++++++++++++++----
>>>   1 file changed, 42 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>> index 6bed75f1541a..3532640c5027 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>> @@ -2520,6 +2520,46 @@ static int intel_planes_min_cdclk(const struct intel_crtc_state *crtc_state)
>>>   	return min_cdclk;
>>>   }
>>>   
>>> +static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
>>> +{
>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>>> +	int min_cdclk = 0;
>>> +
>>> +	/*
>>> +	 * When we decide to use only one VDSC engine, since
>>> +	 * each VDSC operates with 1 ppc throughput, pixel clock
>>> +	 * cannot be higher than the VDSC clock (cdclk)
>>> +	 */
>>> +	if (!crtc_state->dsc.dsc_split)
>>> +		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
>>> +
>>> +	if (crtc_state->bigjoiner_pipes) {
>>> +		/*
>>> +		 * According to Bigjoiner bw check:
>>> +		 * compressed_bpp <= PPC * CDCLK * Big joiner Interface bits / Pixel clock
>>> +		 *
>>> +		 * We have already computed compressed_bpp, so now compute the min CDCLK that
>>> +		 * is required to support this compressed_bpp.
>>> +		 *
>>> +		 * => CDCLK >= compressed_bpp * Pixel clock / (PPC * Bigjoiner Interface bits)
>>> +		 *
>>> +		 * Since Num of pipes joined = 2, and PPC = 2 with bigjoiner
>>> +		 * => CDCLK >= compressed_bpp * pixel_rate  / Bigjoiner Interface bits
>>> +		 *
>>> +		 * #TODO Bspec mentions to account for FEC overhead while using pixel clock.
>>> +		 * Check if we need to use FEC overhead in the above calculations.
>>> +		 */
>>> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
>>> +		int min_cdclk_bj = crtc_state->dsc.compressed_bpp * crtc_state->pixel_rate /
>>> +				   bigjoiner_interface_bits;
>> pixel_rate is the downscale adjusted thing, so it doesn't seem
>> like the correct thing to use here.
>>
>> Hmm. Assuming that the single VDSC engine really throttles the entire
>> pipe to 1 PPC then we should probably account for the 1 vs. 2 PPC
>> difference in *_plane_min_cdclk() and intel_pixel_rate_to_cdclk()
>> directly. Currently all of those assume 2 PPC.

Hmm alright,  I do see in plane_min_cdclk and intel_pixel_rate_to_cdclk 
we assume 2 PPC.

So I can add a check for the dsc_split and use 1 PPC/2PPC  in the two 
functions as a separate patch perhaps.


> Main thing is to properly align that one you propose above with that check,
> where we decide how many VDSC engines to use:
>
>          /*
>           * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
>           * is greater than the maximum Cdclock and if slice count is even
>           * then we need to use 2 VDSC instances.
>           */
>          if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
>                  if (pipe_config->dsc.slice_count > 1) {
>                          pipe_config->dsc.dsc_split = true;
>                  } else {
>                          drm_dbg_kms(&dev_priv->drm,
>                                      "Cannot split stream to use 2 VDSC instances\n");
>                          return -EINVAL;
>                  }
>          }
>
> Otherwise I agree that we should do that check preferrably in *_plane_min_cdclk
> and use plane data rate which is adjusted after scaling is applied(I think we even have correspondent function there)
> It is strange that scaling wasn't mentioned in BSpec formula.
> I would also say that we should account for number of slices(i.e VDSC engines) now only in Bigjoiner case, but always, as I understand that number can be different not only for Bigjoiner cases.
>
> Stan
>
Hmm does it mean:

if (!crtc_state->dsc.dsc_split) {

         if (bigjoiner)

             min_cdclk = compressed_bpp * Pixel clock / (PPC * Bigjoiner 
Interface bits);

     else

             min_cdclk = compressed_bpp * Pixel clock;

}

For Pixel clock, should it not be crtc_state->hw.adjusted_mode->clock ?

Regards,

Ankit


>>> +
>>> +		min_cdclk = max(min_cdclk, min_cdclk_bj);
>>> +	}
>>> +
>>> +	return min_cdclk;
>>> +}
>>> +
>>>   int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>>>   {
>>>   	struct drm_i915_private *dev_priv =
>>> @@ -2591,13 +2631,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>>>   	/* Account for additional needs from the planes */
>>>   	min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
>>>   
>>> -	/*
>>> -	 * When we decide to use only one VDSC engine, since
>>> -	 * each VDSC operates with 1 ppc throughput, pixel clock
>>> -	 * cannot be higher than the VDSC clock (cdclk)
>>> -	 */
>>> -	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
>>> -		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
>>> +	if (crtc_state->dsc.compression_enable)
>>> +		min_cdclk = max(min_cdclk, intel_vdsc_min_cdclk(crtc_state));
>>>   
>>>   	/*
>>>   	 * HACK. Currently for TGL/DG2 platforms we calculate
>>> -- 
>>> 2.25.1
>> -- 
>> Ville Syrjälä
>> Intel

Re: [PATCH 04/13] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Nautiyal, Ankit K]

On 5/15/2023 7:21 PM, Ville Syrjälä wrote:
> On Fri, May 12, 2023 at 11:54:08AM +0530, Ankit Nautiyal wrote:
>> In Bigjoiner check for DSC, bigjoiner interface bits for DP for
>> DISPLAY > 13 is 36 (Bspec: 49259).
>>
>> v2: Corrected Display ver to 13.
>>
>> v3: Follow convention for conditional statement. (Ville)
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 24de25551a49..bca80c0793e9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -783,8 +783,9 @@ u16 intel_dp_dsc_get_max_compressed_bpp(struct drm_i915_private *i915,
>>   	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
>>   
>>   	if (bigjoiner) {
>> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
> 'x >= 14' is the usual convention.
>
> with that
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks Ville for the reviews.

Will fix the check in next version of the patch.

Regards,

Ankit

>
>>   		u32 max_bpp_bigjoiner =
>> -			i915->display.cdclk.max_cdclk_freq * 48 /
>> +			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>>   			intel_dp_mode_to_fec_clock(mode_clock);
>>   
>>   		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>> -- 
>> 2.25.1

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Nautiyal, Ankit K]

Thanks Stan and Ville for the review comments.

I agree can have some documentation about the values used, instead of 
magic numbers.

Also, Ville's approach for dsc_{sink,source}_{min,max}_bpp() seems good, 
and that can be used as helpers in MST case too.

Will add the changes in the next version of the patch.


Thanks & Regards,

Ankit


On 5/16/2023 5:10 PM, Ville Syrjälä wrote:
> On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
>> On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
>>> Currently, we take the max lane, rate and pipe bpp, to get the maximum
>>> compressed bpp possible. We then set the output bpp to this value.
>>> This patch provides support to have max bpp, min rate and min lanes,
>>> that can support the min compressed bpp.
>>>
>>> v2:
>>> -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
>>> -Fix the checks for limits->max/min_bpp while iterating over list of
>>>   valid DSC bpcs. (Stan)
>>>
>>> v3:
>>> -Refactor the code to have pipe bpp/compressed bpp computation and slice
>>> count calculation separately for different cases.
>>>
>>> v4:
>>> -Separate the pipe_bpp calculation for eDP and DP.
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
>>>   1 file changed, 245 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index 39e2bf3d738d..578320220c9a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
>>>   	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
>>>   }
>>>   
>>> +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
>>> +				 const struct drm_display_mode *adjusted_mode)
>>> +{
>>> +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
>>> +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
>>> +
>>> +	return mode_rate <= link_avail;
>>> +}
>>> +
>>> +static int dsc_compute_link_config(struct intel_dp *intel_dp,
>>> +				   struct intel_crtc_state *pipe_config,
>>> +				   struct link_config_limits *limits,
>>> +				   int pipe_bpp,
>>> +				   u16 compressed_bpp,
>>> +				   int timeslots)
>>> +{
>>> +	const struct drm_display_mode *adjusted_mode =
>>> +		&pipe_config->hw.adjusted_mode;
>>> +	int link_rate, lane_count;
>>> +	int dsc_max_bpp;
>>> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < intel_dp->num_common_rates; i++) {
>>> +		link_rate = intel_dp_common_rate(intel_dp, i);
>>> +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
>>> +			continue;
>>> +
>>> +		for (lane_count = limits->min_lane_count;
>>> +		     lane_count <= limits->max_lane_count;
>>> +		     lane_count <<= 1) {
>>> +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
>>> +									  link_rate,
>>> +									  lane_count,
>>> +									  adjusted_mode->crtc_clock,
>>> +									  adjusted_mode->crtc_hdisplay,
>>> +									  pipe_config->bigjoiner_pipes,
>>> +									  pipe_config->output_format,
>>> +									  pipe_bpp, timeslots);
>>> +			/*
>>> +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
>>> +			 * supported PPS value can be 63.9375 and with the further
>>> +			 * mention that bpp should be programmed double the target bpp
>>> +			 * restricting our target bpp to be 31.9375 at max
>>> +			 */
>>> +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
>>> +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
>>> +
>>> +			if (compressed_bpp > dsc_max_bpp)
>>> +				continue;
>>> +
>>> +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
>>> +						  compressed_bpp, adjusted_mode))
>>> +				continue;
>>> +
>>> +			pipe_config->lane_count = lane_count;
>>> +			pipe_config->port_clock = link_rate;
>>> +
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static
>>> +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
>>> +					    struct intel_crtc_state *pipe_config,
>>> +					    int bpc)
>>> +{
>>> +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
>>> +
>>> +	if (max_bppx16)
>>> +		return max_bppx16;
>>> +	/*
>>> +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
>>> +	 * values as given in spec Table 2-157 DP v2.0
>>> +	 */
>>> +	switch (pipe_config->output_format) {
>>> +	case INTEL_OUTPUT_FORMAT_RGB:
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
>>> +		return (3 * bpc) << 4;
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
>>> +		return (3 * (bpc / 2)) << 4;
>>> +	default:
>>> +		MISSING_CASE(pipe_config->output_format);
>>> +		break;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
>>> +{
>>> +	switch (pipe_config->output_format) {
>>> +	case INTEL_OUTPUT_FORMAT_RGB:
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
>>> +		return 8 << 4;
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
>>> +		return 6 << 4;
>>> +	default:
>>> +		MISSING_CASE(pipe_config->output_format);
>>> +		break;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
>>> +				      struct intel_crtc_state *pipe_config,
>>> +				      struct link_config_limits *limits,
>>> +				      int pipe_bpp,
>>> +				      int timeslots)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>> +	u16 compressed_bpp;
>>> +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
>>> +	int ret;
>>> +
>>> +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
>>> +	if (DISPLAY_VER(dev_priv) <= 12)
>>> +		dsc_src_max_bpp = 23;
>>> +	else
>>> +		dsc_src_max_bpp = 27;
>> I would may be added some comment about what are those "23/27" numbers or
>> may be even created some self-explanatory #define constants for those.
> I dislike defines like that. They are single use so don't actually
> do anything in terms of avoiding typoes and other accidental
> mismatches, and people always seem put them in some random place
> (eg. top of file) so then it takes extra work to find them.
>
> The best approach IMO is to just use functions with good names.
> Eg. in this case we could just have a full set of clear functions:
> dsc_{sink,source}_{min,max}_bpp() or something along those line.
>
>> Otherwise in a couple of years, even we ourselves might not be able to recall
>> immediately where those numbers are taken from.
>>
>> Otherwise looks good to me,
>>
>> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>>
>>> +	dsc_sink_max_bpp =
>>> +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
>>> +							pipe_config, pipe_bpp / 3) >> 4;
>>> +
>>> +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
>>> +
>>> +	/* Compressed BPP should be less than the Input DSC bpp */
>>> +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
>>> +
>>> +	for (compressed_bpp = dsc_max_bpp;
>>> +	     compressed_bpp >= dsc_min_bpp;
>>> +	     compressed_bpp--) {
>>> +		ret = dsc_compute_link_config(intel_dp,
>>> +					      pipe_config,
>>> +					      limits,
>>> +					      pipe_bpp,
>>> +					      compressed_bpp,
>>> +					      timeslots);
>>> +		if (ret == 0) {
>>> +			pipe_config->dsc.compressed_bpp = compressed_bpp;
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
>>> +					 struct intel_crtc_state *pipe_config,
>>> +					 struct drm_connector_state *conn_state,
>>> +					 struct link_config_limits *limits,
>>> +					 int timeslots)
>>> +{
>>> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> +	u8 dsc_bpc[3] = {0};
>>> +	u8 dsc_max_bpc, dsc_max_bpp;
>>> +	u8 dsc_min_bpc, dsc_min_bpp;
>>> +	u8 max_req_bpc = conn_state->max_requested_bpc;
>>> +	int i, bpp, ret;
>>> +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
>>> +							   dsc_bpc);
>>> +
>>> +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
>>> +	if (DISPLAY_VER(i915) >= 12)
>>> +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
>>> +	else
>>> +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
>>> +
>>> +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
>>> +
>>> +	/* Min DSC Input BPC for ICL+ is 8 */
>>> +	dsc_min_bpc = 8;
>>> +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
>>> +
>>> +	/*
>>> +	 * Get the maximum DSC bpc that will be supported by any valid
>>> +	 * link configuration and compressed bpp.
>>> +	 */
>>> +	for (i = 0; i < num_bpc; i++) {
>>> +		bpp = dsc_bpc[i] * 3;
>>> +
>>> +		if (bpp < dsc_min_bpp)
>>> +			break;
>>> +
>>> +		if (bpp > dsc_max_bpp)
>>> +			continue;
>>> +
>>> +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
>>> +						 limits, bpp, timeslots);
>>> +		if (ret == 0) {
>>> +			pipe_config->pipe_bpp = bpp;
>>> +
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>>   static
>>>   bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>>>   {
>>> @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>>>   	return (pipe_bpp < 8 * 3);
>>>   }
>>>   
>>> +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
>>> +					  struct intel_crtc_state *pipe_config,
>>> +					  struct drm_connector_state *conn_state,
>>> +					  struct link_config_limits *limits)
>>> +{
>>> +	/* For eDP use max bpp that can be supported with DSC. */
>>> +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
>>> +						    conn_state->max_requested_bpc);
>>> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> +
>>> +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
>>> +		drm_dbg_kms(&i915->drm,
>>> +			    "No DSC support for less than 8bpc\n");
>>> +		return -EINVAL;
>>> +	}
>>> +	pipe_config->pipe_bpp = pipe_bpp;
>>> +	pipe_config->port_clock = limits->max_rate;
>>> +	pipe_config->lane_count = limits->max_lane_count;
>>> +	pipe_config->dsc.compressed_bpp =
>>> +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
>>> +		      pipe_config->pipe_bpp);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   				struct intel_crtc_state *pipe_config,
>>>   				struct drm_connector_state *conn_state,
>>> @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
>>>   		return -EINVAL;
>>>   
>>> +	/*
>>> +	 * compute pipe bpp is set to false for DP MST DSC case
>>> +	 * and compressed_bpp is calculated same time once
>>> +	 * vpci timeslots are allocated, because overall bpp
>>> +	 * calculation procedure is bit different for MST case.
>>> +	 */
>>>   	if (compute_pipe_bpp) {
>>>   		int pipe_bpp;
>>>   		int forced_bpp = intel_dp->force_dsc_bpc * 3;
>>> @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
>>>   				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
>>>   				 intel_dp->force_dsc_bpc);
>>> -
>>> -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
>>> -								conn_state->max_requested_bpc);
>>> -
>>> -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
>>> -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
>>> -				return -EINVAL;
>>> +			if (intel_dp_is_edp(intel_dp))
>>> +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
>>> +								     conn_state, limits);
>>> +			else
>>> +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
>>> +								    conn_state, limits, timeslots);
>>> +			if (ret) {
>>> +				drm_dbg_kms(&dev_priv->drm,
>>> +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
>>> +				return ret;
>>>   			}
>>>   		}
>>> -
>>> -		pipe_config->pipe_bpp = pipe_bpp;
>>>   	}
>>>   
>>> -	/*
>>> -	 * For now enable DSC for max link rate, max lane count.
>>> -	 * Optimize this later for the minimum possible link rate/lane count
>>> -	 * with DSC enabled for the requested mode.
>>> -	 */
>>>   	pipe_config->port_clock = limits->max_rate;
>>>   	pipe_config->lane_count = limits->max_lane_count;
>>>   
>>> +	/* Calculate Slice count */
>>>   	if (intel_dp_is_edp(intel_dp)) {
>>> -		pipe_config->dsc.compressed_bpp =
>>> -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
>>> -			      pipe_config->pipe_bpp);
>>>   		pipe_config->dsc.slice_count =
>>>   			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>>>   							true);
>>> @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   			return -EINVAL;
>>>   		}
>>>   	} else {
>>> -		u16 dsc_max_compressed_bpp = 0;
>>>   		u8 dsc_dp_slice_count;
>>>   
>>> -		if (compute_pipe_bpp) {
>>> -			dsc_max_compressed_bpp =
>>> -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
>>> -								    pipe_config->port_clock,
>>> -								    pipe_config->lane_count,
>>> -								    adjusted_mode->crtc_clock,
>>> -								    adjusted_mode->crtc_hdisplay,
>>> -								    pipe_config->bigjoiner_pipes,
>>> -								    pipe_config->output_format,
>>> -								    pipe_config->pipe_bpp,
>>> -								    timeslots);
>>> -			/*
>>> -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
>>> -			 * supported PPS value can be 63.9375 and with the further
>>> -			 * mention that bpp should be programmed double the target bpp
>>> -			 * restricting our target bpp to be 31.9375 at max
>>> -			 */
>>> -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
>>> -				dsc_max_compressed_bpp = min_t(u16,
>>> -							       dsc_max_compressed_bpp,
>>> -							       31);
>>> -
>>> -			if (!dsc_max_compressed_bpp) {
>>> -				drm_dbg_kms(&dev_priv->drm,
>>> -					    "Compressed BPP not supported\n");
>>> -				return -EINVAL;
>>> -			}
>>> -		}
>>>   		dsc_dp_slice_count =
>>>   			intel_dp_dsc_get_slice_count(intel_dp,
>>>   						     adjusted_mode->crtc_clock,
>>> @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   			return -EINVAL;
>>>   		}
>>>   
>>> -		/*
>>> -		 * compute pipe bpp is set to false for DP MST DSC case
>>> -		 * and compressed_bpp is calculated same time once
>>> -		 * vpci timeslots are allocated, because overall bpp
>>> -		 * calculation procedure is bit different for MST case.
>>> -		 */
>>> -		if (compute_pipe_bpp) {
>>> -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
>>> -							     pipe_config->pipe_bpp);
>>> -
>>> -			pipe_config->dsc.compressed_bpp = min_t(u16,
>>> -								dsc_max_compressed_bpp,
>>> -								output_bpp);
>>> -		}
>>>   		pipe_config->dsc.slice_count = dsc_dp_slice_count;
>>>   	}
>>>   	/*
>>> -- 
>>> 2.25.1
>>>

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Lisovskiy, Stanislav]

On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > compressed bpp possible. We then set the output bpp to this value.
> > > This patch provides support to have max bpp, min rate and min lanes,
> > > that can support the min compressed bpp.
> > > 
> > > v2:
> > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > >  valid DSC bpcs. (Stan)
> > > 
> > > v3:
> > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > count calculation separately for different cases.
> > > 
> > > v4:
> > > -Separate the pipe_bpp calculation for eDP and DP.
> > > 
> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 39e2bf3d738d..578320220c9a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > >  }
> > >  
> > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > +				 const struct drm_display_mode *adjusted_mode)
> > > +{
> > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > +
> > > +	return mode_rate <= link_avail;
> > > +}
> > > +
> > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > +				   struct intel_crtc_state *pipe_config,
> > > +				   struct link_config_limits *limits,
> > > +				   int pipe_bpp,
> > > +				   u16 compressed_bpp,
> > > +				   int timeslots)
> > > +{
> > > +	const struct drm_display_mode *adjusted_mode =
> > > +		&pipe_config->hw.adjusted_mode;
> > > +	int link_rate, lane_count;
> > > +	int dsc_max_bpp;
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	int i;
> > > +
> > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > +			continue;
> > > +
> > > +		for (lane_count = limits->min_lane_count;
> > > +		     lane_count <= limits->max_lane_count;
> > > +		     lane_count <<= 1) {
> > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > +									  link_rate,
> > > +									  lane_count,
> > > +									  adjusted_mode->crtc_clock,
> > > +									  adjusted_mode->crtc_hdisplay,
> > > +									  pipe_config->bigjoiner_pipes,
> > > +									  pipe_config->output_format,
> > > +									  pipe_bpp, timeslots);
> > > +			/*
> > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > +			 * supported PPS value can be 63.9375 and with the further
> > > +			 * mention that bpp should be programmed double the target bpp
> > > +			 * restricting our target bpp to be 31.9375 at max
> > > +			 */
> > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > +
> > > +			if (compressed_bpp > dsc_max_bpp)
> > > +				continue;
> > > +
> > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > +						  compressed_bpp, adjusted_mode))
> > > +				continue;
> > > +
> > > +			pipe_config->lane_count = lane_count;
> > > +			pipe_config->port_clock = link_rate;
> > > +
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static
> > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > +					    struct intel_crtc_state *pipe_config,
> > > +					    int bpc)
> > > +{
> > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > +
> > > +	if (max_bppx16)
> > > +		return max_bppx16;
> > > +	/*
> > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > +	 * values as given in spec Table 2-157 DP v2.0
> > > +	 */
> > > +	switch (pipe_config->output_format) {
> > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > +		return (3 * bpc) << 4;
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > +		return (3 * (bpc / 2)) << 4;
> > > +	default:
> > > +		MISSING_CASE(pipe_config->output_format);
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > +{
> > > +	switch (pipe_config->output_format) {
> > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > +		return 8 << 4;
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > +		return 6 << 4;
> > > +	default:
> > > +		MISSING_CASE(pipe_config->output_format);
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > +				      struct intel_crtc_state *pipe_config,
> > > +				      struct link_config_limits *limits,
> > > +				      int pipe_bpp,
> > > +				      int timeslots)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	u16 compressed_bpp;
> > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > +	int ret;
> > > +
> > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > +		dsc_src_max_bpp = 23;
> > > +	else
> > > +		dsc_src_max_bpp = 27;
> > 
> > I would may be added some comment about what are those "23/27" numbers or
> > may be even created some self-explanatory #define constants for those.
> 
> I dislike defines like that. They are single use so don't actually
> do anything in terms of avoiding typoes and other accidental
> mismatches, and people always seem put them in some random place
> (eg. top of file) so then it takes extra work to find them.

Ah come on, even my primitive mcedit with ctags plugin can track it :))
However my point is that anything is better than just hard-coded magic
numbers, which is proven antipattern.
Also you never know if it is a single or multiple use, I think it should be
either defined as a constant or as a define, which is self explanatory.

> 
> The best approach IMO is to just use functions with good names.
> Eg. in this case we could just have a full set of clear functions:
> dsc_{sink,source}_{min,max}_bpp() or something along those line.

..which still doesn't explain, why it is 23 there, why it is 27, who sets those
numbers, which spec and so on.

Stan

> 
> > Otherwise in a couple of years, even we ourselves might not be able to recall
> > immediately where those numbers are taken from.
> > 
> > Otherwise looks good to me,
> > 
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > 
> > > +	dsc_sink_max_bpp =
> > > +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
> > > +							pipe_config, pipe_bpp / 3) >> 4;
> > > +
> > > +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
> > > +
> > > +	/* Compressed BPP should be less than the Input DSC bpp */
> > > +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
> > > +
> > > +	for (compressed_bpp = dsc_max_bpp;
> > > +	     compressed_bpp >= dsc_min_bpp;
> > > +	     compressed_bpp--) {
> > > +		ret = dsc_compute_link_config(intel_dp,
> > > +					      pipe_config,
> > > +					      limits,
> > > +					      pipe_bpp,
> > > +					      compressed_bpp,
> > > +					      timeslots);
> > > +		if (ret == 0) {
> > > +			pipe_config->dsc.compressed_bpp = compressed_bpp;
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > > +					 struct intel_crtc_state *pipe_config,
> > > +					 struct drm_connector_state *conn_state,
> > > +					 struct link_config_limits *limits,
> > > +					 int timeslots)
> > > +{
> > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > +	u8 dsc_bpc[3] = {0};
> > > +	u8 dsc_max_bpc, dsc_max_bpp;
> > > +	u8 dsc_min_bpc, dsc_min_bpp;
> > > +	u8 max_req_bpc = conn_state->max_requested_bpc;
> > > +	int i, bpp, ret;
> > > +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> > > +							   dsc_bpc);
> > > +
> > > +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
> > > +	if (DISPLAY_VER(i915) >= 12)
> > > +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
> > > +	else
> > > +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
> > > +
> > > +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
> > > +
> > > +	/* Min DSC Input BPC for ICL+ is 8 */
> > > +	dsc_min_bpc = 8;
> > > +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
> > > +
> > > +	/*
> > > +	 * Get the maximum DSC bpc that will be supported by any valid
> > > +	 * link configuration and compressed bpp.
> > > +	 */
> > > +	for (i = 0; i < num_bpc; i++) {
> > > +		bpp = dsc_bpc[i] * 3;
> > > +
> > > +		if (bpp < dsc_min_bpp)
> > > +			break;
> > > +
> > > +		if (bpp > dsc_max_bpp)
> > > +			continue;
> > > +
> > > +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
> > > +						 limits, bpp, timeslots);
> > > +		if (ret == 0) {
> > > +			pipe_config->pipe_bpp = bpp;
> > > +
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  static
> > >  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> > >  {
> > > @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> > >  	return (pipe_bpp < 8 * 3);
> > >  }
> > >  
> > > +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > > +					  struct intel_crtc_state *pipe_config,
> > > +					  struct drm_connector_state *conn_state,
> > > +					  struct link_config_limits *limits)
> > > +{
> > > +	/* For eDP use max bpp that can be supported with DSC. */
> > > +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > > +						    conn_state->max_requested_bpc);
> > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > +
> > > +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > > +		drm_dbg_kms(&i915->drm,
> > > +			    "No DSC support for less than 8bpc\n");
> > > +		return -EINVAL;
> > > +	}
> > > +	pipe_config->pipe_bpp = pipe_bpp;
> > > +	pipe_config->port_clock = limits->max_rate;
> > > +	pipe_config->lane_count = limits->max_lane_count;
> > > +	pipe_config->dsc.compressed_bpp =
> > > +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > > +		      pipe_config->pipe_bpp);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  				struct intel_crtc_state *pipe_config,
> > >  				struct drm_connector_state *conn_state,
> > > @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> > >  		return -EINVAL;
> > >  
> > > +	/*
> > > +	 * compute pipe bpp is set to false for DP MST DSC case
> > > +	 * and compressed_bpp is calculated same time once
> > > +	 * vpci timeslots are allocated, because overall bpp
> > > +	 * calculation procedure is bit different for MST case.
> > > +	 */
> > >  	if (compute_pipe_bpp) {
> > >  		int pipe_bpp;
> > >  		int forced_bpp = intel_dp->force_dsc_bpc * 3;
> > > @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
> > >  				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
> > >  				 intel_dp->force_dsc_bpc);
> > > -
> > > -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > > -								conn_state->max_requested_bpc);
> > > -
> > > -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > > -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
> > > -				return -EINVAL;
> > > +			if (intel_dp_is_edp(intel_dp))
> > > +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > > +								     conn_state, limits);
> > > +			else
> > > +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > > +								    conn_state, limits, timeslots);
> > > +			if (ret) {
> > > +				drm_dbg_kms(&dev_priv->drm,
> > > +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
> > > +				return ret;
> > >  			}
> > >  		}
> > > -
> > > -		pipe_config->pipe_bpp = pipe_bpp;
> > >  	}
> > >  
> > > -	/*
> > > -	 * For now enable DSC for max link rate, max lane count.
> > > -	 * Optimize this later for the minimum possible link rate/lane count
> > > -	 * with DSC enabled for the requested mode.
> > > -	 */
> > >  	pipe_config->port_clock = limits->max_rate;
> > >  	pipe_config->lane_count = limits->max_lane_count;
> > >  
> > > +	/* Calculate Slice count */
> > >  	if (intel_dp_is_edp(intel_dp)) {
> > > -		pipe_config->dsc.compressed_bpp =
> > > -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > > -			      pipe_config->pipe_bpp);
> > >  		pipe_config->dsc.slice_count =
> > >  			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> > >  							true);
> > > @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  			return -EINVAL;
> > >  		}
> > >  	} else {
> > > -		u16 dsc_max_compressed_bpp = 0;
> > >  		u8 dsc_dp_slice_count;
> > >  
> > > -		if (compute_pipe_bpp) {
> > > -			dsc_max_compressed_bpp =
> > > -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > -								    pipe_config->port_clock,
> > > -								    pipe_config->lane_count,
> > > -								    adjusted_mode->crtc_clock,
> > > -								    adjusted_mode->crtc_hdisplay,
> > > -								    pipe_config->bigjoiner_pipes,
> > > -								    pipe_config->output_format,
> > > -								    pipe_config->pipe_bpp,
> > > -								    timeslots);
> > > -			/*
> > > -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > -			 * supported PPS value can be 63.9375 and with the further
> > > -			 * mention that bpp should be programmed double the target bpp
> > > -			 * restricting our target bpp to be 31.9375 at max
> > > -			 */
> > > -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > -				dsc_max_compressed_bpp = min_t(u16,
> > > -							       dsc_max_compressed_bpp,
> > > -							       31);
> > > -
> > > -			if (!dsc_max_compressed_bpp) {
> > > -				drm_dbg_kms(&dev_priv->drm,
> > > -					    "Compressed BPP not supported\n");
> > > -				return -EINVAL;
> > > -			}
> > > -		}
> > >  		dsc_dp_slice_count =
> > >  			intel_dp_dsc_get_slice_count(intel_dp,
> > >  						     adjusted_mode->crtc_clock,
> > > @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  			return -EINVAL;
> > >  		}
> > >  
> > > -		/*
> > > -		 * compute pipe bpp is set to false for DP MST DSC case
> > > -		 * and compressed_bpp is calculated same time once
> > > -		 * vpci timeslots are allocated, because overall bpp
> > > -		 * calculation procedure is bit different for MST case.
> > > -		 */
> > > -		if (compute_pipe_bpp) {
> > > -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> > > -							     pipe_config->pipe_bpp);
> > > -
> > > -			pipe_config->dsc.compressed_bpp = min_t(u16,
> > > -								dsc_max_compressed_bpp,
> > > -								output_bpp);
> > > -		}
> > >  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
> > >  	}
> > >  	/*
> > > -- 
> > > 2.25.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Ville Syrjälä]

On Tue, May 23, 2023 at 12:01:34PM +0300, Lisovskiy, Stanislav wrote:
> On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> > On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > > compressed bpp possible. We then set the output bpp to this value.
> > > > This patch provides support to have max bpp, min rate and min lanes,
> > > > that can support the min compressed bpp.
> > > > 
> > > > v2:
> > > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > > >  valid DSC bpcs. (Stan)
> > > > 
> > > > v3:
> > > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > > count calculation separately for different cases.
> > > > 
> > > > v4:
> > > > -Separate the pipe_bpp calculation for eDP and DP.
> > > > 
> > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 39e2bf3d738d..578320220c9a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > > >  }
> > > >  
> > > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > > +				 const struct drm_display_mode *adjusted_mode)
> > > > +{
> > > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > > +
> > > > +	return mode_rate <= link_avail;
> > > > +}
> > > > +
> > > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > > +				   struct intel_crtc_state *pipe_config,
> > > > +				   struct link_config_limits *limits,
> > > > +				   int pipe_bpp,
> > > > +				   u16 compressed_bpp,
> > > > +				   int timeslots)
> > > > +{
> > > > +	const struct drm_display_mode *adjusted_mode =
> > > > +		&pipe_config->hw.adjusted_mode;
> > > > +	int link_rate, lane_count;
> > > > +	int dsc_max_bpp;
> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > > +			continue;
> > > > +
> > > > +		for (lane_count = limits->min_lane_count;
> > > > +		     lane_count <= limits->max_lane_count;
> > > > +		     lane_count <<= 1) {
> > > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > > +									  link_rate,
> > > > +									  lane_count,
> > > > +									  adjusted_mode->crtc_clock,
> > > > +									  adjusted_mode->crtc_hdisplay,
> > > > +									  pipe_config->bigjoiner_pipes,
> > > > +									  pipe_config->output_format,
> > > > +									  pipe_bpp, timeslots);
> > > > +			/*
> > > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > > +			 * supported PPS value can be 63.9375 and with the further
> > > > +			 * mention that bpp should be programmed double the target bpp
> > > > +			 * restricting our target bpp to be 31.9375 at max
> > > > +			 */
> > > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > > +
> > > > +			if (compressed_bpp > dsc_max_bpp)
> > > > +				continue;
> > > > +
> > > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > > +						  compressed_bpp, adjusted_mode))
> > > > +				continue;
> > > > +
> > > > +			pipe_config->lane_count = lane_count;
> > > > +			pipe_config->port_clock = link_rate;
> > > > +
> > > > +			return 0;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > > +static
> > > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > > +					    struct intel_crtc_state *pipe_config,
> > > > +					    int bpc)
> > > > +{
> > > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > > +
> > > > +	if (max_bppx16)
> > > > +		return max_bppx16;
> > > > +	/*
> > > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > > +	 * values as given in spec Table 2-157 DP v2.0
> > > > +	 */
> > > > +	switch (pipe_config->output_format) {
> > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > +		return (3 * bpc) << 4;
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > +		return (3 * (bpc / 2)) << 4;
> > > > +	default:
> > > > +		MISSING_CASE(pipe_config->output_format);
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > > +{
> > > > +	switch (pipe_config->output_format) {
> > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > +		return 8 << 4;
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > +		return 6 << 4;
> > > > +	default:
> > > > +		MISSING_CASE(pipe_config->output_format);
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > > +				      struct intel_crtc_state *pipe_config,
> > > > +				      struct link_config_limits *limits,
> > > > +				      int pipe_bpp,
> > > > +				      int timeslots)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > +	u16 compressed_bpp;
> > > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > > +	int ret;
> > > > +
> > > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > > +		dsc_src_max_bpp = 23;
> > > > +	else
> > > > +		dsc_src_max_bpp = 27;
> > > 
> > > I would may be added some comment about what are those "23/27" numbers or
> > > may be even created some self-explanatory #define constants for those.
> > 
> > I dislike defines like that. They are single use so don't actually
> > do anything in terms of avoiding typoes and other accidental
> > mismatches, and people always seem put them in some random place
> > (eg. top of file) so then it takes extra work to find them.
> 
> Ah come on, even my primitive mcedit with ctags plugin can track it :))
> However my point is that anything is better than just hard-coded magic
> numbers, which is proven antipattern.

It's still a magic number whether you hide it behind a define or not.

> Also you never know if it is a single or multiple use,

If you use it multiple times then you aren't using the function
correctly.

> I think it should be
> either defined as a constant or as a define, which is self explanatory.

No more self explanatory than a function. Once you have the
function the define is entirely redundant.

> 
> > 
> > The best approach IMO is to just use functions with good names.
> > Eg. in this case we could just have a full set of clear functions:
> > dsc_{sink,source}_{min,max}_bpp() or something along those line.
> 
> ..which still doesn't explain, why it is 23 there, why it is 27, who sets those
> numbers, which spec and so on.

Neither does a define. All a define will do is say that for platform
X return define Y which is defined as Z. Returning Z directly is less
convoluted and just as helpful in figuring out where the numbers came
from. If it's hard to figure out where the number came from then you
can add a comment to indicate where it is specified.

-- 
Ville Syrjälä
Intel

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Lisovskiy, Stanislav]

On Wed, May 24, 2023 at 03:38:42PM +0300, Ville Syrjälä wrote:
> On Tue, May 23, 2023 at 12:01:34PM +0300, Lisovskiy, Stanislav wrote:
> > On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> > > On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > > > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > > > compressed bpp possible. We then set the output bpp to this value.
> > > > > This patch provides support to have max bpp, min rate and min lanes,
> > > > > that can support the min compressed bpp.
> > > > > 
> > > > > v2:
> > > > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > > > >  valid DSC bpcs. (Stan)
> > > > > 
> > > > > v3:
> > > > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > > > count calculation separately for different cases.
> > > > > 
> > > > > v4:
> > > > > -Separate the pipe_bpp calculation for eDP and DP.
> > > > > 
> > > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > > > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > index 39e2bf3d738d..578320220c9a 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > > > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > > > >  }
> > > > >  
> > > > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > > > +				 const struct drm_display_mode *adjusted_mode)
> > > > > +{
> > > > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > > > +
> > > > > +	return mode_rate <= link_avail;
> > > > > +}
> > > > > +
> > > > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > > > +				   struct intel_crtc_state *pipe_config,
> > > > > +				   struct link_config_limits *limits,
> > > > > +				   int pipe_bpp,
> > > > > +				   u16 compressed_bpp,
> > > > > +				   int timeslots)
> > > > > +{
> > > > > +	const struct drm_display_mode *adjusted_mode =
> > > > > +		&pipe_config->hw.adjusted_mode;
> > > > > +	int link_rate, lane_count;
> > > > > +	int dsc_max_bpp;
> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > +	int i;
> > > > > +
> > > > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > > > +			continue;
> > > > > +
> > > > > +		for (lane_count = limits->min_lane_count;
> > > > > +		     lane_count <= limits->max_lane_count;
> > > > > +		     lane_count <<= 1) {
> > > > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > > > +									  link_rate,
> > > > > +									  lane_count,
> > > > > +									  adjusted_mode->crtc_clock,
> > > > > +									  adjusted_mode->crtc_hdisplay,
> > > > > +									  pipe_config->bigjoiner_pipes,
> > > > > +									  pipe_config->output_format,
> > > > > +									  pipe_bpp, timeslots);
> > > > > +			/*
> > > > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > > > +			 * supported PPS value can be 63.9375 and with the further
> > > > > +			 * mention that bpp should be programmed double the target bpp
> > > > > +			 * restricting our target bpp to be 31.9375 at max
> > > > > +			 */
> > > > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > > > +
> > > > > +			if (compressed_bpp > dsc_max_bpp)
> > > > > +				continue;
> > > > > +
> > > > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > > > +						  compressed_bpp, adjusted_mode))
> > > > > +				continue;
> > > > > +
> > > > > +			pipe_config->lane_count = lane_count;
> > > > > +			pipe_config->port_clock = link_rate;
> > > > > +
> > > > > +			return 0;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static
> > > > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > > > +					    struct intel_crtc_state *pipe_config,
> > > > > +					    int bpc)
> > > > > +{
> > > > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > > > +
> > > > > +	if (max_bppx16)
> > > > > +		return max_bppx16;
> > > > > +	/*
> > > > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > > > +	 * values as given in spec Table 2-157 DP v2.0
> > > > > +	 */
> > > > > +	switch (pipe_config->output_format) {
> > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > +		return (3 * bpc) << 4;
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > +		return (3 * (bpc / 2)) << 4;
> > > > > +	default:
> > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > > > +{
> > > > > +	switch (pipe_config->output_format) {
> > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > +		return 8 << 4;
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > +		return 6 << 4;
> > > > > +	default:
> > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > > > +				      struct intel_crtc_state *pipe_config,
> > > > > +				      struct link_config_limits *limits,
> > > > > +				      int pipe_bpp,
> > > > > +				      int timeslots)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > +	u16 compressed_bpp;
> > > > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > > > +	int ret;
> > > > > +
> > > > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > > > +		dsc_src_max_bpp = 23;
> > > > > +	else
> > > > > +		dsc_src_max_bpp = 27;
> > > > 
> > > > I would may be added some comment about what are those "23/27" numbers or
> > > > may be even created some self-explanatory #define constants for those.
> > > 
> > > I dislike defines like that. They are single use so don't actually
> > > do anything in terms of avoiding typoes and other accidental
> > > mismatches, and people always seem put them in some random place
> > > (eg. top of file) so then it takes extra work to find them.
> > 
> > Ah come on, even my primitive mcedit with ctags plugin can track it :))
> > However my point is that anything is better than just hard-coded magic
> > numbers, which is proven antipattern.
> 
> It's still a magic number whether you hide it behind a define or not.

define will gixe a clue at least

> 
> > Also you never know if it is a single or multiple use,
> 
> If you use it multiple times then you aren't using the function
> correctly.

hmm.. I didn't understand we are using defines in many places, like
register names, things like PIPE_A(which is enum but still), it is just 
more explanatory and elegant rather than use 0 instead of PIPE_A, right?

> 
> > I think it should be
> > either defined as a constant or as a define, which is self explanatory.
> 
> No more self explanatory than a function. Once you have the
> function the define is entirely redundant.

The function explains what it does, but I'm afraid you can't put explanation
for all the constants it used in a single function name..

> 
> > 
> > > 
> > > The best approach IMO is to just use functions with good names.
> > > Eg. in this case we could just have a full set of clear functions:
> > > dsc_{sink,source}_{min,max}_bpp() or something along those line.
> > 
> > ..which still doesn't explain, why it is 23 there, why it is 27, who sets those
> > numbers, which spec and so on.
> 
> Neither does a define. All a define will do is say that for platform
> X return define Y which is defined as Z. Returning Z directly is less
> convoluted and just as helpful in figuring out where the numbers came
> from. If it's hard to figure out where the number came from then you
> can add a comment to indicate where it is specified.

Well, I agree about the comment. Comment might do, but hardcoded numbers
are wrong. With this logic we could also not use register names like PLANE_CTL
but just write 0x40*** and then rely that function name will explain what happens
here or use pipe numbers like 0,1,2, instead of PIPE_A/PIPE_B...
Of course we could, but it doesn't look nice then, because numbers do not carry any 
info..

I mean, a concept that magic/hardcoded numbers are wrong isn't even my opinion, it is just 
world-wide known antipattern.
If you don't like defines there are constants also and whatever, my point is that just
hardcoded numbers are almost always bad.

Anyways I'm fine with a comment, as well, just because I think endless arguing is even worse than
magic numbers..

Stan

> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

[Ville Syrjälä]

On Wed, May 24, 2023 at 03:59:47PM +0300, Lisovskiy, Stanislav wrote:
> On Wed, May 24, 2023 at 03:38:42PM +0300, Ville Syrjälä wrote:
> > On Tue, May 23, 2023 at 12:01:34PM +0300, Lisovskiy, Stanislav wrote:
> > > On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> > > > On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > > > > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > > > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > > > > compressed bpp possible. We then set the output bpp to this value.
> > > > > > This patch provides support to have max bpp, min rate and min lanes,
> > > > > > that can support the min compressed bpp.
> > > > > > 
> > > > > > v2:
> > > > > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > > > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > > > > >  valid DSC bpcs. (Stan)
> > > > > > 
> > > > > > v3:
> > > > > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > > > > count calculation separately for different cases.
> > > > > > 
> > > > > > v4:
> > > > > > -Separate the pipe_bpp calculation for eDP and DP.
> > > > > > 
> > > > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > > > > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > index 39e2bf3d738d..578320220c9a 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > > > > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > > > > >  }
> > > > > >  
> > > > > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > > > > +				 const struct drm_display_mode *adjusted_mode)
> > > > > > +{
> > > > > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > > > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > > > > +
> > > > > > +	return mode_rate <= link_avail;
> > > > > > +}
> > > > > > +
> > > > > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > > > > +				   struct intel_crtc_state *pipe_config,
> > > > > > +				   struct link_config_limits *limits,
> > > > > > +				   int pipe_bpp,
> > > > > > +				   u16 compressed_bpp,
> > > > > > +				   int timeslots)
> > > > > > +{
> > > > > > +	const struct drm_display_mode *adjusted_mode =
> > > > > > +		&pipe_config->hw.adjusted_mode;
> > > > > > +	int link_rate, lane_count;
> > > > > > +	int dsc_max_bpp;
> > > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > > +	int i;
> > > > > > +
> > > > > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > > > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > > > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		for (lane_count = limits->min_lane_count;
> > > > > > +		     lane_count <= limits->max_lane_count;
> > > > > > +		     lane_count <<= 1) {
> > > > > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > > > > +									  link_rate,
> > > > > > +									  lane_count,
> > > > > > +									  adjusted_mode->crtc_clock,
> > > > > > +									  adjusted_mode->crtc_hdisplay,
> > > > > > +									  pipe_config->bigjoiner_pipes,
> > > > > > +									  pipe_config->output_format,
> > > > > > +									  pipe_bpp, timeslots);
> > > > > > +			/*
> > > > > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > > > > +			 * supported PPS value can be 63.9375 and with the further
> > > > > > +			 * mention that bpp should be programmed double the target bpp
> > > > > > +			 * restricting our target bpp to be 31.9375 at max
> > > > > > +			 */
> > > > > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > > > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > > > > +
> > > > > > +			if (compressed_bpp > dsc_max_bpp)
> > > > > > +				continue;
> > > > > > +
> > > > > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > > > > +						  compressed_bpp, adjusted_mode))
> > > > > > +				continue;
> > > > > > +
> > > > > > +			pipe_config->lane_count = lane_count;
> > > > > > +			pipe_config->port_clock = link_rate;
> > > > > > +
> > > > > > +			return 0;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	return -EINVAL;
> > > > > > +}
> > > > > > +
> > > > > > +static
> > > > > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > > > > +					    struct intel_crtc_state *pipe_config,
> > > > > > +					    int bpc)
> > > > > > +{
> > > > > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > > > > +
> > > > > > +	if (max_bppx16)
> > > > > > +		return max_bppx16;
> > > > > > +	/*
> > > > > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > > > > +	 * values as given in spec Table 2-157 DP v2.0
> > > > > > +	 */
> > > > > > +	switch (pipe_config->output_format) {
> > > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > > +		return (3 * bpc) << 4;
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > > +		return (3 * (bpc / 2)) << 4;
> > > > > > +	default:
> > > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > > +		break;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > > > > +{
> > > > > > +	switch (pipe_config->output_format) {
> > > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > > +		return 8 << 4;
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > > +		return 6 << 4;
> > > > > > +	default:
> > > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > > +		break;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > > > > +				      struct intel_crtc_state *pipe_config,
> > > > > > +				      struct link_config_limits *limits,
> > > > > > +				      int pipe_bpp,
> > > > > > +				      int timeslots)
> > > > > > +{
> > > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > > +	u16 compressed_bpp;
> > > > > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > > > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > > > > +		dsc_src_max_bpp = 23;
> > > > > > +	else
> > > > > > +		dsc_src_max_bpp = 27;
> > > > > 
> > > > > I would may be added some comment about what are those "23/27" numbers or
> > > > > may be even created some self-explanatory #define constants for those.
> > > > 
> > > > I dislike defines like that. They are single use so don't actually
> > > > do anything in terms of avoiding typoes and other accidental
> > > > mismatches, and people always seem put them in some random place
> > > > (eg. top of file) so then it takes extra work to find them.
> > > 
> > > Ah come on, even my primitive mcedit with ctags plugin can track it :))
> > > However my point is that anything is better than just hard-coded magic
> > > numbers, which is proven antipattern.
> > 
> > It's still a magic number whether you hide it behind a define or not.
> 
> define will gixe a clue at least
> 
> > 
> > > Also you never know if it is a single or multiple use,
> > 
> > If you use it multiple times then you aren't using the function
> > correctly.
> 
> hmm.. I didn't understand we are using defines in many places, like
> register names, things like PIPE_A(which is enum but still), it is just 
> more explanatory and elegant rather than use 0 instead of PIPE_A, right?
> 
> > 
> > > I think it should be
> > > either defined as a constant or as a define, which is self explanatory.
> > 
> > No more self explanatory than a function. Once you have the
> > function the define is entirely redundant.
> 
> The function explains what it does, but I'm afraid you can't put explanation
> for all the constants it used in a single function name..

#define FOO_MAX_BPP 23
#define BAR_MAX_BPP 27

...
int max_bpp()
{
	if (IS_FOO)
		return FOO_MAX_BPP;
	else
		return BAR_MAX_BPP;
}

vs.

int max_bpp()
{
	if (IS_FOO)
		return 23;
	else
		return 27;
}

The defines don't add any extra infrmation here.

-- 
Ville Syrjälä
Intel