[PATCH 00/11] drm/i915: MST+DSC nukage and state stuff

[PATCH 00/11] drm/i915: MST+DSC nukage and state stuff

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The big one here is removal of the defunct i915 MST DSC code.
That one clearly needs a lot more love, and the big issue
there (FEC) probably can't be done in a way that can be
easily backported. So IMO we just need to nuke the whole
MST+DSC thing for now, or else we'll end up with impossible
to debug bug reports.

The rest is mainly improvements around state
readout/check/dumping.

Ville Syrjälä (11):
  drm/dp_mst: Fix fractional DSC bpp handling
  drm/i915/mst: Remove broken MST DSC support
  drm/i915/mst: Read out FEC state
  drm/i915: Fix FEC pipe A vs. DDI A mixup
  drm/i915: Check lane count when determining FEC support
  drm/i915: Fix FEC state dump
  drm/i915: Split some long lines
  drm/i915: Introduce crtc_state->enhanced_framing
  drm/i915: Stop spamming the logs with PLL state
  drm/i915: Drop some redundant eDP checks
  drm/i915: Reduce combo PHY log spam

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   2 +-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |   2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c |  20 +-
 drivers/gpu/drm/i915/display/g4x_dp.c         |  10 +-
 .../gpu/drm/i915/display/intel_combo_phy.c    |  17 +-
 drivers/gpu/drm/i915/display/intel_crt.c      |   2 +
 .../drm/i915/display/intel_crtc_state_dump.c  |   3 +
 drivers/gpu/drm/i915/display/intel_ddi.c      |  29 +--
 drivers/gpu/drm/i915/display/intel_display.c  |   1 +
 .../drm/i915/display/intel_display_types.h    |   2 +
 drivers/gpu/drm/i915/display/intel_dp.c       |  29 +--
 .../drm/i915/display/intel_dp_link_training.c |   2 +-
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 181 +-----------------
 drivers/gpu/drm/i915/display/intel_fdi.c      |   9 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |   3 +-
 .../gpu/drm/tests/drm_dp_mst_helper_test.c    |   2 +-
 include/drm/display/drm_dp_mst_helper.h       |   2 +-
 17 files changed, 80 insertions(+), 236 deletions(-)

-- 
2.39.2

[PATCH 01/11] drm/dp_mst: Fix fractional DSC bpp handling

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current code does '(bpp << 4) / 16' in the MST PBN
calculation, but that is just the same as 'bpp' so the
DSC codepath achieves absolutely nothing. Fix it up so that
the fractional part of the bpp value is actually used instead
of truncated away. 64*1006 has enough zero lsbs that we can
just shift that down in the dividend and thus still manage
to stick to a 32bit divisor.

And while touching this, let's just make the whole thing more
straightforward by making the passed in bpp value .4 binary
fixed point always, instead of having to pass in different
things based on whether DSC is enabled or not.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: David Francis <David.Francis@amd.com>
Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
 .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  2 +-
 include/drm/display/drm_dp_mst_helper.h       |  2 +-
 7 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 6cacb76f389e..7d58f08a5444 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6763,7 +6763,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 								    max_bpc);
 		bpp = convert_dc_color_depth_into_bpc(color_depth) * 3;
 		clock = adjusted_mode->clock;
-		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false);
+		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
 	}
 
 	dm_new_connector_state->vcpi_slots =
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 994ba426ca66..eb4b666e50e8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -1515,7 +1515,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
 	} else {
 		/* check if mode could be supported within full_pbn */
 		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
-		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
+		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
 
 		if (pbn > aconnector->mst_output_port->full_pbn)
 			return DC_FAIL_BANDWIDTH_VALIDATE;
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 38dab76ae69e..cd4c4f22c903 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4619,13 +4619,12 @@ EXPORT_SYMBOL(drm_dp_check_act_status);
 
 /**
  * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode.
- * @clock: dot clock for the mode
- * @bpp: bpp for the mode.
- * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel
+ * @clock: dot clock
+ * @bpp: bpp as .4 binary fixed point
  *
  * This uses the formula in the spec to calculate the PBN value for a mode.
  */
-int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
+int drm_dp_calc_pbn_mode(int clock, int bpp)
 {
 	/*
 	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
@@ -4636,18 +4635,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
 	 * peak_kbps *= (1006/1000)
 	 * peak_kbps *= (64/54)
 	 * peak_kbps *= 8    convert to bytes
-	 *
-	 * If the bpp is in units of 1/16, further divide by 16. Put this
-	 * factor in the numerator rather than the denominator to avoid
-	 * integer overflow
 	 */
-
-	if (dsc)
-		return DIV_ROUND_UP_ULL(mul_u32_u32(clock * (bpp / 16), 64 * 1006),
-					8 * 54 * 1000 * 1000);
-
-	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
-				8 * 54 * 1000 * 1000);
+	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
+				1000 * 8 * 54 * 1000);
 }
 EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 2c49d9ab86a2..44c15d6faac4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -109,8 +109,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 			continue;
 
 		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
-						       dsc ? bpp << 4 : bpp,
-						       dsc);
+						       bpp << 4);
 
 		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
 						      connector->port,
@@ -936,7 +935,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 		return ret;
 
 	if (mode_rate > max_rate || mode->clock > max_dotclk ||
-	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
+	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
 		*status = MODE_CLOCK_HIGH;
 		return 0;
 	}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 5bb777ff1313..d896cbb8cf3d 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -961,8 +961,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 		const int clock = crtc_state->adjusted_mode.clock;
 
 		asyh->or.bpc = connector->display_info.bpc;
-		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
-						    false);
+		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
 	}
 
 	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
index 545beea33e8c..39fc449148e1 100644
--- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
@@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
 {
 	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
 
-	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
+	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
 			params->expected);
 }
 
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 32c764fb9cb5..c254500b4507 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -829,7 +829,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
 int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
 			     int link_rate, int link_lane_count);
 
-int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
+int drm_dp_calc_pbn_mode(int clock, int bpp);
 
 void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
 
-- 
2.39.2

[PATCH 02/11] drm/i915/mst: Remove broken MST DSC support

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The MST DSC code has a myriad of issues:
- Platform checks are wrong (MST+DSC is TGL+ only IIRC)
- Return values of .mode_valid_ctx() are wrong
- .mode_valid_ctx() assumes bigjoiner might be used, but ther rest
  of the code doesn't agree
- compressed bpp calculations don't make sense
- FEC handling needs to consider the entire link as opposed to just
  the single stream. Currently FEC would only get enabled if the
  first enabled stream is compressed. Also I'm not seeing anything
  that would account for the FEC overhead in any bandwidth calculations
- PPS SDP is only handled for the first stream via the dig_port
  hooks, other streams will not be transmittitng any PPS SDPs
- PPS SDP readout is missing (also missing for SST!)
- VDSC readout is missing (also missing for SST!)

The FEC issues is really the big one since we have no way currently
to apply such link wide configuration constraints. Changing that is
going to require a much bigger rework of the higher level modeset
.compute_config() logic. We will also need such a rework to properly
distribute the available bandwidth across all the streams on the
same link (which is a must to eg. enable deep color).

Cc: stable@vger.kernel.org
Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: d51f25eb479a ("drm/i915: Add DSC support to MST path")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 176 +-------------------
 1 file changed, 5 insertions(+), 171 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 44c15d6faac4..d762f37fafb5 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -72,8 +72,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 						int min_bpp,
 						struct link_config_limits *limits,
 						struct drm_connector_state *conn_state,
-						int step,
-						bool dsc)
+						int step)
 {
 	struct drm_atomic_state *state = crtc_state->uapi.state;
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
@@ -104,7 +103,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
 		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
 
-		ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, dsc);
+		ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, false);
 		if (ret)
 			continue;
 
@@ -136,11 +135,8 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 		drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n",
 			    slots);
 	} else {
-		if (!dsc)
-			crtc_state->pipe_bpp = bpp;
-		else
-			crtc_state->dsc.compressed_bpp = bpp;
-		drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d dsc %d\n", slots, bpp, dsc);
+		crtc_state->pipe_bpp = bpp;
+		drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d\n", slots, bpp);
 	}
 
 	return slots;
@@ -157,7 +153,7 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 
 	slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp,
 						     limits->min_bpp, limits,
-						     conn_state, 2 * 3, false);
+						     conn_state, 2 * 3);
 
 	if (slots < 0)
 		return slots;
@@ -173,99 +169,6 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	return 0;
 }
 
-static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder,
-						struct intel_crtc_state *crtc_state,
-						struct drm_connector_state *conn_state,
-						struct link_config_limits *limits)
-{
-	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
-	struct intel_dp *intel_dp = &intel_mst->primary->dp;
-	struct intel_connector *connector =
-		to_intel_connector(conn_state->connector);
-	struct drm_i915_private *i915 = to_i915(connector->base.dev);
-	const struct drm_display_mode *adjusted_mode =
-		&crtc_state->hw.adjusted_mode;
-	int slots = -EINVAL;
-	int i, num_bpc;
-	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;
-
-	/* 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, conn_state->max_requested_bpc);
-	else
-		dsc_max_bpc = min_t(u8, 10, conn_state->max_requested_bpc);
-
-	max_bpp = min_t(u8, dsc_max_bpc * 3, limits->max_bpp);
-	min_bpp = limits->min_bpp;
-
-	num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
-						       dsc_bpc);
-
-	drm_dbg_kms(&i915->drm, "DSC Source supported min bpp %d max bpp %d\n",
-		    min_bpp, max_bpp);
-
-	sink_max_bpp = dsc_bpc[0] * 3;
-	sink_min_bpp = sink_max_bpp;
-
-	for (i = 1; i < num_bpc; i++) {
-		if (sink_min_bpp > dsc_bpc[i] * 3)
-			sink_min_bpp = dsc_bpc[i] * 3;
-		if (sink_max_bpp < dsc_bpc[i] * 3)
-			sink_max_bpp = dsc_bpc[i] * 3;
-	}
-
-	drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n",
-		    sink_min_bpp, sink_max_bpp);
-
-	if (min_bpp < sink_min_bpp)
-		min_bpp = sink_min_bpp;
-
-	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;
-
-	crtc_state->dsc.compressed_bpp = intel_dp_dsc_nearest_valid_bpp(i915,
-									last_compressed_bpp,
-									crtc_state->pipe_bpp);
-
-	if (crtc_state->dsc.compressed_bpp != last_compressed_bpp)
-		need_timeslot_recalc = 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;
-	}
-
-	intel_link_compute_m_n(crtc_state->dsc.compressed_bpp,
-			       crtc_state->lane_count,
-			       adjusted_mode->crtc_clock,
-			       crtc_state->port_clock,
-			       &crtc_state->dp_m_n,
-			       crtc_state->fec_enable);
-	crtc_state->dp_m_n.tu = slots;
-
-	return 0;
-}
 static int intel_dp_mst_update_slots(struct intel_encoder *encoder,
 				     struct intel_crtc_state *crtc_state,
 				     struct drm_connector_state *conn_state)
@@ -349,29 +252,6 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	ret = intel_dp_mst_compute_link_config(encoder, pipe_config,
 					       conn_state, &limits);
-
-	if (ret == -EDEADLK)
-		return ret;
-
-	/* 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) {
-		/*
-		 * Try to get at least some timeslots and then see, if
-		 * we can fit there with DSC.
-		 */
-		drm_dbg_kms(&dev_priv->drm, "Trying to find VCPI slots in DSC mode\n");
-
-		ret = intel_dp_dsc_mst_compute_link_config(encoder, pipe_config,
-							   conn_state, &limits);
-		if (ret < 0)
-			return ret;
-
-		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
-						  conn_state, &limits,
-						  pipe_config->dp_m_n.tu, false);
-	}
-
 	if (ret)
 		return ret;
 
@@ -909,10 +789,6 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 	int ret;
-	bool dsc = false, bigjoiner = false;
-	u16 dsc_max_output_bpp = 0;
-	u8 dsc_slice_count = 0;
-	int target_clock = mode->clock;
 
 	if (drm_connector_is_unregistered(connector)) {
 		*status = MODE_ERROR;
@@ -950,48 +826,6 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 		return 0;
 	}
 
-	if (intel_dp_need_bigjoiner(intel_dp, mode->hdisplay, target_clock)) {
-		bigjoiner = true;
-		max_dotclk *= 2;
-	}
-
-	if (DISPLAY_VER(dev_priv) >= 10 &&
-	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
-		/*
-		 * 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);
-
-		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,
-							    pipe_bpp, 64) >> 4;
-			dsc_slice_count =
-				intel_dp_dsc_get_slice_count(intel_dp,
-							     target_clock,
-							     mode->hdisplay,
-							     bigjoiner);
-		}
-
-		dsc = dsc_max_output_bpp && dsc_slice_count;
-	}
-
-	/*
-	 * Big joiner configuration needs DSC for TGL which is not true for
-	 * XE_LPD where uncompressed joiner is supported.
-	 */
-	if (DISPLAY_VER(dev_priv) < 13 && bigjoiner && !dsc)
-		return MODE_CLOCK_HIGH;
-
-	if (mode_rate > max_rate && !dsc)
-		return MODE_CLOCK_HIGH;
-
 	*status = intel_mode_valid_max_plane_size(dev_priv, mode, false);
 	return 0;
 }
-- 
2.39.2

[PATCH 03/11] drm/i915/mst: Read out FEC state

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The MST codepath is missing FEC readout. Add it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 55f36d9d509c..41cfa28166e4 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3763,6 +3763,11 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
 		intel_cpu_transcoder_get_m1_n1(crtc, cpu_transcoder,
 					       &pipe_config->dp_m_n);
 
+		if (DISPLAY_VER(dev_priv) >= 11)
+			pipe_config->fec_enable =
+				intel_de_read(dev_priv,
+					      dp_tp_ctl_reg(encoder, pipe_config)) & DP_TP_CTL_FEC_ENABLE;
+
 		pipe_config->infoframes.enable |=
 			intel_hdmi_infoframes_enabled(encoder, pipe_config);
 		break;
-- 
2.39.2

[PATCH 04/11] drm/i915: Fix FEC pipe A vs. DDI A mixup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On pre-TGL FEC is a port level feature, not a transcoder
level features, and it's DDI A which doesn't have it, not
trancodere A. Check for the correct thing when determining
whether FEC is supported or not.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 4361c1ac65c3..b27b4fb71ed7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1212,13 +1212,13 @@ void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
 					 const struct intel_crtc_state *pipe_config)
 {
+	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
-	/* On TGL, FEC is supported on all Pipes */
 	if (DISPLAY_VER(dev_priv) >= 12)
 		return true;
 
-	if (DISPLAY_VER(dev_priv) == 11 && pipe_config->cpu_transcoder != TRANSCODER_A)
+	if (DISPLAY_VER(dev_priv) == 11 && encoder->port != PORT_A)
 		return true;
 
 	return false;
-- 
2.39.2

[PATCH 05/11] drm/i915: Check lane count when determining FEC support

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

ICL doesn't support FEC with a x1 DP link. Make sure
we don't try to enable FEC in such cases.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index b27b4fb71ed7..9ac199444155 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1218,7 +1218,8 @@ static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
 	if (DISPLAY_VER(dev_priv) >= 12)
 		return true;
 
-	if (DISPLAY_VER(dev_priv) == 11 && encoder->port != PORT_A)
+	if (DISPLAY_VER(dev_priv) == 11 &&
+	    encoder->port != PORT_A && pipe_config->lane_count != 1)
 		return true;
 
 	return false;
@@ -1234,7 +1235,7 @@ static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
 static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
 				  const struct intel_crtc_state *crtc_state)
 {
-	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP) && !crtc_state->fec_enable)
+	if (!intel_dp_is_edp(intel_dp) && !crtc_state->fec_enable)
 		return false;
 
 	return intel_dsc_source_support(crtc_state) &&
@@ -1580,15 +1581,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	int pipe_bpp;
 	int ret;
 
-	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
-		intel_dp_supports_fec(intel_dp, pipe_config);
-
-	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
-		return -EINVAL;
-
-	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
-		return -EINVAL;
-
 	if (compute_pipe_bpp)
 		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
 	else
@@ -1615,6 +1607,15 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	pipe_config->port_clock = limits->max_rate;
 	pipe_config->lane_count = limits->max_lane_count;
 
+	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
+		intel_dp_supports_fec(intel_dp, pipe_config);
+
+	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
+		return -EINVAL;
+
+	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
+		return -EINVAL;
+
 	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,
-- 
2.39.2

[PATCH 06/11] drm/i915: Fix FEC state dump

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Stop dumping state while reading it out. We have a proper
place for that stuff.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_crtc_state_dump.c    |  2 ++
 drivers/gpu/drm/i915/display/intel_ddi.c            | 13 +++----------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 0cdcaa49656f..91242ffe0768 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -257,6 +257,8 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
 		intel_dump_m_n_config(pipe_config, "dp m2_n2",
 				      pipe_config->lane_count,
 				      &pipe_config->dp_m2_n2);
+		drm_dbg_kms(&i915->drm, "fec: %s\n",
+			    str_enabled_disabled(pipe_config->fec_enable));
 	}
 
 	drm_dbg_kms(&i915->drm, "framestart delay: %d, MSA timing delay: %d\n",
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 41cfa28166e4..4246133950fd 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3725,17 +3725,10 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
 		intel_cpu_transcoder_get_m2_n2(crtc, cpu_transcoder,
 					       &pipe_config->dp_m2_n2);
 
-		if (DISPLAY_VER(dev_priv) >= 11) {
-			i915_reg_t dp_tp_ctl = dp_tp_ctl_reg(encoder, pipe_config);
-
+		if (DISPLAY_VER(dev_priv) >= 11)
 			pipe_config->fec_enable =
-				intel_de_read(dev_priv, dp_tp_ctl) & DP_TP_CTL_FEC_ENABLE;
-
-			drm_dbg_kms(&dev_priv->drm,
-				    "[ENCODER:%d:%s] Fec status: %u\n",
-				    encoder->base.base.id, encoder->base.name,
-				    pipe_config->fec_enable);
-		}
+				intel_de_read(dev_priv,
+					      dp_tp_ctl_reg(encoder, pipe_config)) & DP_TP_CTL_FEC_ENABLE;
 
 		if (dig_port->lspcon.active && dig_port->dp.has_hdmi_sink)
 			pipe_config->infoframes.enable |=
-- 
2.39.2

[PATCH 07/11] drm/i915: Split some long lines

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Split some overly long lines.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fdi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
index 55283677c45a..19ee78ba3936 100644
--- a/drivers/gpu/drm/i915/display/intel_fdi.c
+++ b/drivers/gpu/drm/i915/display/intel_fdi.c
@@ -765,7 +765,10 @@ void hsw_fdi_link_train(struct intel_encoder *encoder,
 	 * WaFDIAutoLinkSetTimingOverrride:hsw
 	 */
 	intel_de_write(dev_priv, FDI_RX_MISC(PIPE_A),
-		       FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2) | FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90);
+		       FDI_RX_PWRDN_LANE1_VAL(2) |
+		       FDI_RX_PWRDN_LANE0_VAL(2) |
+		       FDI_RX_TP1_TO_TP2_48 |
+		       FDI_RX_FDI_DELAY_90);
 
 	/* Enable the PCH Receiver FDI PLL */
 	rx_ctl_val = dev_priv->display.fdi.rx_config | FDI_RX_ENHANCE_FRAME_ENABLE |
@@ -798,7 +801,9 @@ void hsw_fdi_link_train(struct intel_encoder *encoder,
 		 * achieved on the PCH side in FDI_RX_CTL, so no need to set the
 		 * port reversal bit */
 		intel_de_write(dev_priv, DDI_BUF_CTL(PORT_E),
-			       DDI_BUF_CTL_ENABLE | ((crtc_state->fdi_lanes - 1) << 1) | DDI_BUF_TRANS_SELECT(i / 2));
+			       DDI_BUF_CTL_ENABLE |
+			       ((crtc_state->fdi_lanes - 1) << 1) |
+			       DDI_BUF_TRANS_SELECT(i / 2));
 		intel_de_posting_read(dev_priv, DDI_BUF_CTL(PORT_E));
 
 		udelay(600);
-- 
2.39.2

[PATCH 08/11] drm/i915: Introduce crtc_state->enhanced_framing

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Track DP enhanced framing properly in the crtc state instead
of relying just on the cached DPCD everywhere, and hook it
up into the state check and dump.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/g4x_dp.c                 | 10 ++++++++--
 drivers/gpu/drm/i915/display/intel_crt.c              |  2 ++
 drivers/gpu/drm/i915/display/intel_crtc_state_dump.c  |  5 +++--
 drivers/gpu/drm/i915/display/intel_ddi.c              | 11 +++++++++--
 drivers/gpu/drm/i915/display/intel_display.c          |  1 +
 drivers/gpu/drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_dp_link_training.c |  2 +-
 7 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
index 920d570f7594..534546ea7d0b 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -141,7 +141,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
 
 		intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
 			     TRANS_DP_ENH_FRAMING,
-			     drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
+			     pipe_config->enhanced_framing ?
 			     TRANS_DP_ENH_FRAMING : 0);
 	} else {
 		if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
@@ -153,7 +153,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
 			intel_dp->DP |= DP_SYNC_VS_HIGH;
 		intel_dp->DP |= DP_LINK_TRAIN_OFF;
 
-		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+		if (pipe_config->enhanced_framing)
 			intel_dp->DP |= DP_ENHANCED_FRAMING;
 
 		if (IS_CHERRYVIEW(dev_priv))
@@ -351,6 +351,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 		u32 trans_dp = intel_de_read(dev_priv,
 					     TRANS_DP_CTL(crtc->pipe));
 
+		if (trans_dp & TRANS_DP_ENH_FRAMING)
+			pipe_config->enhanced_framing = true;
+
 		if (trans_dp & TRANS_DP_HSYNC_ACTIVE_HIGH)
 			flags |= DRM_MODE_FLAG_PHSYNC;
 		else
@@ -361,6 +364,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 		else
 			flags |= DRM_MODE_FLAG_NVSYNC;
 	} else {
+		if (tmp & DP_ENHANCED_FRAMING)
+			pipe_config->enhanced_framing = true;
+
 		if (tmp & DP_SYNC_HS_HIGH)
 			flags |= DRM_MODE_FLAG_PHSYNC;
 		else
diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
index 13519f78cf9f..52af64aa9953 100644
--- a/drivers/gpu/drm/i915/display/intel_crt.c
+++ b/drivers/gpu/drm/i915/display/intel_crt.c
@@ -449,6 +449,8 @@ static int hsw_crt_compute_config(struct intel_encoder *encoder,
 	/* FDI must always be 2.7 GHz */
 	pipe_config->port_clock = 135000 * 2;
 
+	pipe_config->enhanced_framing = true;
+
 	adjusted_mode->crtc_clock = lpt_iclkip(pipe_config);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 91242ffe0768..14db2b481ff1 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -257,8 +257,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
 		intel_dump_m_n_config(pipe_config, "dp m2_n2",
 				      pipe_config->lane_count,
 				      &pipe_config->dp_m2_n2);
-		drm_dbg_kms(&i915->drm, "fec: %s\n",
-			    str_enabled_disabled(pipe_config->fec_enable));
+		drm_dbg_kms(&i915->drm, "fec: %s, enhanced framing: %s\n",
+			    str_enabled_disabled(pipe_config->fec_enable),
+			    str_enabled_disabled(pipe_config->enhanced_framing));
 	}
 
 	drm_dbg_kms(&i915->drm, "framestart delay: %d, MSA timing delay: %d\n",
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 4246133950fd..51ae1aad7cc7 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3434,7 +3434,7 @@ static void mtl_ddi_prepare_link_retrain(struct intel_dp *intel_dp,
 		dp_tp_ctl |= DP_TP_CTL_MODE_MST;
 	} else {
 		dp_tp_ctl |= DP_TP_CTL_MODE_SST;
-		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+		if (crtc_state->enhanced_framing)
 			dp_tp_ctl |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 	}
 	intel_de_write(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), dp_tp_ctl);
@@ -3491,7 +3491,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp,
 		dp_tp_ctl |= DP_TP_CTL_MODE_MST;
 	} else {
 		dp_tp_ctl |= DP_TP_CTL_MODE_SST;
-		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+		if (crtc_state->enhanced_framing)
 			dp_tp_ctl |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 	}
 	intel_de_write(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), dp_tp_ctl);
@@ -3725,6 +3725,10 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
 		intel_cpu_transcoder_get_m2_n2(crtc, cpu_transcoder,
 					       &pipe_config->dp_m2_n2);
 
+		pipe_config->enhanced_framing =
+			intel_de_read(dev_priv, dp_tp_ctl_reg(encoder, pipe_config)) &
+			DP_TP_CTL_ENHANCED_FRAME_ENABLE;
+
 		if (DISPLAY_VER(dev_priv) >= 11)
 			pipe_config->fec_enable =
 				intel_de_read(dev_priv,
@@ -3741,6 +3745,9 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
 		if (!HAS_DP20(dev_priv)) {
 			/* FDI */
 			pipe_config->output_types |= BIT(INTEL_OUTPUT_ANALOG);
+			pipe_config->enhanced_framing =
+				intel_de_read(dev_priv, dp_tp_ctl_reg(encoder, pipe_config)) &
+				DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 			break;
 		}
 		fallthrough; /* 128b/132b */
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3d3483e6f836..b95eb031abf2 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5226,6 +5226,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
 	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
 	PIPE_CONF_CHECK_BOOL(has_infoframe);
+	PIPE_CONF_CHECK_BOOL(enhanced_framing);
 	PIPE_CONF_CHECK_BOOL(fec_enable);
 
 	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 96a3183675be..9ea96eb19ddd 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1338,6 +1338,8 @@ struct intel_crtc_state {
 	u16 linetime;
 	u16 ips_linetime;
 
+	bool enhanced_framing;
+
 	/* Forward Error correction State */
 	bool fec_enable;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index e92c62bcc9b8..47a212a84fec 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -659,7 +659,7 @@ intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
 	/* Write the link configuration data */
 	link_config[0] = link_bw;
 	link_config[1] = crtc_state->lane_count;
-	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+	if (crtc_state->enhanced_framing)
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
 
-- 
2.39.2

[PATCH 09/11] drm/i915: Stop spamming the logs with PLL state

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

encoder->get_config() is not the place where the state
should be dumped. Get rid of the spam.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 51ae1aad7cc7..65e031ff740c 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3863,11 +3863,9 @@ static void mtl_ddi_get_config(struct intel_encoder *encoder,
 		crtc_state->port_clock = intel_mtl_tbt_calc_port_clock(encoder);
 	} else if (intel_is_c10phy(i915, phy)) {
 		intel_c10pll_readout_hw_state(encoder, &crtc_state->cx0pll_state.c10);
-		intel_c10pll_dump_hw_state(i915, &crtc_state->cx0pll_state.c10);
 		crtc_state->port_clock = intel_c10pll_calc_port_clock(encoder, &crtc_state->cx0pll_state.c10);
 	} else {
 		intel_c20pll_readout_hw_state(encoder, &crtc_state->cx0pll_state.c20);
-		intel_c20pll_dump_hw_state(i915, &crtc_state->cx0pll_state.c20);
 		crtc_state->port_clock = intel_c20pll_calc_port_clock(encoder, &crtc_state->cx0pll_state.c20);
 	}
 
-- 
2.39.2

[PATCH 10/11] drm/i915: Drop some redundant eDP checks

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's no need to check for both eDP and fixed_mode when
deciding whether to do the pfit calculations or not.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 9ac199444155..6bc7ff0c4320 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1044,7 +1044,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 		return MODE_H_ILLEGAL;
 
 	fixed_mode = intel_panel_fixed_mode(connector, mode);
-	if (intel_dp_is_edp(intel_dp) && fixed_mode) {
+	if (fixed_mode) {
 		status = intel_panel_mode_valid(connector, mode);
 		if (status != MODE_OK)
 			return status;
@@ -2175,7 +2175,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		intel_audio_compute_config(encoder, pipe_config, conn_state);
 
 	fixed_mode = intel_panel_fixed_mode(connector, adjusted_mode);
-	if (intel_dp_is_edp(intel_dp) && fixed_mode) {
+	if (fixed_mode) {
 		ret = intel_panel_compute_config(connector, adjusted_mode);
 		if (ret)
 			return ret;
-- 
2.39.2

[PATCH 11/11] drm/i915: Reduce combo PHY log spam

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We always check whether combo PHYs need to be re-initialized
after disabling DC states, which leads to log spam. Switch things
around so that we only log something when we actually have to
re-initialized a PHY.

The log spam was exacerbated by commit 41b4c7fe72b6 ("drm/i915:
Disable DC states for all commits") since we now disable DC
states far more often.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_combo_phy.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
index 922a6d87b553..ee843f2b1af1 100644
--- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
@@ -114,10 +114,6 @@ static bool icl_verify_procmon_ref_values(struct drm_i915_private *dev_priv,
 
 	procmon = icl_get_procmon_ref_values(dev_priv, phy);
 
-	drm_dbg_kms(&dev_priv->drm,
-		    "Combo PHY %c Voltage/Process Info : %s\n",
-		    phy_name(phy), procmon->name);
-
 	ret = check_phy_reg(dev_priv, phy, ICL_PORT_COMP_DW1(phy),
 			    (0xff << 16) | 0xff, procmon->dw1);
 	ret &= check_phy_reg(dev_priv, phy, ICL_PORT_COMP_DW9(phy),
@@ -312,14 +308,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
 	enum phy phy;
 
 	for_each_combo_phy(dev_priv, phy) {
+		const struct icl_procmon *procmon;
 		u32 val;
 
-		if (icl_combo_phy_verify_state(dev_priv, phy)) {
-			drm_dbg(&dev_priv->drm,
-				"Combo PHY %c already enabled, won't reprogram it.\n",
-				phy_name(phy));
+		if (icl_combo_phy_verify_state(dev_priv, phy))
 			continue;
-		}
+
+		procmon = icl_get_procmon_ref_values(dev_priv, phy);
+
+		drm_dbg(&dev_priv->drm,
+			"Initializing combo PHY %c (Voltage/Process Info : %s)\n",
+			phy_name(phy), procmon->name);
 
 		if (!has_phy_misc(dev_priv, phy))
 			goto skip_phy_misc;
-- 
2.39.2

Re: [PATCH 02/11] drm/i915/mst: Remove broken MST DSC support

[Lisovskiy, Stanislav]

On Tue, May 02, 2023 at 05:38:57PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The MST DSC code has a myriad of issues:
> - Platform checks are wrong (MST+DSC is TGL+ only IIRC)
> - Return values of .mode_valid_ctx() are wrong
> - .mode_valid_ctx() assumes bigjoiner might be used, but ther rest
>   of the code doesn't agree
> - compressed bpp calculations don't make sense
> - FEC handling needs to consider the entire link as opposed to just
>   the single stream. Currently FEC would only get enabled if the
>   first enabled stream is compressed. Also I'm not seeing anything
>   that would account for the FEC overhead in any bandwidth calculations
> - PPS SDP is only handled for the first stream via the dig_port
>   hooks, other streams will not be transmittitng any PPS SDPs
> - PPS SDP readout is missing (also missing for SST!)
> - VDSC readout is missing (also missing for SST!)
> 
> The FEC issues is really the big one since we have no way currently
> to apply such link wide configuration constraints. Changing that is
> going to require a much bigger rework of the higher level modeset
> .compute_config() logic. We will also need such a rework to properly
> distribute the available bandwidth across all the streams on the
> same link (which is a must to eg. enable deep color).

Hi Ville,

I'm not going to argue even that this code has some issues or not,
sure it has. However you are currently totally removing everything
without proposing what fixes have to be done or could have been done
instead.
We had multiple DP MST hubs which started to work because of this code,
also we had a public gitlab issues which were solved. So now they will loose
that support. I mean if this goes through, then I guess its up to you to 
handle this. Why not just fix instead of remove? Just add fixing patches on
top of those which remove the wrong code.
Now we are simply removing all that works at least,
instead of proposing how to fix, leaving no support at all. 
I have 2 MST hubs on my desk which won't work after this code is removed,
so if this patch goes through I recommend taking them.
But at least from my side, I think this is kinda unconstructive.

Stan


> 
> Cc: stable@vger.kernel.org
> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: d51f25eb479a ("drm/i915: Add DSC support to MST path")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 176 +-------------------
>  1 file changed, 5 insertions(+), 171 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 44c15d6faac4..d762f37fafb5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -72,8 +72,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  						int min_bpp,
>  						struct link_config_limits *limits,
>  						struct drm_connector_state *conn_state,
> -						int step,
> -						bool dsc)
> +						int step)
>  {
>  	struct drm_atomic_state *state = crtc_state->uapi.state;
>  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> @@ -104,7 +103,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
>  		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
>  
> -		ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, dsc);
> +		ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, false);
>  		if (ret)
>  			continue;
>  
> @@ -136,11 +135,8 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  		drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n",
>  			    slots);
>  	} else {
> -		if (!dsc)
> -			crtc_state->pipe_bpp = bpp;
> -		else
> -			crtc_state->dsc.compressed_bpp = bpp;
> -		drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d dsc %d\n", slots, bpp, dsc);
> +		crtc_state->pipe_bpp = bpp;
> +		drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d\n", slots, bpp);
>  	}
>  
>  	return slots;
> @@ -157,7 +153,7 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  
>  	slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp,
>  						     limits->min_bpp, limits,
> -						     conn_state, 2 * 3, false);
> +						     conn_state, 2 * 3);
>  
>  	if (slots < 0)
>  		return slots;
> @@ -173,99 +169,6 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> -static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder,
> -						struct intel_crtc_state *crtc_state,
> -						struct drm_connector_state *conn_state,
> -						struct link_config_limits *limits)
> -{
> -	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> -	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> -	struct intel_connector *connector =
> -		to_intel_connector(conn_state->connector);
> -	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> -	const struct drm_display_mode *adjusted_mode =
> -		&crtc_state->hw.adjusted_mode;
> -	int slots = -EINVAL;
> -	int i, num_bpc;
> -	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;
> -
> -	/* 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, conn_state->max_requested_bpc);
> -	else
> -		dsc_max_bpc = min_t(u8, 10, conn_state->max_requested_bpc);
> -
> -	max_bpp = min_t(u8, dsc_max_bpc * 3, limits->max_bpp);
> -	min_bpp = limits->min_bpp;
> -
> -	num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> -						       dsc_bpc);
> -
> -	drm_dbg_kms(&i915->drm, "DSC Source supported min bpp %d max bpp %d\n",
> -		    min_bpp, max_bpp);
> -
> -	sink_max_bpp = dsc_bpc[0] * 3;
> -	sink_min_bpp = sink_max_bpp;
> -
> -	for (i = 1; i < num_bpc; i++) {
> -		if (sink_min_bpp > dsc_bpc[i] * 3)
> -			sink_min_bpp = dsc_bpc[i] * 3;
> -		if (sink_max_bpp < dsc_bpc[i] * 3)
> -			sink_max_bpp = dsc_bpc[i] * 3;
> -	}
> -
> -	drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n",
> -		    sink_min_bpp, sink_max_bpp);
> -
> -	if (min_bpp < sink_min_bpp)
> -		min_bpp = sink_min_bpp;
> -
> -	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;
> -
> -	crtc_state->dsc.compressed_bpp = intel_dp_dsc_nearest_valid_bpp(i915,
> -									last_compressed_bpp,
> -									crtc_state->pipe_bpp);
> -
> -	if (crtc_state->dsc.compressed_bpp != last_compressed_bpp)
> -		need_timeslot_recalc = 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;
> -	}
> -
> -	intel_link_compute_m_n(crtc_state->dsc.compressed_bpp,
> -			       crtc_state->lane_count,
> -			       adjusted_mode->crtc_clock,
> -			       crtc_state->port_clock,
> -			       &crtc_state->dp_m_n,
> -			       crtc_state->fec_enable);
> -	crtc_state->dp_m_n.tu = slots;
> -
> -	return 0;
> -}
>  static int intel_dp_mst_update_slots(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *crtc_state,
>  				     struct drm_connector_state *conn_state)
> @@ -349,29 +252,6 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	ret = intel_dp_mst_compute_link_config(encoder, pipe_config,
>  					       conn_state, &limits);
> -
> -	if (ret == -EDEADLK)
> -		return ret;
> -
> -	/* 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) {
> -		/*
> -		 * Try to get at least some timeslots and then see, if
> -		 * we can fit there with DSC.
> -		 */
> -		drm_dbg_kms(&dev_priv->drm, "Trying to find VCPI slots in DSC mode\n");
> -
> -		ret = intel_dp_dsc_mst_compute_link_config(encoder, pipe_config,
> -							   conn_state, &limits);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
> -						  conn_state, &limits,
> -						  pipe_config->dp_m_n.tu, false);
> -	}
> -
>  	if (ret)
>  		return ret;
>  
> @@ -909,10 +789,6 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
>  	int ret;
> -	bool dsc = false, bigjoiner = false;
> -	u16 dsc_max_output_bpp = 0;
> -	u8 dsc_slice_count = 0;
> -	int target_clock = mode->clock;
>  
>  	if (drm_connector_is_unregistered(connector)) {
>  		*status = MODE_ERROR;
> @@ -950,48 +826,6 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  		return 0;
>  	}
>  
> -	if (intel_dp_need_bigjoiner(intel_dp, mode->hdisplay, target_clock)) {
> -		bigjoiner = true;
> -		max_dotclk *= 2;
> -	}
> -
> -	if (DISPLAY_VER(dev_priv) >= 10 &&
> -	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> -		/*
> -		 * 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);
> -
> -		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,
> -							    pipe_bpp, 64) >> 4;
> -			dsc_slice_count =
> -				intel_dp_dsc_get_slice_count(intel_dp,
> -							     target_clock,
> -							     mode->hdisplay,
> -							     bigjoiner);
> -		}
> -
> -		dsc = dsc_max_output_bpp && dsc_slice_count;
> -	}
> -
> -	/*
> -	 * Big joiner configuration needs DSC for TGL which is not true for
> -	 * XE_LPD where uncompressed joiner is supported.
> -	 */
> -	if (DISPLAY_VER(dev_priv) < 13 && bigjoiner && !dsc)
> -		return MODE_CLOCK_HIGH;
> -
> -	if (mode_rate > max_rate && !dsc)
> -		return MODE_CLOCK_HIGH;
> -
>  	*status = intel_mode_valid_max_plane_size(dev_priv, mode, false);
>  	return 0;
>  }
> -- 
> 2.39.2
> 

Re: [PATCH 02/11] drm/i915/mst: Remove broken MST DSC support

[Lisovskiy, Stanislav]

On Tue, May 02, 2023 at 05:38:57PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The MST DSC code has a myriad of issues:
> - Platform checks are wrong (MST+DSC is TGL+ only IIRC)
> - Return values of .mode_valid_ctx() are wrong
> - .mode_valid_ctx() assumes bigjoiner might be used, but ther rest
>   of the code doesn't agree
> - compressed bpp calculations don't make sense
> - FEC handling needs to consider the entire link as opposed to just
>   the single stream. Currently FEC would only get enabled if the
>   first enabled stream is compressed. Also I'm not seeing anything
>   that would account for the FEC overhead in any bandwidth calculations
> - PPS SDP is only handled for the first stream via the dig_port
>   hooks, other streams will not be transmittitng any PPS SDPs
> - PPS SDP readout is missing (also missing for SST!)
> - VDSC readout is missing (also missing for SST!)
> 
> The FEC issues is really the big one since we have no way currently
> to apply such link wide configuration constraints. Changing that is
> going to require a much bigger rework of the higher level modeset
> .compute_config() logic. We will also need such a rework to properly
> distribute the available bandwidth across all the streams on the
> same link (which is a must to eg. enable deep color).

Also all the things you mentioned are subject for discussion, for example
I see that FEC overhead is actually accounted for bpp calculation for instance.
We usually improve things by gradually fixing, because if we act same way towards
all wrong code in the driver, we could end up removing the whole i915.
So from my side I would nack it, at least until you have a code which handles
all of this better - I have no doubt you probably have some ideas in your mind, so lets be constructive at least and propose something better first.
This code doesn't cause any regressions, but still provides "some" support to DP MST DSC to say the least and even if that would be removed, if some of those users 
refer to me, I would probably then just point to this mail discussion everytime.

Stan


> 
> Cc: stable@vger.kernel.org
> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: d51f25eb479a ("drm/i915: Add DSC support to MST path")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 176 +-------------------
>  1 file changed, 5 insertions(+), 171 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 44c15d6faac4..d762f37fafb5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -72,8 +72,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  						int min_bpp,
>  						struct link_config_limits *limits,
>  						struct drm_connector_state *conn_state,
> -						int step,
> -						bool dsc)
> +						int step)
>  {
>  	struct drm_atomic_state *state = crtc_state->uapi.state;
>  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> @@ -104,7 +103,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
>  		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
>  
> -		ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, dsc);
> +		ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, false);
>  		if (ret)
>  			continue;
>  
> @@ -136,11 +135,8 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  		drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n",
>  			    slots);
>  	} else {
> -		if (!dsc)
> -			crtc_state->pipe_bpp = bpp;
> -		else
> -			crtc_state->dsc.compressed_bpp = bpp;
> -		drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d dsc %d\n", slots, bpp, dsc);
> +		crtc_state->pipe_bpp = bpp;
> +		drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d\n", slots, bpp);
>  	}
>  
>  	return slots;
> @@ -157,7 +153,7 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  
>  	slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, limits->max_bpp,
>  						     limits->min_bpp, limits,
> -						     conn_state, 2 * 3, false);
> +						     conn_state, 2 * 3);
>  
>  	if (slots < 0)
>  		return slots;
> @@ -173,99 +169,6 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> -static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder,
> -						struct intel_crtc_state *crtc_state,
> -						struct drm_connector_state *conn_state,
> -						struct link_config_limits *limits)
> -{
> -	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> -	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> -	struct intel_connector *connector =
> -		to_intel_connector(conn_state->connector);
> -	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> -	const struct drm_display_mode *adjusted_mode =
> -		&crtc_state->hw.adjusted_mode;
> -	int slots = -EINVAL;
> -	int i, num_bpc;
> -	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;
> -
> -	/* 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, conn_state->max_requested_bpc);
> -	else
> -		dsc_max_bpc = min_t(u8, 10, conn_state->max_requested_bpc);
> -
> -	max_bpp = min_t(u8, dsc_max_bpc * 3, limits->max_bpp);
> -	min_bpp = limits->min_bpp;
> -
> -	num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> -						       dsc_bpc);
> -
> -	drm_dbg_kms(&i915->drm, "DSC Source supported min bpp %d max bpp %d\n",
> -		    min_bpp, max_bpp);
> -
> -	sink_max_bpp = dsc_bpc[0] * 3;
> -	sink_min_bpp = sink_max_bpp;
> -
> -	for (i = 1; i < num_bpc; i++) {
> -		if (sink_min_bpp > dsc_bpc[i] * 3)
> -			sink_min_bpp = dsc_bpc[i] * 3;
> -		if (sink_max_bpp < dsc_bpc[i] * 3)
> -			sink_max_bpp = dsc_bpc[i] * 3;
> -	}
> -
> -	drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n",
> -		    sink_min_bpp, sink_max_bpp);
> -
> -	if (min_bpp < sink_min_bpp)
> -		min_bpp = sink_min_bpp;
> -
> -	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;
> -
> -	crtc_state->dsc.compressed_bpp = intel_dp_dsc_nearest_valid_bpp(i915,
> -									last_compressed_bpp,
> -									crtc_state->pipe_bpp);
> -
> -	if (crtc_state->dsc.compressed_bpp != last_compressed_bpp)
> -		need_timeslot_recalc = 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;
> -	}
> -
> -	intel_link_compute_m_n(crtc_state->dsc.compressed_bpp,
> -			       crtc_state->lane_count,
> -			       adjusted_mode->crtc_clock,
> -			       crtc_state->port_clock,
> -			       &crtc_state->dp_m_n,
> -			       crtc_state->fec_enable);
> -	crtc_state->dp_m_n.tu = slots;
> -
> -	return 0;
> -}
>  static int intel_dp_mst_update_slots(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *crtc_state,
>  				     struct drm_connector_state *conn_state)
> @@ -349,29 +252,6 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	ret = intel_dp_mst_compute_link_config(encoder, pipe_config,
>  					       conn_state, &limits);
> -
> -	if (ret == -EDEADLK)
> -		return ret;
> -
> -	/* 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) {
> -		/*
> -		 * Try to get at least some timeslots and then see, if
> -		 * we can fit there with DSC.
> -		 */
> -		drm_dbg_kms(&dev_priv->drm, "Trying to find VCPI slots in DSC mode\n");
> -
> -		ret = intel_dp_dsc_mst_compute_link_config(encoder, pipe_config,
> -							   conn_state, &limits);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
> -						  conn_state, &limits,
> -						  pipe_config->dp_m_n.tu, false);
> -	}
> -
>  	if (ret)
>  		return ret;
>  
> @@ -909,10 +789,6 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
>  	int ret;
> -	bool dsc = false, bigjoiner = false;
> -	u16 dsc_max_output_bpp = 0;
> -	u8 dsc_slice_count = 0;
> -	int target_clock = mode->clock;
>  
>  	if (drm_connector_is_unregistered(connector)) {
>  		*status = MODE_ERROR;
> @@ -950,48 +826,6 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  		return 0;
>  	}
>  
> -	if (intel_dp_need_bigjoiner(intel_dp, mode->hdisplay, target_clock)) {
> -		bigjoiner = true;
> -		max_dotclk *= 2;
> -	}
> -
> -	if (DISPLAY_VER(dev_priv) >= 10 &&
> -	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> -		/*
> -		 * 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);
> -
> -		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,
> -							    pipe_bpp, 64) >> 4;
> -			dsc_slice_count =
> -				intel_dp_dsc_get_slice_count(intel_dp,
> -							     target_clock,
> -							     mode->hdisplay,
> -							     bigjoiner);
> -		}
> -
> -		dsc = dsc_max_output_bpp && dsc_slice_count;
> -	}
> -
> -	/*
> -	 * Big joiner configuration needs DSC for TGL which is not true for
> -	 * XE_LPD where uncompressed joiner is supported.
> -	 */
> -	if (DISPLAY_VER(dev_priv) < 13 && bigjoiner && !dsc)
> -		return MODE_CLOCK_HIGH;
> -
> -	if (mode_rate > max_rate && !dsc)
> -		return MODE_CLOCK_HIGH;
> -
>  	*status = intel_mode_valid_max_plane_size(dev_priv, mode, false);
>  	return 0;
>  }
> -- 
> 2.39.2
> 

Re: [PATCH 02/11] drm/i915/mst: Remove broken MST DSC support

[Ville Syrjälä]

On Wed, May 03, 2023 at 10:36:42AM +0300, Lisovskiy, Stanislav wrote:
> On Tue, May 02, 2023 at 05:38:57PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The MST DSC code has a myriad of issues:
> > - Platform checks are wrong (MST+DSC is TGL+ only IIRC)
> > - Return values of .mode_valid_ctx() are wrong
> > - .mode_valid_ctx() assumes bigjoiner might be used, but ther rest
> >   of the code doesn't agree
> > - compressed bpp calculations don't make sense
> > - FEC handling needs to consider the entire link as opposed to just
> >   the single stream. Currently FEC would only get enabled if the
> >   first enabled stream is compressed. Also I'm not seeing anything
> >   that would account for the FEC overhead in any bandwidth calculations
> > - PPS SDP is only handled for the first stream via the dig_port
> >   hooks, other streams will not be transmittitng any PPS SDPs
> > - PPS SDP readout is missing (also missing for SST!)
> > - VDSC readout is missing (also missing for SST!)
> > 
> > The FEC issues is really the big one since we have no way currently
> > to apply such link wide configuration constraints. Changing that is
> > going to require a much bigger rework of the higher level modeset
> > .compute_config() logic. We will also need such a rework to properly
> > distribute the available bandwidth across all the streams on the
> > same link (which is a must to eg. enable deep color).
> 
> Also all the things you mentioned are subject for discussion, for example
> I see that FEC overhead is actually accounted for bpp calculation for instance.

AFAICS FEC is only accounted for in the data M/N calculations,
assuming that particular stream happened to be compressed. I'm
not sure if that actually matters since at least the link M/N
are not even used by the MST sink. I suppose the data M/N might
still be used for something though. For any uncompressed stream
on the same link the data M/N values will be calculated
incorrectly without FEC.

And as mentioned, the FEC bandwidth overhead doesn't seem to
be accounted anywhere so no guarantee that we won't try to
oversubcribe the link.

And FEC will only be enabled if the first stream to be enabled
is compressed, otherwise we will enable the link without FEC
and still try to cram other compressed streams through it
(albeit without the PPS SDP so who knows what will happen)
and that is illegal.

> We usually improve things by gradually fixing, because if we act same way towards
> all wrong code in the driver, we could end up removing the whole i915.

We ususally don't merge code that has this many obvious and/or
fundemental issues.

Now, most of the issues I listed above are probably fixable
in a way that could be backported to stable kernels, but
unfortunately the FEC issue is not one of those. That one
will likely need massive amounts of work all over the driver
modeset code, making a backport impossible.

> So from my side I would nack it, at least until you have a code which handles
> all of this better - I have no doubt you probably have some ideas in your mind, so lets be constructive at least and propose something better first.
> This code doesn't cause any regressions, but still provides "some" support to DP MST DSC to say the least and even if that would be removed, if some of those users 
> refer to me, I would probably then just point to this mail discussion everytime.

It seems very likely that it will cause regressions at some point,
it just needs a specific multi-display MST setup. The resulting
problems will be very confusing to debug since the order in which
you enable/disable the outputs will have an impact on what actually
goes wrong on account of the FEC and PPS SDP issues. The longer
we wait disabling this the harder it will be to deal with those
regressions since we the probably can't revert anymore (a straight
revert was already not possible) but also can't fix it in a way
that can be backported (due to the FEC issues in particular).

-- 
Ville Syrjälä
Intel

[PATCH v2 08/11] drm/i915: Introduce crtc_state->enhanced_framing

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Track DP enhanced framing properly in the crtc state instead
of relying just on the cached DPCD everywhere, and hook it
up into the state check and dump.

v2: Actually set enhanced_framing in .compute_config()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/g4x_dp.c                 | 10 ++++++++--
 drivers/gpu/drm/i915/display/intel_crt.c              |  2 ++
 drivers/gpu/drm/i915/display/intel_crtc_state_dump.c  |  5 +++--
 drivers/gpu/drm/i915/display/intel_ddi.c              | 11 +++++++++--
 drivers/gpu/drm/i915/display/intel_display.c          |  1 +
 drivers/gpu/drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_dp.c               |  3 +++
 drivers/gpu/drm/i915/display/intel_dp_link_training.c |  2 +-
 8 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
index 920d570f7594..534546ea7d0b 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -141,7 +141,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
 
 		intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
 			     TRANS_DP_ENH_FRAMING,
-			     drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
+			     pipe_config->enhanced_framing ?
 			     TRANS_DP_ENH_FRAMING : 0);
 	} else {
 		if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
@@ -153,7 +153,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
 			intel_dp->DP |= DP_SYNC_VS_HIGH;
 		intel_dp->DP |= DP_LINK_TRAIN_OFF;
 
-		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+		if (pipe_config->enhanced_framing)
 			intel_dp->DP |= DP_ENHANCED_FRAMING;
 
 		if (IS_CHERRYVIEW(dev_priv))
@@ -351,6 +351,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 		u32 trans_dp = intel_de_read(dev_priv,
 					     TRANS_DP_CTL(crtc->pipe));
 
+		if (trans_dp & TRANS_DP_ENH_FRAMING)
+			pipe_config->enhanced_framing = true;
+
 		if (trans_dp & TRANS_DP_HSYNC_ACTIVE_HIGH)
 			flags |= DRM_MODE_FLAG_PHSYNC;
 		else
@@ -361,6 +364,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 		else
 			flags |= DRM_MODE_FLAG_NVSYNC;
 	} else {
+		if (tmp & DP_ENHANCED_FRAMING)
+			pipe_config->enhanced_framing = true;
+
 		if (tmp & DP_SYNC_HS_HIGH)
 			flags |= DRM_MODE_FLAG_PHSYNC;
 		else
diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
index 13519f78cf9f..52af64aa9953 100644
--- a/drivers/gpu/drm/i915/display/intel_crt.c
+++ b/drivers/gpu/drm/i915/display/intel_crt.c
@@ -449,6 +449,8 @@ static int hsw_crt_compute_config(struct intel_encoder *encoder,
 	/* FDI must always be 2.7 GHz */
 	pipe_config->port_clock = 135000 * 2;
 
+	pipe_config->enhanced_framing = true;
+
 	adjusted_mode->crtc_clock = lpt_iclkip(pipe_config);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 91242ffe0768..14db2b481ff1 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -257,8 +257,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
 		intel_dump_m_n_config(pipe_config, "dp m2_n2",
 				      pipe_config->lane_count,
 				      &pipe_config->dp_m2_n2);
-		drm_dbg_kms(&i915->drm, "fec: %s\n",
-			    str_enabled_disabled(pipe_config->fec_enable));
+		drm_dbg_kms(&i915->drm, "fec: %s, enhanced framing: %s\n",
+			    str_enabled_disabled(pipe_config->fec_enable),
+			    str_enabled_disabled(pipe_config->enhanced_framing));
 	}
 
 	drm_dbg_kms(&i915->drm, "framestart delay: %d, MSA timing delay: %d\n",
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 4246133950fd..51ae1aad7cc7 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3434,7 +3434,7 @@ static void mtl_ddi_prepare_link_retrain(struct intel_dp *intel_dp,
 		dp_tp_ctl |= DP_TP_CTL_MODE_MST;
 	} else {
 		dp_tp_ctl |= DP_TP_CTL_MODE_SST;
-		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+		if (crtc_state->enhanced_framing)
 			dp_tp_ctl |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 	}
 	intel_de_write(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), dp_tp_ctl);
@@ -3491,7 +3491,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp,
 		dp_tp_ctl |= DP_TP_CTL_MODE_MST;
 	} else {
 		dp_tp_ctl |= DP_TP_CTL_MODE_SST;
-		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+		if (crtc_state->enhanced_framing)
 			dp_tp_ctl |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 	}
 	intel_de_write(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), dp_tp_ctl);
@@ -3725,6 +3725,10 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
 		intel_cpu_transcoder_get_m2_n2(crtc, cpu_transcoder,
 					       &pipe_config->dp_m2_n2);
 
+		pipe_config->enhanced_framing =
+			intel_de_read(dev_priv, dp_tp_ctl_reg(encoder, pipe_config)) &
+			DP_TP_CTL_ENHANCED_FRAME_ENABLE;
+
 		if (DISPLAY_VER(dev_priv) >= 11)
 			pipe_config->fec_enable =
 				intel_de_read(dev_priv,
@@ -3741,6 +3745,9 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
 		if (!HAS_DP20(dev_priv)) {
 			/* FDI */
 			pipe_config->output_types |= BIT(INTEL_OUTPUT_ANALOG);
+			pipe_config->enhanced_framing =
+				intel_de_read(dev_priv, dp_tp_ctl_reg(encoder, pipe_config)) &
+				DP_TP_CTL_ENHANCED_FRAME_ENABLE;
 			break;
 		}
 		fallthrough; /* 128b/132b */
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3d3483e6f836..b95eb031abf2 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5226,6 +5226,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
 	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
 	PIPE_CONF_CHECK_BOOL(has_infoframe);
+	PIPE_CONF_CHECK_BOOL(enhanced_framing);
 	PIPE_CONF_CHECK_BOOL(fec_enable);
 
 	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 96a3183675be..9ea96eb19ddd 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1338,6 +1338,8 @@ struct intel_crtc_state {
 	u16 linetime;
 	u16 ips_linetime;
 
+	bool enhanced_framing;
+
 	/* Forward Error correction State */
 	bool fec_enable;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 9ac199444155..9b44ba99fd1d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2214,6 +2214,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->limited_color_range =
 		intel_dp_limited_color_range(pipe_config, conn_state);
 
+	pipe_config->enhanced_framing =
+		drm_dp_enhanced_frame_cap(intel_dp->dpcd);
+
 	if (pipe_config->dsc.compression_enable)
 		output_bpp = pipe_config->dsc.compressed_bpp;
 	else
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index e92c62bcc9b8..47a212a84fec 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -659,7 +659,7 @@ intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
 	/* Write the link configuration data */
 	link_config[0] = link_bw;
 	link_config[1] = crtc_state->lane_count;
-	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
+	if (crtc_state->enhanced_framing)
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
 
-- 
2.39.2

Re: [PATCH 02/11] drm/i915/mst: Remove broken MST DSC support

[Lisovskiy, Stanislav]

On Wed, May 03, 2023 at 02:07:04PM +0300, Ville Syrjälä wrote:
> On Wed, May 03, 2023 at 10:36:42AM +0300, Lisovskiy, Stanislav wrote:
> > On Tue, May 02, 2023 at 05:38:57PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The MST DSC code has a myriad of issues:
> > > - Platform checks are wrong (MST+DSC is TGL+ only IIRC)
> > > - Return values of .mode_valid_ctx() are wrong
> > > - .mode_valid_ctx() assumes bigjoiner might be used, but ther rest
> > >   of the code doesn't agree
> > > - compressed bpp calculations don't make sense
> > > - FEC handling needs to consider the entire link as opposed to just
> > >   the single stream. Currently FEC would only get enabled if the
> > >   first enabled stream is compressed. Also I'm not seeing anything
> > >   that would account for the FEC overhead in any bandwidth calculations
> > > - PPS SDP is only handled for the first stream via the dig_port
> > >   hooks, other streams will not be transmittitng any PPS SDPs
> > > - PPS SDP readout is missing (also missing for SST!)
> > > - VDSC readout is missing (also missing for SST!)
> > > 
> > > The FEC issues is really the big one since we have no way currently
> > > to apply such link wide configuration constraints. Changing that is
> > > going to require a much bigger rework of the higher level modeset
> > > .compute_config() logic. We will also need such a rework to properly
> > > distribute the available bandwidth across all the streams on the
> > > same link (which is a must to eg. enable deep color).
> > 
> > Also all the things you mentioned are subject for discussion, for example
> > I see that FEC overhead is actually accounted for bpp calculation for instance.
> 
> AFAICS FEC is only accounted for in the data M/N calculations,
> assuming that particular stream happened to be compressed. I'm
> not sure if that actually matters since at least the link M/N
> are not even used by the MST sink. I suppose the data M/N might
> still be used for something though. For any uncompressed stream
> on the same link the data M/N values will be calculated
> incorrectly without FEC.
> 
> And as mentioned, the FEC bandwidth overhead doesn't seem to
> be accounted anywhere so no guarantee that we won't try to
> oversubcribe the link.
> 
> And FEC will only be enabled if the first stream to be enabled
> is compressed, otherwise we will enable the link without FEC
> and still try to cram other compressed streams through it
> (albeit without the PPS SDP so who knows what will happen)
> and that is illegal.
> 
> > We usually improve things by gradually fixing, because if we act same way towards
> > all wrong code in the driver, we could end up removing the whole i915.
> 
> We ususally don't merge code that has this many obvious and/or
> fundemental issues.

Well, this is arguable and subjective judgement. Fact is that, so far we had more MST hubs
working with that code than without. Also no regressions or anything like that.
Moreover we usually merge code after code review, in particular those patches
did spend lots of time in review, where you could comment also.

Regarding merging code with fundamental issues, just recently you had admitted yourself
that bigjoiner issue for instance, we had recently, was partly caused by your code, because
we don't anymore copy the pll state to slave crtc. 
I would say that words like "obvious" and "fundamental"
issues can be applied to many things, however I thought that we always fix things in constructive,
but not destructive/negative way. 
Should I call also all code completely broken and remove it, once we discover some flaws 
there? Oh, we had many regressions, where I could say the same.

And once again I'm completely okay, if you did introduce better functionality instead
AND I know you have some valid points there, but now we are just removing everything completely,
without providing anything better.

But okay, I've mentioned what I think about this and from side this is nak. 
And once the guys to whom those patches helped will pop up from gitlab,
asking why their MST hubs stopped working - I will just refer them here.

> 
> Now, most of the issues I listed above are probably fixable
> in a way that could be backported to stable kernels, but
> unfortunately the FEC issue is not one of those. That one
> will likely need massive amounts of work all over the driver
> modeset code, making a backport impossible.
> 
> > So from my side I would nack it, at least until you have a code which handles
> > all of this better - I have no doubt you probably have some ideas in your mind, so lets be constructive at least and propose something better first.
> > This code doesn't cause any regressions, but still provides "some" support to DP MST DSC to say the least and even if that would be removed, if some of those users 
> > refer to me, I would probably then just point to this mail discussion everytime.
> 
> It seems very likely that it will cause regressions at some point,
> it just needs a specific multi-display MST setup. The resulting
> problems will be very confusing to debug since the order in which
> you enable/disable the outputs will have an impact on what actually
> goes wrong on account of the FEC and PPS SDP issues. The longer
> we wait disabling this the harder it will be to deal with those
> regressions since we the probably can't revert anymore (a straight
> revert was already not possible) but also can't fix it in a way
> that can be backported (due to the FEC issues in particular).
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 01/11] drm/dp_mst: Fix fractional DSC bpp handling

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks!

On Tue, 2023-05-02 at 17:38 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current code does '(bpp << 4) / 16' in the MST PBN
> calculation, but that is just the same as 'bpp' so the
> DSC codepath achieves absolutely nothing. Fix it up so that
> the fractional part of the bpp value is actually used instead
> of truncated away. 64*1006 has enough zero lsbs that we can
> just shift that down in the dividend and thus still manage
> to stick to a 32bit divisor.
> 
> And while touching this, let's just make the whole thing more
> straightforward by making the passed in bpp value .4 binary
> fixed point always, instead of having to pass in different
> things based on whether DSC is enabled or not.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: David Francis <David.Francis@amd.com>
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  2 +-
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  5 ++---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +--
>  .../gpu/drm/tests/drm_dp_mst_helper_test.c    |  2 +-
>  include/drm/display/drm_dp_mst_helper.h       |  2 +-
>  7 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 6cacb76f389e..7d58f08a5444 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6763,7 +6763,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>  								    max_bpc);
>  		bpp = convert_dc_color_depth_into_bpc(color_depth) * 3;
>  		clock = adjusted_mode->clock;
> -		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false);
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
>  	}
>  
>  	dm_new_connector_state->vcpi_slots =
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 994ba426ca66..eb4b666e50e8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -1515,7 +1515,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
>  	} else {
>  		/* check if mode could be supported within full_pbn */
>  		bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
> -		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
> +		pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
>  
>  		if (pbn > aconnector->mst_output_port->full_pbn)
>  			return DC_FAIL_BANDWIDTH_VALIDATE;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 38dab76ae69e..cd4c4f22c903 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4619,13 +4619,12 @@ EXPORT_SYMBOL(drm_dp_check_act_status);
>  
>  /**
>   * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode.
> - * @clock: dot clock for the mode
> - * @bpp: bpp for the mode.
> - * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel
> + * @clock: dot clock
> + * @bpp: bpp as .4 binary fixed point
>   *
>   * This uses the formula in the spec to calculate the PBN value for a mode.
>   */
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
> +int drm_dp_calc_pbn_mode(int clock, int bpp)
>  {
>  	/*
>  	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4636,18 +4635,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
>  	 * peak_kbps *= (1006/1000)
>  	 * peak_kbps *= (64/54)
>  	 * peak_kbps *= 8    convert to bytes
> -	 *
> -	 * If the bpp is in units of 1/16, further divide by 16. Put this
> -	 * factor in the numerator rather than the denominator to avoid
> -	 * integer overflow
>  	 */
> -
> -	if (dsc)
> -		return DIV_ROUND_UP_ULL(mul_u32_u32(clock * (bpp / 16), 64 * 1006),
> -					8 * 54 * 1000 * 1000);
> -
> -	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> -				8 * 54 * 1000 * 1000);
> +	return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
> +				1000 * 8 * 54 * 1000);
>  }
>  EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 2c49d9ab86a2..44c15d6faac4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -109,8 +109,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  			continue;
>  
>  		crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> -						       dsc ? bpp << 4 : bpp,
> -						       dsc);
> +						       bpp << 4);
>  
>  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>  						      connector->port,
> @@ -936,7 +935,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  		return ret;
>  
>  	if (mode_rate > max_rate || mode->clock > max_dotclk ||
> -	    drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
> +	    drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
>  		*status = MODE_CLOCK_HIGH;
>  		return 0;
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 5bb777ff1313..d896cbb8cf3d 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -961,8 +961,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  		const int clock = crtc_state->adjusted_mode.clock;
>  
>  		asyh->or.bpc = connector->display_info.bpc;
> -		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
> -						    false);
> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
>  	}
>  
>  	mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
> diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> index 545beea33e8c..39fc449148e1 100644
> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
>  {
>  	const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
>  
> -	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
> +	KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
>  			params->expected);
>  }
>  
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 32c764fb9cb5..c254500b4507 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -829,7 +829,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
>  int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
>  			     int link_rate, int link_lane_count);
>  
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
> +int drm_dp_calc_pbn_mode(int clock, int bpp);
>  
>  void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [Intel-gfx] [PATCH 03/11] drm/i915/mst: Read out FEC state

[Luca Coelho]

On Tue, 2023-05-02 at 17:38 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The MST codepath is missing FEC readout. Add it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 55f36d9d509c..41cfa28166e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3763,6 +3763,11 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
>  		intel_cpu_transcoder_get_m1_n1(crtc, cpu_transcoder,
>  					       &pipe_config->dp_m_n);
>  
> +		if (DISPLAY_VER(dev_priv) >= 11)
> +			pipe_config->fec_enable =
> +				intel_de_read(dev_priv,
> +					      dp_tp_ctl_reg(encoder, pipe_config)) & DP_TP_CTL_FEC_ENABLE;
> +
>  		pipe_config->infoframes.enable |=
>  			intel_hdmi_infoframes_enabled(encoder, pipe_config);
>  		break;

LGTM.

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 04/11] drm/i915: Fix FEC pipe A vs. DDI A mixup

[Luca Coelho]

On Tue, 2023-05-02 at 17:38 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On pre-TGL FEC is a port level feature, not a transcoder
> level features, and it's DDI A which doesn't have it, not
> trancodere A.

A couple of typos: "level feature" and "transcoder A".


>  Check for the correct thing when determining
> whether FEC is supported or not.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4361c1ac65c3..b27b4fb71ed7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1212,13 +1212,13 @@ void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
>  static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
>  					 const struct intel_crtc_state *pipe_config)
>  {
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  
> -	/* On TGL, FEC is supported on all Pipes */
>  	if (DISPLAY_VER(dev_priv) >= 12)
>  		return true;
>  
> -	if (DISPLAY_VER(dev_priv) == 11 && pipe_config->cpu_transcoder != TRANSCODER_A)
> +	if (DISPLAY_VER(dev_priv) == 11 && encoder->port != PORT_A)
>  		return true;
>  
>  	return false;

Other than that, looks good:

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 05/11] drm/i915: Check lane count when determining FEC support

[Luca Coelho]

On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ICL doesn't support FEC with a x1 DP link. Make sure
> we don't try to enable FEC in such cases.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index b27b4fb71ed7..9ac199444155 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1218,7 +1218,8 @@ static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
>  	if (DISPLAY_VER(dev_priv) >= 12)
>  		return true;
>  
> -	if (DISPLAY_VER(dev_priv) == 11 && encoder->port != PORT_A)
> +	if (DISPLAY_VER(dev_priv) == 11 &&
> +	    encoder->port != PORT_A && pipe_config->lane_count != 1)
>  		return true;
>  
>  	return false;
> @@ -1234,7 +1235,7 @@ static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
>  static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>  				  const struct intel_crtc_state *crtc_state)
>  {
> -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP) && !crtc_state->fec_enable)
> +	if (!intel_dp_is_edp(intel_dp) && !crtc_state->fec_enable)

I'm probably missing something, but this change...


>  		return false;
>  
>  	return intel_dsc_source_support(crtc_state) &&
> @@ -1580,15 +1581,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	int pipe_bpp;
>  	int ret;
>  
> -	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> -		intel_dp_supports_fec(intel_dp, pipe_config);
> -
> -	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
> -		return -EINVAL;
> -
> -	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> -		return -EINVAL;
> -
>  	if (compute_pipe_bpp)
>  		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
>  	else
> @@ -1615,6 +1607,15 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	pipe_config->port_clock = limits->max_rate;
>  	pipe_config->lane_count = limits->max_lane_count;
>  
> +	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> +		intel_dp_supports_fec(intel_dp, pipe_config);
> +
> +	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
> +		return -EINVAL;
> +
> +	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> +		return -EINVAL;
> +
>  	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,

...and this code move are not explained in the commit message? How are
they related?

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 06/11] drm/i915: Fix FEC state dump

[Luca Coelho]

On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Stop dumping state while reading it out. We have a proper
> place for that stuff.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_crtc_state_dump.c    |  2 ++
>  drivers/gpu/drm/i915/display/intel_ddi.c            | 13 +++----------
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 0cdcaa49656f..91242ffe0768 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -257,6 +257,8 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
>  		intel_dump_m_n_config(pipe_config, "dp m2_n2",
>  				      pipe_config->lane_count,
>  				      &pipe_config->dp_m2_n2);
> +		drm_dbg_kms(&i915->drm, "fec: %s\n",
> +			    str_enabled_disabled(pipe_config->fec_enable));
>  	}
>  
>  	drm_dbg_kms(&i915->drm, "framestart delay: %d, MSA timing delay: %d\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 41cfa28166e4..4246133950fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3725,17 +3725,10 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
>  		intel_cpu_transcoder_get_m2_n2(crtc, cpu_transcoder,
>  					       &pipe_config->dp_m2_n2);
>  
> -		if (DISPLAY_VER(dev_priv) >= 11) {
> -			i915_reg_t dp_tp_ctl = dp_tp_ctl_reg(encoder, pipe_config);
> -
> +		if (DISPLAY_VER(dev_priv) >= 11)
>  			pipe_config->fec_enable =
> -				intel_de_read(dev_priv, dp_tp_ctl) & DP_TP_CTL_FEC_ENABLE;
> -
> -			drm_dbg_kms(&dev_priv->drm,
> -				    "[ENCODER:%d:%s] Fec status: %u\n",
> -				    encoder->base.base.id, encoder->base.name,
> -				    pipe_config->fec_enable);
> -		}
> +				intel_de_read(dev_priv,
> +					      dp_tp_ctl_reg(encoder, pipe_config)) & DP_TP_CTL_FEC_ENABLE;
>  
>  		if (dig_port->lspcon.active && dig_port->dp.has_hdmi_sink)
>  			pipe_config->infoframes.enable |=

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 07/11] drm/i915: Split some long lines

[Luca Coelho]

On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Split some overly long lines.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fdi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

It doesn't matter much, but it would be nice to say where you're
splitting the long lines? If nothing else, it would at least make the
commit message more unique:

"drm/i915: Split some long lines in hsw_fdi_link_train()"

Other than this nitpick:

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v2 08/11] drm/i915: Introduce crtc_state->enhanced_framing

[Luca Coelho]

On Wed, 2023-05-03 at 14:36 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Track DP enhanced framing properly in the crtc state instead
> of relying just on the cached DPCD everywhere, and hook it
> up into the state check and dump.
> 
> v2: Actually set enhanced_framing in .compute_config()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/g4x_dp.c                 | 10 ++++++++--
>  drivers/gpu/drm/i915/display/intel_crt.c              |  2 ++
>  drivers/gpu/drm/i915/display/intel_crtc_state_dump.c  |  5 +++--
>  drivers/gpu/drm/i915/display/intel_ddi.c              | 11 +++++++++--
>  drivers/gpu/drm/i915/display/intel_display.c          |  1 +
>  drivers/gpu/drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_dp.c               |  3 +++
>  drivers/gpu/drm/i915/display/intel_dp_link_training.c |  2 +-
>  8 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> index 920d570f7594..534546ea7d0b 100644
> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> @@ -141,7 +141,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  
>  		intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
>  			     TRANS_DP_ENH_FRAMING,
> -			     drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
> +			     pipe_config->enhanced_framing ?
>  			     TRANS_DP_ENH_FRAMING : 0);
>  	} else {
>  		if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
> @@ -153,7 +153,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  			intel_dp->DP |= DP_SYNC_VS_HIGH;
>  		intel_dp->DP |= DP_LINK_TRAIN_OFF;
>  
> -		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> +		if (pipe_config->enhanced_framing)
>  			intel_dp->DP |= DP_ENHANCED_FRAMING;
>  
>  		if (IS_CHERRYVIEW(dev_priv))
> @@ -351,6 +351,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>  		u32 trans_dp = intel_de_read(dev_priv,
>  					     TRANS_DP_CTL(crtc->pipe));
>  
> +		if (trans_dp & TRANS_DP_ENH_FRAMING)
> +			pipe_config->enhanced_framing = true;
> +
>  		if (trans_dp & TRANS_DP_HSYNC_ACTIVE_HIGH)
>  			flags |= DRM_MODE_FLAG_PHSYNC;
>  		else
> @@ -361,6 +364,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>  		else
>  			flags |= DRM_MODE_FLAG_NVSYNC;
>  	} else {
> +		if (tmp & DP_ENHANCED_FRAMING)
> +			pipe_config->enhanced_framing = true;
> +
>  		if (tmp & DP_SYNC_HS_HIGH)
>  			flags |= DRM_MODE_FLAG_PHSYNC;
>  		else
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> index 13519f78cf9f..52af64aa9953 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -449,6 +449,8 @@ static int hsw_crt_compute_config(struct intel_encoder *encoder,
>  	/* FDI must always be 2.7 GHz */
>  	pipe_config->port_clock = 135000 * 2;
>  
> +	pipe_config->enhanced_framing = true;
> +

Just curious, why are you setting it to true by default here?

Otherwise, the changes look reasonable:

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 09/11] drm/i915: Stop spamming the logs with PLL state

[Luca Coelho]

On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> encoder->get_config() is not the place where the state
> should be dumped. Get rid of the spam.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 51ae1aad7cc7..65e031ff740c 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3863,11 +3863,9 @@ static void mtl_ddi_get_config(struct intel_encoder *encoder,
>  		crtc_state->port_clock = intel_mtl_tbt_calc_port_clock(encoder);
>  	} else if (intel_is_c10phy(i915, phy)) {
>  		intel_c10pll_readout_hw_state(encoder, &crtc_state->cx0pll_state.c10);
> -		intel_c10pll_dump_hw_state(i915, &crtc_state->cx0pll_state.c10);
>  		crtc_state->port_clock = intel_c10pll_calc_port_clock(encoder, &crtc_state->cx0pll_state.c10);
>  	} else {
>  		intel_c20pll_readout_hw_state(encoder, &crtc_state->cx0pll_state.c20);
> -		intel_c20pll_dump_hw_state(i915, &crtc_state->cx0pll_state.c20);
>  		crtc_state->port_clock = intel_c20pll_calc_port_clock(encoder, &crtc_state->cx0pll_state.c20);
>  	}
>  

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 10/11] drm/i915: Drop some redundant eDP checks

[Luca Coelho]

On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There's no need to check for both eDP and fixed_mode when
> deciding whether to do the pfit calculations or not.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

It would be nice to explain _why_ this is not needed.  Is it because
fixed_mode is always eDP?

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH 11/11] drm/i915: Reduce combo PHY log spam

[Luca Coelho]

On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We always check whether combo PHYs need to be re-initialized
> after disabling DC states, which leads to log spam. Switch things
> around so that we only log something when we actually have to
> re-initialized a PHY.
> 
> The log spam was exacerbated by commit 41b4c7fe72b6 ("drm/i915:
> Disable DC states for all commits") since we now disable DC
> states far more often.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_combo_phy.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> index 922a6d87b553..ee843f2b1af1 100644
> --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> @@ -114,10 +114,6 @@ static bool icl_verify_procmon_ref_values(struct drm_i915_private *dev_priv,
>  
>  	procmon = icl_get_procmon_ref_values(dev_priv, phy);
>  
> -	drm_dbg_kms(&dev_priv->drm,
> -		    "Combo PHY %c Voltage/Process Info : %s\n",
> -		    phy_name(phy), procmon->name);
> -
>  	ret = check_phy_reg(dev_priv, phy, ICL_PORT_COMP_DW1(phy),
>  			    (0xff << 16) | 0xff, procmon->dw1);
>  	ret &= check_phy_reg(dev_priv, phy, ICL_PORT_COMP_DW9(phy),
> @@ -312,14 +308,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
>  	enum phy phy;
>  
>  	for_each_combo_phy(dev_priv, phy) {
> +		const struct icl_procmon *procmon;
>  		u32 val;
>  
> -		if (icl_combo_phy_verify_state(dev_priv, phy)) {
> -			drm_dbg(&dev_priv->drm,
> -				"Combo PHY %c already enabled, won't reprogram it.\n",
> -				phy_name(phy));
> +		if (icl_combo_phy_verify_state(dev_priv, phy))
>  			continue;
> -		}
> +
> +		procmon = icl_get_procmon_ref_values(dev_priv, phy);
> +
> +		drm_dbg(&dev_priv->drm,
> +			"Initializing combo PHY %c (Voltage/Process Info : %s)\n",
> +			phy_name(phy), procmon->name);
>  
>  		if (!has_phy_misc(dev_priv, phy))
>  			goto skip_phy_misc;

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [PATCH 02/11] drm/i915/mst: Remove broken MST DSC support

[Dave Airlie]

On Wed, 3 May 2023 at 22:23, Lisovskiy, Stanislav
<stanislav.lisovskiy@intel.com> wrote:
>
> On Wed, May 03, 2023 at 02:07:04PM +0300, Ville Syrjälä wrote:
> > On Wed, May 03, 2023 at 10:36:42AM +0300, Lisovskiy, Stanislav wrote:
> > > On Tue, May 02, 2023 at 05:38:57PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > The MST DSC code has a myriad of issues:
> > > > - Platform checks are wrong (MST+DSC is TGL+ only IIRC)
> > > > - Return values of .mode_valid_ctx() are wrong
> > > > - .mode_valid_ctx() assumes bigjoiner might be used, but ther rest
> > > >   of the code doesn't agree
> > > > - compressed bpp calculations don't make sense
> > > > - FEC handling needs to consider the entire link as opposed to just
> > > >   the single stream. Currently FEC would only get enabled if the
> > > >   first enabled stream is compressed. Also I'm not seeing anything
> > > >   that would account for the FEC overhead in any bandwidth calculations
> > > > - PPS SDP is only handled for the first stream via the dig_port
> > > >   hooks, other streams will not be transmittitng any PPS SDPs
> > > > - PPS SDP readout is missing (also missing for SST!)
> > > > - VDSC readout is missing (also missing for SST!)
> > > >
> > > > The FEC issues is really the big one since we have no way currently
> > > > to apply such link wide configuration constraints. Changing that is
> > > > going to require a much bigger rework of the higher level modeset
> > > > .compute_config() logic. We will also need such a rework to properly
> > > > distribute the available bandwidth across all the streams on the
> > > > same link (which is a must to eg. enable deep color).
> > >
> > > Also all the things you mentioned are subject for discussion, for example
> > > I see that FEC overhead is actually accounted for bpp calculation for instance.
> >
> > AFAICS FEC is only accounted for in the data M/N calculations,
> > assuming that particular stream happened to be compressed. I'm
> > not sure if that actually matters since at least the link M/N
> > are not even used by the MST sink. I suppose the data M/N might
> > still be used for something though. For any uncompressed stream
> > on the same link the data M/N values will be calculated
> > incorrectly without FEC.
> >
> > And as mentioned, the FEC bandwidth overhead doesn't seem to
> > be accounted anywhere so no guarantee that we won't try to
> > oversubcribe the link.
> >
> > And FEC will only be enabled if the first stream to be enabled
> > is compressed, otherwise we will enable the link without FEC
> > and still try to cram other compressed streams through it
> > (albeit without the PPS SDP so who knows what will happen)
> > and that is illegal.
> >
> > > We usually improve things by gradually fixing, because if we act same way towards
> > > all wrong code in the driver, we could end up removing the whole i915.
> >
> > We ususally don't merge code that has this many obvious and/or
> > fundemental issues.
>
> Well, this is arguable and subjective judgement. Fact is that, so far we had more MST hubs
> working with that code than without. Also no regressions or anything like that.
> Moreover we usually merge code after code review, in particular those patches
> did spend lots of time in review, where you could comment also.
>
> Regarding merging code with fundamental issues, just recently you had admitted yourself
> that bigjoiner issue for instance, we had recently, was partly caused by your code, because
> we don't anymore copy the pll state to slave crtc.
> I would say that words like "obvious" and "fundamental"
> issues can be applied to many things, however I thought that we always fix things in constructive,
> but not destructive/negative way.
> Should I call also all code completely broken and remove it, once we discover some flaws
> there? Oh, we had many regressions, where I could say the same.
>
> And once again I'm completely okay, if you did introduce better functionality instead
> AND I know you have some valid points there, but now we are just removing everything completely,
> without providing anything better.
>
> But okay, I've mentioned what I think about this and from side this is nak.
> And once the guys to whom those patches helped will pop up from gitlab,
> asking why their MST hubs stopped working - I will just refer them here.
>
> >
> > Now, most of the issues I listed above are probably fixable
> > in a way that could be backported to stable kernels, but
> > unfortunately the FEC issue is not one of those. That one
> > will likely need massive amounts of work all over the driver
> > modeset code, making a backport impossible.
> >
> > > So from my side I would nack it, at least until you have a code which handles
> > > all of this better - I have no doubt you probably have some ideas in your mind, so lets be constructive at least and propose something better first.
> > > This code doesn't cause any regressions, but still provides "some" support to DP MST DSC to say the least and even if that would be removed, if some of those users
> > > refer to me, I would probably then just point to this mail discussion everytime.
> >
> > It seems very likely that it will cause regressions at some point,
> > it just needs a specific multi-display MST setup. The resulting
> > problems will be very confusing to debug since the order in which
> > you enable/disable the outputs will have an impact on what actually
> > goes wrong on account of the FEC and PPS SDP issues. The longer
> > we wait disabling this the harder it will be to deal with those
> > regressions since we the probably can't revert anymore (a straight
> > revert was already not possible) but also can't fix it in a way
> > that can be backported (due to the FEC issues in particular).

I don't think we can reasonably disable this either.

If there are any existant systems which work due to this even if it's
crap, merging this will regress them and Linus will make us revert it.

Sorry this isn't an engineering solution to the problem, going to have
to go around the long way.

Dave.

Re: [Intel-gfx] [PATCH v2 08/11] drm/i915: Introduce crtc_state->enhanced_framing

[Ville Syrjälä]

On Thu, May 25, 2023 at 12:51:28PM +0300, Luca Coelho wrote:
> On Wed, 2023-05-03 at 14:36 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Track DP enhanced framing properly in the crtc state instead
> > of relying just on the cached DPCD everywhere, and hook it
> > up into the state check and dump.
> > 
> > v2: Actually set enhanced_framing in .compute_config()
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/g4x_dp.c                 | 10 ++++++++--
> >  drivers/gpu/drm/i915/display/intel_crt.c              |  2 ++
> >  drivers/gpu/drm/i915/display/intel_crtc_state_dump.c  |  5 +++--
> >  drivers/gpu/drm/i915/display/intel_ddi.c              | 11 +++++++++--
> >  drivers/gpu/drm/i915/display/intel_display.c          |  1 +
> >  drivers/gpu/drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_dp.c               |  3 +++
> >  drivers/gpu/drm/i915/display/intel_dp_link_training.c |  2 +-
> >  8 files changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> > index 920d570f7594..534546ea7d0b 100644
> > --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> > +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> > @@ -141,7 +141,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
> >  
> >  		intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
> >  			     TRANS_DP_ENH_FRAMING,
> > -			     drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
> > +			     pipe_config->enhanced_framing ?
> >  			     TRANS_DP_ENH_FRAMING : 0);
> >  	} else {
> >  		if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
> > @@ -153,7 +153,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
> >  			intel_dp->DP |= DP_SYNC_VS_HIGH;
> >  		intel_dp->DP |= DP_LINK_TRAIN_OFF;
> >  
> > -		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> > +		if (pipe_config->enhanced_framing)
> >  			intel_dp->DP |= DP_ENHANCED_FRAMING;
> >  
> >  		if (IS_CHERRYVIEW(dev_priv))
> > @@ -351,6 +351,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
> >  		u32 trans_dp = intel_de_read(dev_priv,
> >  					     TRANS_DP_CTL(crtc->pipe));
> >  
> > +		if (trans_dp & TRANS_DP_ENH_FRAMING)
> > +			pipe_config->enhanced_framing = true;
> > +
> >  		if (trans_dp & TRANS_DP_HSYNC_ACTIVE_HIGH)
> >  			flags |= DRM_MODE_FLAG_PHSYNC;
> >  		else
> > @@ -361,6 +364,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
> >  		else
> >  			flags |= DRM_MODE_FLAG_NVSYNC;
> >  	} else {
> > +		if (tmp & DP_ENHANCED_FRAMING)
> > +			pipe_config->enhanced_framing = true;
> > +
> >  		if (tmp & DP_SYNC_HS_HIGH)
> >  			flags |= DRM_MODE_FLAG_PHSYNC;
> >  		else
> > diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> > index 13519f78cf9f..52af64aa9953 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> > @@ -449,6 +449,8 @@ static int hsw_crt_compute_config(struct intel_encoder *encoder,
> >  	/* FDI must always be 2.7 GHz */
> >  	pipe_config->port_clock = 135000 * 2;
> >  
> > +	pipe_config->enhanced_framing = true;
> > +
> 
> Just curious, why are you setting it to true by default here?

We always want to use enhanced framing with FDI. Setting it here
and doing the readout allows us to also state check it also for FDI.

> 
> Otherwise, the changes look reasonable:
> 
> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> 
> --
> Cheers,
> Luca.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 05/11] drm/i915: Check lane count when determining FEC support

[Ville Syrjälä]

On Thu, May 25, 2023 at 11:09:30AM +0300, Luca Coelho wrote:
> On Tue, 2023-05-02 at 17:39 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > ICL doesn't support FEC with a x1 DP link. Make sure
> > we don't try to enable FEC in such cases.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index b27b4fb71ed7..9ac199444155 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1218,7 +1218,8 @@ static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
> >  	if (DISPLAY_VER(dev_priv) >= 12)
> >  		return true;
> >  
> > -	if (DISPLAY_VER(dev_priv) == 11 && encoder->port != PORT_A)
> > +	if (DISPLAY_VER(dev_priv) == 11 &&
> > +	    encoder->port != PORT_A && pipe_config->lane_count != 1)
> >  		return true;
> >  
> >  	return false;
> > @@ -1234,7 +1235,7 @@ static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> >  static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
> >  				  const struct intel_crtc_state *crtc_state)
> >  {
> > -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP) && !crtc_state->fec_enable)
> > +	if (!intel_dp_is_edp(intel_dp) && !crtc_state->fec_enable)
> 
> I'm probably missing something, but this change...

This should have been a separate change I suppose. What this is
currently asserting is DP-SST needs FEC to use DSC, but so does DP-MST
which this is totally forgetting to check. eDP is only case where we
can skip FEC.

> 
> 
> >  		return false;
> >  
> >  	return intel_dsc_source_support(crtc_state) &&
> > @@ -1580,15 +1581,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  	int pipe_bpp;
> >  	int ret;
> >  
> > -	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> > -		intel_dp_supports_fec(intel_dp, pipe_config);
> > -
> > -	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
> > -		return -EINVAL;
> > -
> > -	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> > -		return -EINVAL;
> > -
> >  	if (compute_pipe_bpp)
> >  		pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
> >  	else
> > @@ -1615,6 +1607,15 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  	pipe_config->port_clock = limits->max_rate;
> >  	pipe_config->lane_count = limits->max_lane_count;
> >  
> > +	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> > +		intel_dp_supports_fec(intel_dp, pipe_config);
> > +
> > +	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
> > +		return -EINVAL;
> > +
> > +	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> > +		return -EINVAL;
> > +
> >  	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,
> 
> ...and this code move are not explained in the commit message? How are
> they related?

This is moved becaue we need to compute lanel_count before we can
actually check it.

-- 
Ville Syrjälä
Intel