[Intel-gfx] [PATCH v4 1/6] drm/i915/display: Share intel_connector_needs_modeset()

[Intel-gfx] [PATCH v4 1/6] drm/i915/display: Share intel_connector_needs_modeset()

[José Roberto de Souza]

intel_connector_needs_modeset() will be used outside of
intel_display.c in a future patch so it would only be necessary to
remove the state and add the prototype to the header file.

But while at it, I simplified the arguments and moved it to a better
place intel_atomic.c.

No behavior changes intended here.

v3:
- removed digital from exported version of intel_connector_needs_modeset
- rollback connector to drm type

v4:
- Renamed new_connector_state to new_conn_state
- Going back to drm_connector_state in
intel_encoders_update_prepare/complete as we also have
intel_tv_connector_state

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c  | 18 ++++++++
 drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
 drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++--------------
 3 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index fd0026fc3618..b7dda18b6f29 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector)
 	return &state->base;
 }
 
+/**
+ * intel_connector_needs_modeset - check if connector needs a modeset
+ */
+bool
+intel_connector_needs_modeset(struct intel_atomic_state *state,
+			      struct drm_connector *connector)
+{
+	const struct drm_connector_state *old_conn_state, *new_conn_state;
+
+	old_conn_state = drm_atomic_get_old_connector_state(&state->base, connector);
+	new_conn_state = drm_atomic_get_new_connector_state(&state->base, connector);
+
+	return old_conn_state->crtc != new_conn_state->crtc ||
+	       (new_conn_state->crtc &&
+		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
+									    new_conn_state->crtc)));
+}
+
 /**
  * intel_crtc_duplicate_state - duplicate crtc state
  * @crtc: drm crtc
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index 7b49623419ba..a7d1a8576c48 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -32,6 +32,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
 					 struct drm_atomic_state *state);
 struct drm_connector_state *
 intel_digital_connector_duplicate_state(struct drm_connector *connector);
+bool intel_connector_needs_modeset(struct intel_atomic_state *state,
+				   struct drm_connector *connector);
 
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 25af0ffe1c3a..25bf084427bf 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6168,39 +6168,23 @@ intel_connector_primary_encoder(struct intel_connector *connector)
 	return encoder;
 }
 
-static bool
-intel_connector_needs_modeset(struct intel_atomic_state *state,
-			      const struct drm_connector_state *old_conn_state,
-			      const struct drm_connector_state *new_conn_state)
-{
-	struct intel_crtc *old_crtc = old_conn_state->crtc ?
-				      to_intel_crtc(old_conn_state->crtc) : NULL;
-	struct intel_crtc *new_crtc = new_conn_state->crtc ?
-				      to_intel_crtc(new_conn_state->crtc) : NULL;
-
-	return new_crtc != old_crtc ||
-	       (new_crtc &&
-		needs_modeset(intel_atomic_get_new_crtc_state(state, new_crtc)));
-}
-
 static void intel_encoders_update_prepare(struct intel_atomic_state *state)
 {
-	struct drm_connector_state *old_conn_state;
 	struct drm_connector_state *new_conn_state;
-	struct drm_connector *conn;
+	struct drm_connector *connector;
 	int i;
 
-	for_each_oldnew_connector_in_state(&state->base, conn,
-					   old_conn_state, new_conn_state, i) {
+	for_each_new_connector_in_state(&state->base, connector, new_conn_state,
+					i) {
+		struct intel_connector *intel_connector;
 		struct intel_encoder *encoder;
 		struct intel_crtc *crtc;
 
-		if (!intel_connector_needs_modeset(state,
-						   old_conn_state,
-						   new_conn_state))
+		if (!intel_connector_needs_modeset(state, connector))
 			continue;
 
-		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
+		intel_connector = to_intel_connector(connector);
+		encoder = intel_connector_primary_encoder(intel_connector);
 		if (!encoder->update_prepare)
 			continue;
 
@@ -6212,22 +6196,21 @@ static void intel_encoders_update_prepare(struct intel_atomic_state *state)
 
 static void intel_encoders_update_complete(struct intel_atomic_state *state)
 {
-	struct drm_connector_state *old_conn_state;
 	struct drm_connector_state *new_conn_state;
-	struct drm_connector *conn;
+	struct drm_connector *connector;
 	int i;
 
-	for_each_oldnew_connector_in_state(&state->base, conn,
-					   old_conn_state, new_conn_state, i) {
+	for_each_new_connector_in_state(&state->base, connector, new_conn_state,
+					i) {
+		struct intel_connector *intel_connector;
 		struct intel_encoder *encoder;
 		struct intel_crtc *crtc;
 
-		if (!intel_connector_needs_modeset(state,
-						   old_conn_state,
-						   new_conn_state))
+		if (!intel_connector_needs_modeset(state, connector))
 			continue;
 
-		encoder = intel_connector_primary_encoder(to_intel_connector(conn));
+		intel_connector = to_intel_connector(connector);
+		encoder = intel_connector_primary_encoder(intel_connector);
 		if (!encoder->update_complete)
 			continue;
 
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 2/6] drm/i915/tgl: Select master transcoder for MST stream

[José Roberto de Souza]

On TGL the blending of all the streams have moved from DDI to
transcoder, so now every transcoder working over the same MST port must
send its stream to a master transcoder and master will send to DDI
respecting the time slots.

So here adding all the CRTCs that shares the same MST stream if
needed and computing their state again, it will pick the lowest
pipe/transcoder among the ones in the same stream to be master.

Most of the time skl_commit_modeset_enables() enables pipes in a
crescent order but due DDB overlapping it might not happen, this
scenarios will be handled in the next patch.

v2:
- Using recently added intel_crtc_state_reset() to set
mst_master_transcoder to invalid transcoder for all non gen12 & MST
code paths
- Setting lowest pipe/transcoder as master, previously it was the
first one but setting a predictable one will help in future MST e
port sync integration
- Moving to intel type as much as we can

v3:
- Now intel_dp_mst_master_trans_compute() returns the MST master transcoder
- Replaced stdbool.h by linux/types.h
- Skip the connector being checked in
intel_dp_mst_atomic_master_trans_check()
- Using pipe instead of transcoder to compute MST master

v4:
- renamed connector_state to conn_state

BSpec: 50493
BSpec: 49190
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
 drivers/gpu/drm/i915/display/intel_atomic.h   |   4 +
 drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
 drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
 .../drm/i915/display/intel_display_types.h    |   3 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 143 ++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
 7 files changed, 183 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index b7dda18b6f29..0eb973f65977 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct intel_atomic_state *state,
 									    new_conn_state->crtc)));
 }
 
+struct intel_digital_connector_state *
+intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
+					 struct intel_connector *connector)
+{
+	struct drm_connector_state *conn_state;
+
+	conn_state = drm_atomic_get_connector_state(&state->base,
+						    &connector->base);
+	if (IS_ERR(conn_state))
+		return ERR_CAST(conn_state);
+
+	return to_intel_digital_connector_state(conn_state);
+}
+
 /**
  * intel_crtc_duplicate_state - duplicate crtc state
  * @crtc: drm crtc
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
index a7d1a8576c48..74c749dbfb4f 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -17,6 +17,7 @@ struct drm_device;
 struct drm_i915_private;
 struct drm_property;
 struct intel_atomic_state;
+struct intel_connector;
 struct intel_crtc;
 struct intel_crtc_state;
 
@@ -34,6 +35,9 @@ struct drm_connector_state *
 intel_digital_connector_duplicate_state(struct drm_connector *connector);
 bool intel_connector_needs_modeset(struct intel_atomic_state *state,
 				   struct drm_connector *connector);
+struct intel_digital_connector_state *
+intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
+					 struct intel_connector *connector);
 
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index fa40ba7cbcad..9d99ec82d072 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1899,8 +1899,13 @@ intel_ddi_transcoder_func_reg_val_get(const struct intel_crtc_state *crtc_state)
 		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
 		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
 
-		if (INTEL_GEN(dev_priv) >= 12)
-			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
+		if (INTEL_GEN(dev_priv) >= 12) {
+			enum transcoder master;
+
+			master = crtc_state->mst_master_transcoder;
+			WARN_ON(master == INVALID_TRANSCODER);
+			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
+		}
 	} else {
 		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
 		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
@@ -4400,6 +4405,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
 		pipe_config->lane_count =
 			((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT) + 1;
+
+		if (INTEL_GEN(dev_priv) >= 12)
+			pipe_config->mst_master_transcoder =
+					REG_FIELD_GET(TRANS_DDI_MST_TRANSPORT_SELECT_MASK, temp);
+
 		intel_dp_get_m_n(intel_crtc, pipe_config);
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 25bf084427bf..59b3bfe8b721 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -46,6 +46,7 @@
 #include "display/intel_crt.h"
 #include "display/intel_ddi.h"
 #include "display/intel_dp.h"
+#include "display/intel_dp_mst.h"
 #include "display/intel_dsi.h"
 #include "display/intel_dvo.h"
 #include "display/intel_gmbus.h"
@@ -11630,6 +11631,7 @@ static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
 	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
 	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
 	crtc_state->scaler_state.scaler_id = -1;
+	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
 }
 
 /* Returns the currently programmed mode of the given encoder. */
@@ -12477,6 +12479,9 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
 			      pipe_config->csc_mode, pipe_config->gamma_mode,
 			      pipe_config->gamma_enable, pipe_config->csc_enable);
 
+	DRM_DEBUG_KMS("MST master transcoder: %s\n",
+		      transcoder_name(pipe_config->mst_master_transcoder));
+
 dump_planes:
 	if (!state)
 		return;
@@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
 	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
 	       sizeof(saved_state->icl_port_dplls));
 	saved_state->crc_enabled = crtc_state->crc_enabled;
+	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
@@ -13257,6 +13263,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_I(dsc.dsc_split);
 	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
 
+	PIPE_CONF_CHECK_I(mst_master_transcoder);
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
@@ -14341,7 +14349,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
 	u32 handled = 0;
 	int i;
 
-	/* Only disable port sync slaves */
+	/* Only disable port sync and MST slaves */
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (!needs_modeset(new_crtc_state))
@@ -14355,7 +14363,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
 		 * slave CRTCs are disabled first and then master CRTC since
 		 * Slave vblanks are masked till Master Vblanks.
 		 */
-		if (!is_trans_port_sync_slave(old_crtc_state))
+		if (!is_trans_port_sync_slave(old_crtc_state) &&
+		    !intel_dp_mst_is_slave_trans(old_crtc_state))
 			continue;
 
 		intel_pre_plane_update(state, crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 83ea04149b77..630a94892b7b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1054,6 +1054,9 @@ struct intel_crtc_state {
 
 	/* Bitmask to indicate slaves attached */
 	u8 sync_mode_slaves_mask;
+
+	/* Only valid on TGL+ */
+	enum transcoder mst_master_transcoder;
 };
 
 struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 7aa0975c33b7..710137984c71 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -87,6 +87,54 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	return 0;
 }
 
+/*
+ * Iterate over all connectors and set mst_master_transcoder to the smallest
+ * transcoder id that shares the same MST connector.
+ */
+static enum transcoder
+intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
+				  struct intel_crtc_state *crtc_state,
+				  struct drm_connector_state *connector_state)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
+	struct intel_connector *connector = to_intel_connector(connector_state->connector);
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_connector_state *iter_connector_state;
+	struct intel_connector *iter_connector;
+	enum pipe ret = I915_MAX_PIPES;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return INVALID_TRANSCODER;
+
+	for_each_new_intel_connector_in_state(state, iter_connector,
+					      iter_connector_state, i) {
+		struct intel_crtc_state *iter_crtc_state;
+		struct intel_crtc *iter_crtc;
+
+		if (connector->mst_port != iter_connector->mst_port ||
+		    !iter_connector_state->base.crtc)
+			continue;
+
+		iter_crtc = to_intel_crtc(iter_connector_state->base.crtc);
+		iter_crtc_state = intel_atomic_get_new_crtc_state(state,
+								  iter_crtc);
+		if (!iter_crtc_state->uapi.active)
+			continue;
+
+		/*
+		 * Using crtc->pipe because crtc_state->cpu_transcoder is
+		 * computed, so others pipe cpu_transcoder could have being
+		 * computed yet
+		 */
+		if (iter_crtc->pipe < ret)
+			ret = iter_crtc->pipe;
+	}
+
+	/* Simple cast works because TGL don't have a eDP transcoder */
+	return (enum transcoder)ret;
+}
+
 static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 				       struct intel_crtc_state *pipe_config,
 				       struct drm_connector_state *conn_state)
@@ -154,24 +202,89 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
 
+	pipe_config->mst_master_transcoder = intel_dp_mst_master_trans_compute(encoder,
+									       pipe_config,
+									       conn_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * If one of the connectors in a MST stream needs a modeset, mark all CRTCs
+ * that have a connector in the same MST stream as mode changed,
+ * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take to do a
+ * fastset when possible.
+ */
+static int
+intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
+				       struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct drm_connector_list_iter connector_list_iter;
+	struct intel_connector *connector_iter;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return  0;
+
+	if (!intel_connector_needs_modeset(state, &connector->base))
+		return 0;
+
+	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
+	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
+		struct intel_digital_connector_state *conn_iter_state;
+		struct intel_crtc_state *crtc_state;
+		struct intel_crtc *crtc;
+
+		if (connector_iter->mst_port != connector->mst_port ||
+		    connector_iter == connector)
+			continue;
+
+		conn_iter_state = intel_atomic_get_digital_connector_state(state,
+									   connector_iter);
+		if (IS_ERR(conn_iter_state)) {
+			drm_connector_list_iter_end(&connector_list_iter);
+			return PTR_ERR(conn_iter_state);
+		}
+
+		if (!conn_iter_state->base.crtc)
+			continue;
+
+		crtc = to_intel_crtc(conn_iter_state->base.crtc);
+		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(crtc_state)) {
+			drm_connector_list_iter_end(&connector_list_iter);
+			return PTR_ERR(crtc_state);
+		}
+
+		crtc_state->uapi.mode_changed = true;
+	}
+	drm_connector_list_iter_end(&connector_list_iter);
+
 	return 0;
 }
 
 static int
 intel_dp_mst_atomic_check(struct drm_connector *connector,
-			  struct drm_atomic_state *state)
+			  struct drm_atomic_state *_state)
 {
+	struct intel_atomic_state *state = to_intel_atomic_state(_state);
 	struct drm_connector_state *new_conn_state =
-		drm_atomic_get_new_connector_state(state, connector);
+		drm_atomic_get_new_connector_state(&state->base, connector);
 	struct drm_connector_state *old_conn_state =
-		drm_atomic_get_old_connector_state(state, connector);
+		drm_atomic_get_old_connector_state(&state->base, connector);
 	struct intel_connector *intel_connector =
 		to_intel_connector(connector);
 	struct drm_crtc *new_crtc = new_conn_state->crtc;
 	struct drm_dp_mst_topology_mgr *mgr;
 	int ret;
 
-	ret = intel_digital_connector_atomic_check(connector, state);
+	ret = intel_digital_connector_atomic_check(connector, &state->base);
+	if (ret)
+		return ret;
+
+	ret = intel_dp_mst_atomic_master_trans_check(intel_connector, state);
 	if (ret)
 		return ret;
 
@@ -182,12 +295,9 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
 	 * connector
 	 */
 	if (new_crtc) {
-		struct intel_atomic_state *intel_state =
-			to_intel_atomic_state(state);
 		struct intel_crtc *intel_crtc = to_intel_crtc(new_crtc);
 		struct intel_crtc_state *crtc_state =
-			intel_atomic_get_new_crtc_state(intel_state,
-							intel_crtc);
+			intel_atomic_get_new_crtc_state(state, intel_crtc);
 
 		if (!crtc_state ||
 		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi) ||
@@ -196,7 +306,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
 	}
 
 	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary->dp.mst_mgr;
-	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
+	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
 					       intel_connector->port);
 
 	return ret;
@@ -240,6 +350,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 
 	intel_dp->active_mst_links--;
 	last_mst_stream = intel_dp->active_mst_links == 0;
+	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
+		!intel_dp_mst_is_master_trans(old_crtc_state));
 
 	intel_crtc_vblank_off(old_crtc_state);
 
@@ -317,6 +429,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 	connector->encoder = encoder;
 	intel_mst->connector = connector;
 	first_mst_stream = intel_dp->active_mst_links == 0;
+	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
+		!intel_dp_mst_is_master_trans(pipe_config));
 
 	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 
@@ -722,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port)
 	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
 	/* encoders will get killed by normal cleanup */
 }
+
+bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state)
+{
+	return crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder;
+}
+
+bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
+{
+	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
+	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index f660ad80db04..854724f68f09 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -6,10 +6,15 @@
 #ifndef __INTEL_DP_MST_H__
 #define __INTEL_DP_MST_H__
 
+#include <linux/types.h>
+
 struct intel_digital_port;
+struct intel_crtc_state;
 
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
 int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
+bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
+bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 3/6] drm/i915/display: Always enables MST master pipe first

[José Roberto de Souza]

Due to DDB overlaps the pipe enabling sequence is not always crescent.
As the previous patch selects the smallest pipe/transcoder in the MST
stream to be master and it needs to be enabled first this changes
were needed to guarantee that.

So first lets enable all pipes that did not needed a fullmodeset so
it don't have any external dependency, this ones can overlap with
each other ddb allocations.

Then on the second loop it will enable all the pipes that needs a
modeset and don't depends on other pipes like MST master
pipe/transcoder.

Then finally all the pipes that needs a modeset and have dependency
on other pipes.

v3: rebased

v4:
- added check for modeset_pipes too to decide if is necessary for a
wait a vblank
- added DDB allocation overlap check for pipes that needs a modeset

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 108 ++++++++++++++-----
 1 file changed, 83 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 59b3bfe8b721..a4f252d05b37 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14517,15 +14517,21 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
 	u8 required_slices = state->wm_results.ddb.enabled_slices;
 	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
-	u8 dirty_pipes = 0;
+	const u8 num_pipes = INTEL_NUM_PIPES(dev_priv);
+	u8 update_pipes = 0, modeset_pipes = 0;
 	int i;
 
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		if (!new_crtc_state->hw.active)
+			continue;
+
 		/* ignore allocations for crtc's that have been turned off. */
-		if (!needs_modeset(new_crtc_state) && new_crtc_state->hw.active)
+		if (!needs_modeset(new_crtc_state)) {
 			entries[i] = old_crtc_state->wm.skl.ddb;
-		if (new_crtc_state->hw.active)
-			dirty_pipes |= BIT(crtc->pipe);
+			update_pipes |= BIT(crtc->pipe);
+		} else {
+			modeset_pipes |= BIT(modeset_pipes);
+		}
 	}
 
 	/* If 2nd DBuf slice required, enable it here */
@@ -14535,38 +14541,29 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 	/*
 	 * Whenever the number of active pipes changes, we need to make sure we
 	 * update the pipes in the right order so that their ddb allocations
-	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
+	 * never overlap with each other between CRTC updates. Otherwise we'll
 	 * cause pipe underruns and other bad stuff.
+	 *
+	 * So first lets enable all pipes that did not needed a fullmodeset so
+	 * it don't have any external dependency
 	 */
-	while (dirty_pipes) {
+	while (update_pipes) {
 		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 						    new_crtc_state, i) {
 			enum pipe pipe = crtc->pipe;
-			bool modeset = needs_modeset(new_crtc_state);
 
-			if ((dirty_pipes & BIT(pipe)) == 0)
+			if ((update_pipes & BIT(pipe)) == 0)
 				continue;
 
 			if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
-							entries,
-							INTEL_NUM_PIPES(dev_priv), i))
+							entries, num_pipes, i))
 				continue;
 
 			entries[i] = new_crtc_state->wm.skl.ddb;
-			dirty_pipes &= ~BIT(pipe);
-
-			if (modeset && is_trans_port_sync_mode(new_crtc_state)) {
-				if (is_trans_port_sync_master(new_crtc_state))
-					intel_update_trans_port_sync_crtcs(crtc,
-									   state,
-									   old_crtc_state,
-									   new_crtc_state);
-				else
-					continue;
-			} else {
-				intel_update_crtc(crtc, state, old_crtc_state,
-						  new_crtc_state);
-			}
+			update_pipes &= ~BIT(pipe);
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  new_crtc_state);
 
 			/*
 			 * If this is an already active pipe, it's DDB changed,
@@ -14576,11 +14573,72 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 			 */
 			if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
 						 &old_crtc_state->wm.skl.ddb) &&
-			    !modeset && dirty_pipes)
+			    (update_pipes | modeset_pipes))
 				intel_wait_for_vblank(dev_priv, pipe);
 		}
 	}
 
+	/*
+	 * Enabling all pipes that needs a modeset and do not depends on other
+	 * pipes
+	 */
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		enum pipe pipe = crtc->pipe;
+
+		if ((modeset_pipes & BIT(pipe)) == 0)
+			continue;
+
+		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
+		    is_trans_port_sync_slave(new_crtc_state))
+			continue;
+
+		WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
+						    entries, num_pipes, i));
+
+		entries[i] = new_crtc_state->wm.skl.ddb;
+		modeset_pipes &= ~BIT(pipe);
+
+		if (is_trans_port_sync_mode(new_crtc_state)) {
+			struct intel_crtc *slave_crtc;
+
+			intel_update_trans_port_sync_crtcs(crtc, state,
+							   old_crtc_state,
+							   new_crtc_state);
+
+			slave_crtc = intel_get_slave_crtc(new_crtc_state);
+			/* TODO: update entries[] of slave */
+			modeset_pipes &= ~BIT(slave_crtc->pipe);
+
+		} else {
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  new_crtc_state);
+		}
+	}
+
+	/*
+	 * Finally enable all pipes that needs a modeset and depends on
+	 * other pipes, right now it is only MST slaves as both port sync slave
+	 * and master are enabled together
+	 */
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		enum pipe pipe = crtc->pipe;
+
+		if ((modeset_pipes & BIT(pipe)) == 0)
+			continue;
+
+		WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
+						    entries, num_pipes, i));
+
+		entries[i] = new_crtc_state->wm.skl.ddb;
+		modeset_pipes &= ~BIT(pipe);
+
+		intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
+	}
+
+	WARN_ON(modeset_pipes);
+
 	/* If 2nd DBuf slice is no more required disable it */
 	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
 		icl_dbuf_slices_update(dev_priv, required_slices);
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 4/6] drm/i915/dp: Fix MST disable sequences

[José Roberto de Souza]

The disable sequence after wait for transcoder off was not correctly
implemented.
The MST disable sequence is basically the same for HSW, SKL, ICL and
TGL, with just minor changes for TGL.

With this last patch we finally fixed the hotplugs triggered by MST
sinks during the disable/enable sequence, those were causing source
to try to do a link training while it was not ready causing CPU pipe
FIFO underrrus on TGL.

v2: Only unsetting TGL_TRANS_DDI_PORT_MASK for TGL on the post
disable sequence

v4: Rebased, moved MST sequences to intel_mst_post_disable_dp()

BSpec: 4231
BSpec: 4163
BSpec: 22243
BSpec: 49190
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c    | 33 +++++++++++++++------
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 33 +++++++++++++--------
 2 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 9d99ec82d072..94ca26be2fee 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -34,6 +34,7 @@
 #include "intel_ddi.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
+#include "intel_dp_mst.h"
 #include "intel_dp_link_training.h"
 #include "intel_dpio_phy.h"
 #include "intel_dsi.h"
@@ -1949,17 +1950,19 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
-	u32 val = I915_READ(reg);
+	u32 val;
+
+	val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+	val &= ~TRANS_DDI_FUNC_ENABLE;
 
 	if (INTEL_GEN(dev_priv) >= 12) {
-		val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK |
-			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
+		if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) ||
+		    intel_dp_mst_is_slave_trans(crtc_state))
+			val &= ~TGL_TRANS_DDI_PORT_MASK;
 	} else {
-		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
-			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
+		val &= ~TRANS_DDI_PORT_MASK;
 	}
-	I915_WRITE(reg, val);
+	I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
 
 	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
 	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
@@ -3808,8 +3811,20 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 	 */
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 
-	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
-		intel_ddi_disable_pipe_clock(old_crtc_state);
+	if (INTEL_GEN(dev_priv) >= 12) {
+		if (is_mst) {
+			enum transcoder cpu_transcoder;
+			u32 val;
+
+			cpu_transcoder = old_crtc_state->cpu_transcoder;
+			val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+			val &= ~TGL_TRANS_DDI_PORT_MASK;
+			I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
+		}
+	} else {
+		if (!is_mst)
+			intel_ddi_disable_pipe_clock(old_crtc_state);
+	}
 
 	intel_disable_ddi_buf(encoder, old_crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 710137984c71..efd14b0b507b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -347,6 +347,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 		to_intel_connector(old_conn_state->connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	bool last_mst_stream;
+	u32 val;
 
 	intel_dp->active_mst_links--;
 	last_mst_stream = intel_dp->active_mst_links == 0;
@@ -357,6 +358,19 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 
 	intel_disable_pipe(old_crtc_state);
 
+	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
+
+	val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder));
+	val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC;
+	I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), val);
+
+	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
+				  DP_TP_STATUS_ACT_SENT, 1))
+		DRM_ERROR("Timed out waiting for ACT sent when disabling\n");
+	drm_dp_check_act_status(&intel_dp->mst_mgr);
+
+	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);
+
 	intel_ddi_disable_transcoder_func(old_crtc_state);
 
 	if (INTEL_GEN(dev_priv) >= 9)
@@ -364,6 +378,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	else
 		ironlake_pfit_disable(old_crtc_state);
 
+	/*
+	 * Power down mst path before disabling the port, otherwise we end
+	 * up getting interrupts from the sink upon detecting link loss.
+	 */
+	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
+				     false);
 	/*
 	 * From TGL spec: "If multi-stream slave transcoder: Configure
 	 * Transcoder Clock Select to direct no clock to the transcoder"
@@ -374,19 +394,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream)
 		intel_ddi_disable_pipe_clock(old_crtc_state);
 
-	/* this can fail */
-	drm_dp_check_act_status(&intel_dp->mst_mgr);
-	/* and this can also fail */
-	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
-
-	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);
-
-	/*
-	 * Power down mst path before disabling the port, otherwise we end
-	 * up getting interrupts from the sink upon detecting link loss.
-	 */
-	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
-				     false);
 
 	intel_mst->connector = NULL;
 	if (last_mst_stream)
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[José Roberto de Souza]

Check if fastset is allowed by external dependencies like other pipes
and transcoders.

Right now this patch only forces a fullmodeset in MST slaves of MST
masters that needs a fullmodeset but it will be needed for port sync
as well.

v3:
- moved handling to intel_atomic_check() this way is guarantee that
all pipes will have its state computed

v4:
- added a function to return if MST master neeeds modeset to simply
code in intel_atomic_check()

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
 3 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a4f252d05b37..2a406891567b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
 
 	new_crtc_state->uapi.mode_changed = false;
 	new_crtc_state->update_pipe = true;
-
-	/*
-	 * If we're not doing the full modeset we want to
-	 * keep the current M/N values as they may be
-	 * sufficiently different to the computed values
-	 * to cause problems.
-	 *
-	 * FIXME: should really copy more fuzzy state here
-	 */
-	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
-	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
-	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
-	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
 }
 
 static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
@@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct drm_device *dev,
 			any_ms = true;
 	}
 
+	/**
+	 * Check if fastset is allowed by external dependencies like other
+	 * pipes and transcoders.
+	 *
+	 * Right now it only forces a fullmodeset when the MST master
+	 * transcoder did not changed but the pipe of the master transcoder
+	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
+	 */
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+		enum transcoder master = new_crtc_state->mst_master_transcoder;
+
+		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
+		    needs_modeset(new_crtc_state))
+			continue;
+
+		if (intel_dp_mst_master_trans_needs_modeset(state, master)) {
+			new_crtc_state->uapi.mode_changed = true;
+			new_crtc_state->update_pipe = false;
+		}
+	}
+
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		if (needs_modeset(new_crtc_state))
+			continue;
+
+		/*
+		 * If we're not doing the full modeset we want to
+		 * keep the current M/N values as they may be
+		 * sufficiently different to the computed values
+		 * to cause problems.
+		 *
+		 * FIXME: should really copy more fuzzy state here
+		 */
+		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
+		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
+		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
+		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
+	}
+
 	if (any_ms && !check_digital_port_conflicts(state)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
 		ret = EINVAL;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index efd14b0b507b..4aba1d702a83 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
 	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
 	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
 }
+
+bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
+					     enum transcoder master)
+{
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
+		if (new_crtc_state->mst_master_transcoder == master)
+			return drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
+
+	return false;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index 854724f68f09..72cb486f32ab 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -8,6 +8,7 @@
 
 #include <linux/types.h>
 
+struct intel_atomic_state;
 struct intel_digital_port;
 struct intel_crtc_state;
 
@@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
 int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
 bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
 bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
+bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
+					     enum transcoder master);
 
 #endif /* __INTEL_DP_MST_H__ */
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 6/6] drm/i915/display: Add comment to a function that probably can be removed

[José Roberto de Souza]

This function is only called from port sync and it is identical to
what will be executed again in intel_update_crtc() over port sync
pipes.
If it is really necessary it at least deserves a better name and a
comment, leaving it to people working on port sync.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 2a406891567b..c4898d58e02e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14456,6 +14456,10 @@ static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
 	intel_dp_stop_link_train(intel_dp);
 }
 
+/*
+ * TODO: This is only called from port sync and it is identical to what will be
+ * executed again in intel_update_crtc() over port sync pipes
+ */
 static void intel_post_crtc_enable_updates(struct intel_crtc *crtc,
 					   struct intel_atomic_state *state)
 {
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/tgl: Select master transcoder for MST stream

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 10:59:06AM -0800, José Roberto de Souza wrote:
> On TGL the blending of all the streams have moved from DDI to
> transcoder, so now every transcoder working over the same MST port must
> send its stream to a master transcoder and master will send to DDI
> respecting the time slots.
> 
> So here adding all the CRTCs that shares the same MST stream if
> needed and computing their state again, it will pick the lowest
> pipe/transcoder among the ones in the same stream to be master.
> 
> Most of the time skl_commit_modeset_enables() enables pipes in a
> crescent order but due DDB overlapping it might not happen, this
> scenarios will be handled in the next patch.
> 
> v2:
> - Using recently added intel_crtc_state_reset() to set
> mst_master_transcoder to invalid transcoder for all non gen12 & MST
> code paths
> - Setting lowest pipe/transcoder as master, previously it was the
> first one but setting a predictable one will help in future MST e
> port sync integration
> - Moving to intel type as much as we can
> 
> v3:
> - Now intel_dp_mst_master_trans_compute() returns the MST master transcoder
> - Replaced stdbool.h by linux/types.h
> - Skip the connector being checked in
> intel_dp_mst_atomic_master_trans_check()
> - Using pipe instead of transcoder to compute MST master
> 
> v4:
> - renamed connector_state to conn_state
> 
> BSpec: 50493
> BSpec: 49190
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
>  drivers/gpu/drm/i915/display/intel_atomic.h   |   4 +
>  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
>  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
>  .../drm/i915/display/intel_display_types.h    |   3 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 143 ++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
>  7 files changed, 183 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index b7dda18b6f29..0eb973f65977 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct intel_atomic_state *state,
>  									    new_conn_state->crtc)));
>  }
>  
> +struct intel_digital_connector_state *
> +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> +					 struct intel_connector *connector)
> +{
> +	struct drm_connector_state *conn_state;
> +
> +	conn_state = drm_atomic_get_connector_state(&state->base,
> +						    &connector->base);
> +	if (IS_ERR(conn_state))
> +		return ERR_CAST(conn_state);
> +
> +	return to_intel_digital_connector_state(conn_state);
> +}
> +
>  /**
>   * intel_crtc_duplicate_state - duplicate crtc state
>   * @crtc: drm crtc
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index a7d1a8576c48..74c749dbfb4f 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -17,6 +17,7 @@ struct drm_device;
>  struct drm_i915_private;
>  struct drm_property;
>  struct intel_atomic_state;
> +struct intel_connector;
>  struct intel_crtc;
>  struct intel_crtc_state;
>  
> @@ -34,6 +35,9 @@ struct drm_connector_state *
>  intel_digital_connector_duplicate_state(struct drm_connector *connector);
>  bool intel_connector_needs_modeset(struct intel_atomic_state *state,
>  				   struct drm_connector *connector);
> +struct intel_digital_connector_state *
> +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> +					 struct intel_connector *connector);
>  
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index fa40ba7cbcad..9d99ec82d072 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1899,8 +1899,13 @@ intel_ddi_transcoder_func_reg_val_get(const struct intel_crtc_state *crtc_state)
>  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
>  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
>  
> -		if (INTEL_GEN(dev_priv) >= 12)
> -			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> +		if (INTEL_GEN(dev_priv) >= 12) {
> +			enum transcoder master;
> +
> +			master = crtc_state->mst_master_transcoder;
> +			WARN_ON(master == INVALID_TRANSCODER);
> +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> +		}
>  	} else {
>  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
>  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> @@ -4400,6 +4405,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
>  		pipe_config->lane_count =
>  			((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT) + 1;
> +
> +		if (INTEL_GEN(dev_priv) >= 12)
> +			pipe_config->mst_master_transcoder =
> +					REG_FIELD_GET(TRANS_DDI_MST_TRANSPORT_SELECT_MASK, temp);
> +
>  		intel_dp_get_m_n(intel_crtc, pipe_config);
>  		break;
>  	default:
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 25bf084427bf..59b3bfe8b721 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -46,6 +46,7 @@
>  #include "display/intel_crt.h"
>  #include "display/intel_ddi.h"
>  #include "display/intel_dp.h"
> +#include "display/intel_dp_mst.h"
>  #include "display/intel_dsi.h"
>  #include "display/intel_dvo.h"
>  #include "display/intel_gmbus.h"
> @@ -11630,6 +11631,7 @@ static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
>  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
>  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
>  	crtc_state->scaler_state.scaler_id = -1;
> +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
>  }
>  
>  /* Returns the currently programmed mode of the given encoder. */
> @@ -12477,6 +12479,9 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>  			      pipe_config->csc_mode, pipe_config->gamma_mode,
>  			      pipe_config->gamma_enable, pipe_config->csc_enable);
>  
> +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> +		      transcoder_name(pipe_config->mst_master_transcoder));
> +
>  dump_planes:
>  	if (!state)
>  		return;
> @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
>  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
>  	       sizeof(saved_state->icl_port_dplls));
>  	saved_state->crc_enabled = crtc_state->crc_enabled;
> +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;

That's redundant now?

>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
> @@ -13257,6 +13263,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_I(dsc.dsc_split);
>  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
>  
> +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL
> @@ -14341,7 +14349,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  	u32 handled = 0;
>  	int i;
>  
> -	/* Only disable port sync slaves */
> +	/* Only disable port sync and MST slaves */
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state))
> @@ -14355,7 +14363,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  		 * slave CRTCs are disabled first and then master CRTC since
>  		 * Slave vblanks are masked till Master Vblanks.
>  		 */
> -		if (!is_trans_port_sync_slave(old_crtc_state))
> +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
>  			continue;
>  
>  		intel_pre_plane_update(state, crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 83ea04149b77..630a94892b7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
>  
>  	/* Bitmask to indicate slaves attached */
>  	u8 sync_mode_slaves_mask;
> +
> +	/* Only valid on TGL+ */
> +	enum transcoder mst_master_transcoder;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7aa0975c33b7..710137984c71 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -87,6 +87,54 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> +/*
> + * Iterate over all connectors and set mst_master_transcoder to the smallest
> + * transcoder id that shares the same MST connector.
> + */
> +static enum transcoder
> +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> +				  struct intel_crtc_state *crtc_state,
> +				  struct drm_connector_state *connector_state)
> +{
> +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);

Could grab that from the connector state and not pass in crtc_state at
all?

Hmm. Might be even better to just pass inin intel_atomic_state+mst_port
so we wouldn't have the clash with the iterators and could use the
normal names for those. Shrug.


> +	struct intel_connector *connector = to_intel_connector(connector_state->connector);
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_connector_state *iter_connector_state;

Could sprinkle some consts on the crtc/connector states now that we're pure.

> +	struct intel_connector *iter_connector;
> +	enum pipe ret = I915_MAX_PIPES;
> +	int i;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return INVALID_TRANSCODER;
> +
> +	for_each_new_intel_connector_in_state(state, iter_connector,
> +					      iter_connector_state, i) {
> +		struct intel_crtc_state *iter_crtc_state;
> +		struct intel_crtc *iter_crtc;
> +
> +		if (connector->mst_port != iter_connector->mst_port ||
> +		    !iter_connector_state->base.crtc)
> +			continue;
> +
> +		iter_crtc = to_intel_crtc(iter_connector_state->base.crtc);
> +		iter_crtc_state = intel_atomic_get_new_crtc_state(state,
> +								  iter_crtc);
> +		if (!iter_crtc_state->uapi.active)
> +			continue;
> +
> +		/*
> +		 * Using crtc->pipe because crtc_state->cpu_transcoder is
> +		 * computed, so others pipe cpu_transcoder could have being
> +		 * computed yet
> +		 */
> +		if (iter_crtc->pipe < ret)
> +			ret = iter_crtc->pipe;
> +	}

What if we have no active pipes? Aren't we going to come here with
ret==MAX_PIPES?

I guess we can't really change the !active check to !enabled
because we surely can't use an inactive transcoder as the 
master. So !active does seem like the right thing to check.

> +
> +	/* Simple cast works because TGL don't have a eDP transcoder */
> +	return (enum transcoder)ret;

I'd have probably extracted the thing to compute the transcoder.
But doesn't matter for MST since it can't be going over the EDP
transcoder. Port sync will be a different story. We can cross that
bridge when we come to it.

> +}
> +
>  static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  				       struct intel_crtc_state *pipe_config,
>  				       struct drm_connector_state *conn_state)
> @@ -154,24 +202,89 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
>  
> +	pipe_config->mst_master_transcoder = intel_dp_mst_master_trans_compute(encoder,
> +									       pipe_config,
> +									       conn_state);
> +	if (ret)
> +		return ret;

ret?

> +
> +	return 0;
> +}
> +
> +/*
> + * If one of the connectors in a MST stream needs a modeset, mark all CRTCs
> + * that have a connector in the same MST stream as mode changed,
> + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take to do a
> + * fastset when possible.
> + */
> +static int
> +intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> +				       struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector_list_iter connector_list_iter;
> +	struct intel_connector *connector_iter;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return  0;
> +
> +	if (!intel_connector_needs_modeset(state, &connector->base))
> +		return 0;
> +
> +	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
> +	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
> +		struct intel_digital_connector_state *conn_iter_state;
> +		struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (connector_iter->mst_port != connector->mst_port ||
> +		    connector_iter == connector)
> +			continue;
> +
> +		conn_iter_state = intel_atomic_get_digital_connector_state(state,
> +									   connector_iter);
> +		if (IS_ERR(conn_iter_state)) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return PTR_ERR(conn_iter_state);
> +		}
> +
> +		if (!conn_iter_state->base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(conn_iter_state->base.crtc);
> +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return PTR_ERR(crtc_state);
> +		}
> +
> +		crtc_state->uapi.mode_changed = true;

We might need drm_atomic_add_affected_planes() here. The helper won't do
that for us if the connector->atomic_check() was called from the second
loop.

The rest lgtm.

> +	}
> +	drm_connector_list_iter_end(&connector_list_iter);
> +
>  	return 0;
>  }
>  
>  static int
>  intel_dp_mst_atomic_check(struct drm_connector *connector,
> -			  struct drm_atomic_state *state)
> +			  struct drm_atomic_state *_state)
>  {
> +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_connector_state *new_conn_state =
> -		drm_atomic_get_new_connector_state(state, connector);
> +		drm_atomic_get_new_connector_state(&state->base, connector);
>  	struct drm_connector_state *old_conn_state =
> -		drm_atomic_get_old_connector_state(state, connector);
> +		drm_atomic_get_old_connector_state(&state->base, connector);
>  	struct intel_connector *intel_connector =
>  		to_intel_connector(connector);
>  	struct drm_crtc *new_crtc = new_conn_state->crtc;
>  	struct drm_dp_mst_topology_mgr *mgr;
>  	int ret;
>  
> -	ret = intel_digital_connector_atomic_check(connector, state);
> +	ret = intel_digital_connector_atomic_check(connector, &state->base);
> +	if (ret)
> +		return ret;
> +
> +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector, state);
>  	if (ret)
>  		return ret;
>  
> @@ -182,12 +295,9 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
>  	 * connector
>  	 */
>  	if (new_crtc) {
> -		struct intel_atomic_state *intel_state =
> -			to_intel_atomic_state(state);
>  		struct intel_crtc *intel_crtc = to_intel_crtc(new_crtc);
>  		struct intel_crtc_state *crtc_state =
> -			intel_atomic_get_new_crtc_state(intel_state,
> -							intel_crtc);
> +			intel_atomic_get_new_crtc_state(state, intel_crtc);
>  
>  		if (!crtc_state ||
>  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi) ||
> @@ -196,7 +306,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
>  	}
>  
>  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary->dp.mst_mgr;
> -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
>  					       intel_connector->port);
>  
>  	return ret;
> @@ -240,6 +350,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  
>  	intel_dp->active_mst_links--;
>  	last_mst_stream = intel_dp->active_mst_links == 0;
> +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> +		!intel_dp_mst_is_master_trans(old_crtc_state));
>  
>  	intel_crtc_vblank_off(old_crtc_state);
>  
> @@ -317,6 +429,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  	connector->encoder = encoder;
>  	intel_mst->connector = connector;
>  	first_mst_stream = intel_dp->active_mst_links == 0;
> +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> +		!intel_dp_mst_is_master_trans(pipe_config));
>  
>  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>  
> @@ -722,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port)
>  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
>  	/* encoders will get killed by normal cleanup */
>  }
> +
> +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state)
> +{
> +	return crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder;
> +}
> +
> +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
> +{
> +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
> +	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f660ad80db04..854724f68f09 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -6,10 +6,15 @@
>  #ifndef __INTEL_DP_MST_H__
>  #define __INTEL_DP_MST_H__
>  
> +#include <linux/types.h>
> +
>  struct intel_digital_port;
> +struct intel_crtc_state;
>  
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>  void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
>  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
> +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
> +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 3/6] drm/i915/display: Always enables MST master pipe first

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 10:59:07AM -0800, José Roberto de Souza wrote:
> Due to DDB overlaps the pipe enabling sequence is not always crescent.
> As the previous patch selects the smallest pipe/transcoder in the MST
> stream to be master and it needs to be enabled first this changes
> were needed to guarantee that.
> 
> So first lets enable all pipes that did not needed a fullmodeset so
> it don't have any external dependency, this ones can overlap with
> each other ddb allocations.
> 
> Then on the second loop it will enable all the pipes that needs a
> modeset and don't depends on other pipes like MST master
> pipe/transcoder.
> 
> Then finally all the pipes that needs a modeset and have dependency
> on other pipes.
> 
> v3: rebased
> 
> v4:
> - added check for modeset_pipes too to decide if is necessary for a
> wait a vblank
> - added DDB allocation overlap check for pipes that needs a modeset
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 108 ++++++++++++++-----
>  1 file changed, 83 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 59b3bfe8b721..a4f252d05b37 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14517,15 +14517,21 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
>  	u8 required_slices = state->wm_results.ddb.enabled_slices;
>  	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> -	u8 dirty_pipes = 0;
> +	const u8 num_pipes = INTEL_NUM_PIPES(dev_priv);
> +	u8 update_pipes = 0, modeset_pipes = 0;
>  	int i;
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		if (!new_crtc_state->hw.active)
> +			continue;
> +
>  		/* ignore allocations for crtc's that have been turned off. */
> -		if (!needs_modeset(new_crtc_state) && new_crtc_state->hw.active)
> +		if (!needs_modeset(new_crtc_state)) {
>  			entries[i] = old_crtc_state->wm.skl.ddb;
> -		if (new_crtc_state->hw.active)
> -			dirty_pipes |= BIT(crtc->pipe);
> +			update_pipes |= BIT(crtc->pipe);
> +		} else {
> +			modeset_pipes |= BIT(modeset_pipes);
> +		}
>  	}
>  
>  	/* If 2nd DBuf slice required, enable it here */
> @@ -14535,38 +14541,29 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  	/*
>  	 * Whenever the number of active pipes changes, we need to make sure we
>  	 * update the pipes in the right order so that their ddb allocations
> -	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
> +	 * never overlap with each other between CRTC updates. Otherwise we'll
>  	 * cause pipe underruns and other bad stuff.
> +	 *
> +	 * So first lets enable all pipes that did not needed a fullmodeset so
> +	 * it don't have any external dependency
>  	 */
> -	while (dirty_pipes) {
> +	while (update_pipes) {
>  		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  						    new_crtc_state, i) {
>  			enum pipe pipe = crtc->pipe;
> -			bool modeset = needs_modeset(new_crtc_state);
>  
> -			if ((dirty_pipes & BIT(pipe)) == 0)
> +			if ((update_pipes & BIT(pipe)) == 0)
>  				continue;
>  
>  			if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> -							entries,
> -							INTEL_NUM_PIPES(dev_priv), i))
> +							entries, num_pipes, i))
>  				continue;
>  
>  			entries[i] = new_crtc_state->wm.skl.ddb;
> -			dirty_pipes &= ~BIT(pipe);
> -
> -			if (modeset && is_trans_port_sync_mode(new_crtc_state)) {
> -				if (is_trans_port_sync_master(new_crtc_state))
> -					intel_update_trans_port_sync_crtcs(crtc,
> -									   state,
> -									   old_crtc_state,
> -									   new_crtc_state);
> -				else
> -					continue;
> -			} else {
> -				intel_update_crtc(crtc, state, old_crtc_state,
> -						  new_crtc_state);
> -			}
> +			update_pipes &= ~BIT(pipe);
> +
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  new_crtc_state);
>  
>  			/*
>  			 * If this is an already active pipe, it's DDB changed,
> @@ -14576,11 +14573,72 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			 */
>  			if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
>  						 &old_crtc_state->wm.skl.ddb) &&
> -			    !modeset && dirty_pipes)
> +			    (update_pipes | modeset_pipes))
>  				intel_wait_for_vblank(dev_priv, pipe);
>  		}
>  	}
>  
> +	/*
> +	 * Enabling all pipes that needs a modeset and do not depends on other
> +	 * pipes
> +	 */
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		enum pipe pipe = crtc->pipe;
> +
> +		if ((modeset_pipes & BIT(pipe)) == 0)
> +			continue;
> +
> +		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    is_trans_port_sync_slave(new_crtc_state))
> +			continue;
> +
> +		WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> +						    entries, num_pipes, i));
> +
> +		entries[i] = new_crtc_state->wm.skl.ddb;
> +		modeset_pipes &= ~BIT(pipe);
> +
> +		if (is_trans_port_sync_mode(new_crtc_state)) {
> +			struct intel_crtc *slave_crtc;
> +
> +			intel_update_trans_port_sync_crtcs(crtc, state,
> +							   old_crtc_state,
> +							   new_crtc_state);
> +
> +			slave_crtc = intel_get_slave_crtc(new_crtc_state);
> +			/* TODO: update entries[] of slave */
> +			modeset_pipes &= ~BIT(slave_crtc->pipe);

OK. So we clear the bit for both master+slave at the same time. Should
work and not trip un the WARN at the end (assuming both have their
modeset properly flagged).

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

> +
> +		} else {
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  new_crtc_state);
> +		}
> +	}
> +
> +	/*
> +	 * Finally enable all pipes that needs a modeset and depends on
> +	 * other pipes, right now it is only MST slaves as both port sync slave
> +	 * and master are enabled together
> +	 */
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		enum pipe pipe = crtc->pipe;
> +
> +		if ((modeset_pipes & BIT(pipe)) == 0)
> +			continue;
> +
> +		WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> +						    entries, num_pipes, i));
> +
> +		entries[i] = new_crtc_state->wm.skl.ddb;
> +		modeset_pipes &= ~BIT(pipe);
> +
> +		intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
> +	}
> +
> +	WARN_ON(modeset_pipes);
> +
>  	/* If 2nd DBuf slice is no more required disable it */
>  	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
>  		icl_dbuf_slices_update(dev_priv, required_slices);
> -- 
> 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza wrote:
> Check if fastset is allowed by external dependencies like other pipes
> and transcoders.
> 
> Right now this patch only forces a fullmodeset in MST slaves of MST
> masters that needs a fullmodeset but it will be needed for port sync
> as well.
> 
> v3:
> - moved handling to intel_atomic_check() this way is guarantee that
> all pipes will have its state computed
> 
> v4:
> - added a function to return if MST master neeeds modeset to simply
> code in intel_atomic_check()
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
>  3 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a4f252d05b37..2a406891567b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
> -
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }
>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}
>  
> +	/**
> +	 * Check if fastset is allowed by external dependencies like other
> +	 * pipes and transcoders.
> +	 *
> +	 * Right now it only forces a fullmodeset when the MST master
> +	 * transcoder did not changed but the pipe of the master transcoder
> +	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
> +	 */
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		enum transcoder master = new_crtc_state->mst_master_transcoder;
> +
> +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    needs_modeset(new_crtc_state))
> +			continue;
> +
> +		if (intel_dp_mst_master_trans_needs_modeset(state, master)) {

I think this has the loops the opposite way of what I was thinking,
but should work fine I think... OK. I'm convinced your way is in fact
better.

> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}
> +	}
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (needs_modeset(new_crtc_state))
> +			continue;

I suppose there isn't any way we should have crtcs in the state that
neither have update_pipe or needs_modeset flagged here. Could maybe
WARN_ON(!update_pipe) here if we're being paranoid.

> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;

Still a bit unhappy having this state copy inlined in intel_atomic_check().

> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index efd14b0b507b..4aba1d702a83 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
>  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
>  }
> +
> +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> +					     enum transcoder master)

Are we going to need this elsewhere? Or be static in intel_display.c?
Not that people are 100% happy of stuffing everything in there.

> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> +		if (new_crtc_state->mst_master_transcoder == master)

So we need to modeset everything when either the master or any
other slave wants to modeset? That is, we can't just modeset slaves
independently and only force the modesets for all slaves if the master
needs a modeset?

> +			return drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> +
> +	return false;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index 854724f68f09..72cb486f32ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/types.h>
>  
> +struct intel_atomic_state;
>  struct intel_digital_port;
>  struct intel_crtc_state;
>  
> @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
>  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
>  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
> +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> +					     enum transcoder master);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/tgl: Select master transcoder for MST stream

[Souza, Jose]

On Wed, 2019-12-18 at 21:24 +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 10:59:06AM -0800, José Roberto de Souza
> wrote:
> > On TGL the blending of all the streams have moved from DDI to
> > transcoder, so now every transcoder working over the same MST port
> > must
> > send its stream to a master transcoder and master will send to DDI
> > respecting the time slots.
> > 
> > So here adding all the CRTCs that shares the same MST stream if
> > needed and computing their state again, it will pick the lowest
> > pipe/transcoder among the ones in the same stream to be master.
> > 
> > Most of the time skl_commit_modeset_enables() enables pipes in a
> > crescent order but due DDB overlapping it might not happen, this
> > scenarios will be handled in the next patch.
> > 
> > v2:
> > - Using recently added intel_crtc_state_reset() to set
> > mst_master_transcoder to invalid transcoder for all non gen12 & MST
> > code paths
> > - Setting lowest pipe/transcoder as master, previously it was the
> > first one but setting a predictable one will help in future MST e
> > port sync integration
> > - Moving to intel type as much as we can
> > 
> > v3:
> > - Now intel_dp_mst_master_trans_compute() returns the MST master
> > transcoder
> > - Replaced stdbool.h by linux/types.h
> > - Skip the connector being checked in
> > intel_dp_mst_atomic_master_trans_check()
> > - Using pipe instead of transcoder to compute MST master
> > 
> > v4:
> > - renamed connector_state to conn_state
> > 
> > BSpec: 50493
> > BSpec: 49190
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> >  drivers/gpu/drm/i915/display/intel_atomic.h   |   4 +
> >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> >  .../drm/i915/display/intel_display_types.h    |   3 +
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 143
> > ++++++++++++++++--
> >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> >  7 files changed, 183 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index b7dda18b6f29..0eb973f65977 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct
> > intel_atomic_state *state,
> >  									
> >     new_conn_state->crtc)));
> >  }
> >  
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > *state,
> > +					 struct intel_connector
> > *connector)
> > +{
> > +	struct drm_connector_state *conn_state;
> > +
> > +	conn_state = drm_atomic_get_connector_state(&state->base,
> > +						    &connector->base);
> > +	if (IS_ERR(conn_state))
> > +		return ERR_CAST(conn_state);
> > +
> > +	return to_intel_digital_connector_state(conn_state);
> > +}
> > +
> >  /**
> >   * intel_crtc_duplicate_state - duplicate crtc state
> >   * @crtc: drm crtc
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > index a7d1a8576c48..74c749dbfb4f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -17,6 +17,7 @@ struct drm_device;
> >  struct drm_i915_private;
> >  struct drm_property;
> >  struct intel_atomic_state;
> > +struct intel_connector;
> >  struct intel_crtc;
> >  struct intel_crtc_state;
> >  
> > @@ -34,6 +35,9 @@ struct drm_connector_state *
> >  intel_digital_connector_duplicate_state(struct drm_connector
> > *connector);
> >  bool intel_connector_needs_modeset(struct intel_atomic_state
> > *state,
> >  				   struct drm_connector *connector);
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > *state,
> > +					 struct intel_connector
> > *connector);
> >  
> >  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc
> > *crtc);
> >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index fa40ba7cbcad..9d99ec82d072 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1899,8 +1899,13 @@ intel_ddi_transcoder_func_reg_val_get(const
> > struct intel_crtc_state *crtc_state)
> >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> >  
> > -		if (INTEL_GEN(dev_priv) >= 12)
> > -			temp |=
> > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > +		if (INTEL_GEN(dev_priv) >= 12) {
> > +			enum transcoder master;
> > +
> > +			master = crtc_state->mst_master_transcoder;
> > +			WARN_ON(master == INVALID_TRANSCODER);
> > +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > +		}
> >  	} else {
> >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > @@ -4400,6 +4405,11 @@ void intel_ddi_get_config(struct
> > intel_encoder *encoder,
> >  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> >  		pipe_config->lane_count =
> >  			((temp & DDI_PORT_WIDTH_MASK) >>
> > DDI_PORT_WIDTH_SHIFT) + 1;
> > +
> > +		if (INTEL_GEN(dev_priv) >= 12)
> > +			pipe_config->mst_master_transcoder =
> > +					REG_FIELD_GET(TRANS_DDI_MST_TRA
> > NSPORT_SELECT_MASK, temp);
> > +
> >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> >  		break;
> >  	default:
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 25bf084427bf..59b3bfe8b721 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -46,6 +46,7 @@
> >  #include "display/intel_crt.h"
> >  #include "display/intel_ddi.h"
> >  #include "display/intel_dp.h"
> > +#include "display/intel_dp_mst.h"
> >  #include "display/intel_dsi.h"
> >  #include "display/intel_dvo.h"
> >  #include "display/intel_gmbus.h"
> > @@ -11630,6 +11631,7 @@ static void intel_crtc_state_reset(struct
> > intel_crtc_state *crtc_state,
> >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> >  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> >  	crtc_state->scaler_state.scaler_id = -1;
> > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> >  }
> >  
> >  /* Returns the currently programmed mode of the given encoder. */
> > @@ -12477,6 +12479,9 @@ static void intel_dump_pipe_config(const
> > struct intel_crtc_state *pipe_config,
> >  			      pipe_config->csc_mode, pipe_config-
> > >gamma_mode,
> >  			      pipe_config->gamma_enable, pipe_config-
> > >csc_enable);
> >  
> > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > +		      transcoder_name(pipe_config-
> > >mst_master_transcoder));
> > +
> >  dump_planes:
> >  	if (!state)
> >  		return;
> > @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct
> > intel_crtc_state *crtc_state)
> >  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> >  	       sizeof(saved_state->icl_port_dplls));
> >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
> 
> That's redundant now?

No, we don't call intel_crtc_state_reset() when computing state.
Did not changed that because we are copying a bunch of stuff from the
saved_state.

> 
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		saved_state->wm = crtc_state->wm;
> > @@ -13257,6 +13263,8 @@ intel_pipe_config_compare(const struct
> > intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> >  
> > +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_BOOL
> > @@ -14341,7 +14349,7 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  	u32 handled = 0;
> >  	int i;
> >  
> > -	/* Only disable port sync slaves */
> > +	/* Only disable port sync and MST slaves */
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state))
> > @@ -14355,7 +14363,8 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  		 * slave CRTCs are disabled first and then master CRTC
> > since
> >  		 * Slave vblanks are masked till Master Vblanks.
> >  		 */
> > -		if (!is_trans_port_sync_slave(old_crtc_state))
> > +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> > +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
> >  			continue;
> >  
> >  		intel_pre_plane_update(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 83ea04149b77..630a94892b7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> >  
> >  	/* Bitmask to indicate slaves attached */
> >  	u8 sync_mode_slaves_mask;
> > +
> > +	/* Only valid on TGL+ */
> > +	enum transcoder mst_master_transcoder;
> >  };
> >  
> >  struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 7aa0975c33b7..710137984c71 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -87,6 +87,54 @@ static int
> > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Iterate over all connectors and set mst_master_transcoder to
> > the smallest
> > + * transcoder id that shares the same MST connector.
> > + */
> > +static enum transcoder
> > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> > +				  struct intel_crtc_state *crtc_state,
> > +				  struct drm_connector_state
> > *connector_state)
> > +{
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(crtc_state->uapi.state);
> 
> Could grab that from the connector state and not pass in crtc_state
> at
> all?
> 
> Hmm. Might be even better to just pass inin
> intel_atomic_state+mst_port
> so we wouldn't have the clash with the iterators and could use the
> normal names for those. Shrug.

Okay

> 
> 
> > +	struct intel_connector *connector =
> > to_intel_connector(connector_state->connector);
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_connector_state *iter_connector_state;
> 
> Could sprinkle some consts on the crtc/connector states now that
> we're pure.
> 
> > +	struct intel_connector *iter_connector;
> > +	enum pipe ret = I915_MAX_PIPES;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return INVALID_TRANSCODER;
> > +
> > +	for_each_new_intel_connector_in_state(state, iter_connector,
> > +					      iter_connector_state, i)
> > {
> > +		struct intel_crtc_state *iter_crtc_state;
> > +		struct intel_crtc *iter_crtc;
> > +
> > +		if (connector->mst_port != iter_connector->mst_port ||
> > +		    !iter_connector_state->base.crtc)
> > +			continue;
> > +
> > +		iter_crtc = to_intel_crtc(iter_connector_state-
> > >base.crtc);
> > +		iter_crtc_state =
> > intel_atomic_get_new_crtc_state(state,
> > +								  iter_
> > crtc);
> > +		if (!iter_crtc_state->uapi.active)
> > +			continue;
> > +
> > +		/*
> > +		 * Using crtc->pipe because crtc_state->cpu_transcoder
> > is
> > +		 * computed, so others pipe cpu_transcoder could have
> > being
> > +		 * computed yet
> > +		 */
> > +		if (iter_crtc->pipe < ret)
> > +			ret = iter_crtc->pipe;
> > +	}
> 
> What if we have no active pipes? Aren't we going to come here with
> ret==MAX_PIPES?
> 
> I guess we can't really change the !active check to !enabled
> because we surely can't use an inactive transcoder as the 
> master. So !active does seem like the right thing to check.
> 
> > +
> > +	/* Simple cast works because TGL don't have a eDP transcoder */
> > +	return (enum transcoder)ret;
> 
> I'd have probably extracted the thing to compute the transcoder.
> But doesn't matter for MST since it can't be going over the EDP
> transcoder. Port sync will be a different story. We can cross that
> bridge when we come to it.
> 
> > +}
> > +
> >  static int intel_dp_mst_compute_config(struct intel_encoder
> > *encoder,
> >  				       struct intel_crtc_state
> > *pipe_config,
> >  				       struct drm_connector_state
> > *conn_state)
> > @@ -154,24 +202,89 @@ static int intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >  
> >  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> >  
> > +	pipe_config->mst_master_transcoder =
> > intel_dp_mst_master_trans_compute(encoder,
> > +									
> >        pipe_config,
> > +									
> >        conn_state);
> > +	if (ret)
> > +		return ret;
> 
> ret?

My bad, it is left over from the change to return the master
transcoder.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * If one of the connectors in a MST stream needs a modeset, mark
> > all CRTCs
> > + * that have a connector in the same MST stream as mode changed,
> > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will
> > take to do a
> > + * fastset when possible.
> > + */
> > +static int
> > +intel_dp_mst_atomic_master_trans_check(struct intel_connector
> > *connector,
> > +				       struct intel_atomic_state
> > *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector_list_iter connector_list_iter;
> > +	struct intel_connector *connector_iter;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return  0;
> > +
> > +	if (!intel_connector_needs_modeset(state, &connector->base))
> > +		return 0;
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm,
> > &connector_list_iter);
> > +	for_each_intel_connector_iter(connector_iter,
> > &connector_list_iter) {
> > +		struct intel_digital_connector_state *conn_iter_state;
> > +		struct intel_crtc_state *crtc_state;
> > +		struct intel_crtc *crtc;
> > +
> > +		if (connector_iter->mst_port != connector->mst_port ||
> > +		    connector_iter == connector)
> > +			continue;
> > +
> > +		conn_iter_state =
> > intel_atomic_get_digital_connector_state(state,
> > +									
> >    connector_iter);
> > +		if (IS_ERR(conn_iter_state)) {
> > +			drm_connector_list_iter_end(&connector_list_ite
> > r);
> > +			return PTR_ERR(conn_iter_state);
> > +		}
> > +
> > +		if (!conn_iter_state->base.crtc)
> > +			continue;
> > +
> > +		crtc = to_intel_crtc(conn_iter_state->base.crtc);
> > +		crtc_state = intel_atomic_get_crtc_state(&state->base,
> > crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			drm_connector_list_iter_end(&connector_list_ite
> > r);
> > +			return PTR_ERR(crtc_state);
> > +		}
> > +
> > +		crtc_state->uapi.mode_changed = true;
> 
> We might need drm_atomic_add_affected_planes() here. The helper won't
> do
> that for us if the connector->atomic_check() was called from the
> second
> loop.

Done

> 
> The rest lgtm.
> 
> > +	}
> > +	drm_connector_list_iter_end(&connector_list_iter);
> > +
> >  	return 0;
> >  }
> >  
> >  static int
> >  intel_dp_mst_atomic_check(struct drm_connector *connector,
> > -			  struct drm_atomic_state *state)
> > +			  struct drm_atomic_state *_state)
> >  {
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(_state);
> >  	struct drm_connector_state *new_conn_state =
> > -		drm_atomic_get_new_connector_state(state, connector);
> > +		drm_atomic_get_new_connector_state(&state->base,
> > connector);
> >  	struct drm_connector_state *old_conn_state =
> > -		drm_atomic_get_old_connector_state(state, connector);
> > +		drm_atomic_get_old_connector_state(&state->base,
> > connector);
> >  	struct intel_connector *intel_connector =
> >  		to_intel_connector(connector);
> >  	struct drm_crtc *new_crtc = new_conn_state->crtc;
> >  	struct drm_dp_mst_topology_mgr *mgr;
> >  	int ret;
> >  
> > -	ret = intel_digital_connector_atomic_check(connector, state);
> > +	ret = intel_digital_connector_atomic_check(connector, &state-
> > >base);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector,
> > state);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -182,12 +295,9 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> >  	 * connector
> >  	 */
> >  	if (new_crtc) {
> > -		struct intel_atomic_state *intel_state =
> > -			to_intel_atomic_state(state);
> >  		struct intel_crtc *intel_crtc =
> > to_intel_crtc(new_crtc);
> >  		struct intel_crtc_state *crtc_state =
> > -			intel_atomic_get_new_crtc_state(intel_state,
> > -							intel_crtc);
> > +			intel_atomic_get_new_crtc_state(state,
> > intel_crtc);
> >  
> >  		if (!crtc_state ||
> >  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi)
> > ||
> > @@ -196,7 +306,7 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> >  	}
> >  
> >  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary-
> > >dp.mst_mgr;
> > -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
> >  					       intel_connector->port);
> >  
> >  	return ret;
> > @@ -240,6 +350,8 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  
> >  	intel_dp->active_mst_links--;
> >  	last_mst_stream = intel_dp->active_mst_links == 0;
> > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> > +		!intel_dp_mst_is_master_trans(old_crtc_state));
> >  
> >  	intel_crtc_vblank_off(old_crtc_state);
> >  
> > @@ -317,6 +429,8 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  	connector->encoder = encoder;
> >  	intel_mst->connector = connector;
> >  	first_mst_stream = intel_dp->active_mst_links == 0;
> > +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> > +		!intel_dp_mst_is_master_trans(pipe_config));
> >  
> >  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >  
> > @@ -722,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct
> > intel_digital_port *intel_dig_port)
> >  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
> >  	/* encoders will get killed by normal cleanup */
> >  }
> > +
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return crtc_state->mst_master_transcoder == crtc_state-
> > >cpu_transcoder;
> > +}
> > +
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > &&
> > +	       crtc_state->mst_master_transcoder != crtc_state-
> > >cpu_transcoder;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index f660ad80db04..854724f68f09 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -6,10 +6,15 @@
> >  #ifndef __INTEL_DP_MST_H__
> >  #define __INTEL_DP_MST_H__
> >  
> > +#include <linux/types.h>
> > +
> >  struct intel_digital_port;
> > +struct intel_crtc_state;
> >  
> >  int intel_dp_mst_encoder_init(struct intel_digital_port
> > *intel_dig_port, int conn_id);
> >  void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > *intel_dig_port);
> >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > *intel_dig_port);
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state);
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state);
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > -- 
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 4/6] drm/i915/dp: Fix MST disable sequences

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 10:59:08AM -0800, José Roberto de Souza wrote:
> The disable sequence after wait for transcoder off was not correctly
> implemented.
> The MST disable sequence is basically the same for HSW, SKL, ICL and
> TGL, with just minor changes for TGL.
> 
> With this last patch we finally fixed the hotplugs triggered by MST
> sinks during the disable/enable sequence, those were causing source
> to try to do a link training while it was not ready causing CPU pipe
> FIFO underrrus on TGL.
> 
> v2: Only unsetting TGL_TRANS_DDI_PORT_MASK for TGL on the post
> disable sequence
> 
> v4: Rebased, moved MST sequences to intel_mst_post_disable_dp()
> 
> BSpec: 4231
> BSpec: 4163
> BSpec: 22243
> BSpec: 49190
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c    | 33 +++++++++++++++------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 33 +++++++++++++--------
>  2 files changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 9d99ec82d072..94ca26be2fee 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -34,6 +34,7 @@
>  #include "intel_ddi.h"
>  #include "intel_display_types.h"
>  #include "intel_dp.h"
> +#include "intel_dp_mst.h"
>  #include "intel_dp_link_training.h"
>  #include "intel_dpio_phy.h"
>  #include "intel_dsi.h"
> @@ -1949,17 +1950,19 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
> -	u32 val = I915_READ(reg);
> +	u32 val;
> +
> +	val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> +	val &= ~TRANS_DDI_FUNC_ENABLE;
>  
>  	if (INTEL_GEN(dev_priv) >= 12) {
> -		val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK |
> -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> +		if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) ||
> +		    intel_dp_mst_is_slave_trans(crtc_state))

if (!intel_dp_mst_is_master_trans())
	val &= ...;

?

> +			val &= ~TGL_TRANS_DDI_PORT_MASK;
>  	} else {
> -		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
> -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> +		val &= ~TRANS_DDI_PORT_MASK;
>  	}
> -	I915_WRITE(reg, val);
> +	I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
>  
>  	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
>  	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> @@ -3808,8 +3811,20 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
>  	 */
>  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>  
> -	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
> -		intel_ddi_disable_pipe_clock(old_crtc_state);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		if (is_mst) {

if (intel_dp_mst_is_master_trans()) {

?

> +			enum transcoder cpu_transcoder;
> +			u32 val;
> +
> +			cpu_transcoder = old_crtc_state->cpu_transcoder;

Assignment can be done when declaring the variable.

> +			val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> +			val &= ~TGL_TRANS_DDI_PORT_MASK;
> +			I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
> +		}
> +	} else {
> +		if (!is_mst)
> +			intel_ddi_disable_pipe_clock(old_crtc_state);
> +	}
>  
>  	intel_disable_ddi_buf(encoder, old_crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 710137984c71..efd14b0b507b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -347,6 +347,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  		to_intel_connector(old_conn_state->connector);
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	bool last_mst_stream;
> +	u32 val;
>  
>  	intel_dp->active_mst_links--;
>  	last_mst_stream = intel_dp->active_mst_links == 0;
> @@ -357,6 +358,19 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  
>  	intel_disable_pipe(old_crtc_state);
>  
> +	drm_dp_update_payload_part2(&intel_dp->mst_mgr);

Hmm. I'm having hard to figuring out what these things do. But from a
cursory glance it almost looks like part1 is the one that does the AUX
stuff to deallocate stuff. So feels like even that part should be here.

> +
> +	val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder));
> +	val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC;
> +	I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), val);
> +
> +	if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status,
> +				  DP_TP_STATUS_ACT_SENT, 1))
> +		DRM_ERROR("Timed out waiting for ACT sent when disabling\n");

I guess we were missing this step entirely. Dunno if we want ERROR for
this. Not sure how much noise it'll generate when someone forcefully
yanks the cable...

> +	drm_dp_check_act_status(&intel_dp->mst_mgr);
> +
> +	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);

This seems to be pure sw stuff so not so critical where it is. Keeping
it next to the hw payload deallocation seems like a good approach.

> +
>  	intel_ddi_disable_transcoder_func(old_crtc_state);
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
> @@ -364,6 +378,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  	else
>  		ironlake_pfit_disable(old_crtc_state);
>  
> +	/*
> +	 * Power down mst path before disabling the port, otherwise we end
> +	 * up getting interrupts from the sink upon detecting link loss.
> +	 */
> +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> +				     false);

And I guess this was needed to avoid the link loss screams from the sink?

Seems to match the spec better than before at least. Still a bit unsure
about he part1/2 stuff for the deallocation. But if that should be
changed we can do it as a followup.

The code movement into .post_disable() really did make for a much
nicer experience when comparing with the spec. Or at least I didn't
lose as much hair as with the previous version :)

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

>  	/*
>  	 * From TGL spec: "If multi-stream slave transcoder: Configure
>  	 * Transcoder Clock Select to direct no clock to the transcoder"
> @@ -374,19 +394,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  	if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream)
>  		intel_ddi_disable_pipe_clock(old_crtc_state);
>  
> -	/* this can fail */
> -	drm_dp_check_act_status(&intel_dp->mst_mgr);
> -	/* and this can also fail */
> -	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> -
> -	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);
> -
> -	/*
> -	 * Power down mst path before disabling the port, otherwise we end
> -	 * up getting interrupts from the sink upon detecting link loss.
> -	 */
> -	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> -				     false);
>  
>  	intel_mst->connector = NULL;
>  	if (last_mst_stream)
> -- 
> 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 09:39:17PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza wrote:
> > Check if fastset is allowed by external dependencies like other pipes
> > and transcoders.
> > 
> > Right now this patch only forces a fullmodeset in MST slaves of MST
> > masters that needs a fullmodeset but it will be needed for port sync
> > as well.
> > 
> > v3:
> > - moved handling to intel_atomic_check() this way is guarantee that
> > all pipes will have its state computed
> > 
> > v4:
> > - added a function to return if MST master neeeds modeset to simply
> > code in intel_atomic_check()
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-----
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> >  3 files changed, 57 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a4f252d05b37..2a406891567b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> >  
> >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct drm_device *dev,
> >  			any_ms = true;
> >  	}
> >  
> > +	/**
> > +	 * Check if fastset is allowed by external dependencies like other
> > +	 * pipes and transcoders.
> > +	 *
> > +	 * Right now it only forces a fullmodeset when the MST master
> > +	 * transcoder did not changed but the pipe of the master transcoder
> > +	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
> > +	 */
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +		enum transcoder master = new_crtc_state->mst_master_transcoder;
> > +
> > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > +		    needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		if (intel_dp_mst_master_trans_needs_modeset(state, master)) {
> 
> I think this has the loops the opposite way of what I was thinking,
> but should work fine I think... OK. I'm convinced your way is in fact
> better.
> 
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> > +	}
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (needs_modeset(new_crtc_state))
> > +			continue;
> 
> I suppose there isn't any way we should have crtcs in the state that
> neither have update_pipe or needs_modeset flagged here. Could maybe
> WARN_ON(!update_pipe) here if we're being paranoid.
> 
> > +
> > +		/*
> > +		 * If we're not doing the full modeset we want to
> > +		 * keep the current M/N values as they may be
> > +		 * sufficiently different to the computed values
> > +		 * to cause problems.
> > +		 *
> > +		 * FIXME: should really copy more fuzzy state here
> > +		 */
> > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> 
> Still a bit unhappy having this state copy inlined in intel_atomic_check().
> 
> > +	}
> > +
> >  	if (any_ms && !check_digital_port_conflicts(state)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> >  		ret = EINVAL;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index efd14b0b507b..4aba1d702a83 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
> >  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
> >  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
> >  }
> > +
> > +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> > +					     enum transcoder master)
> 
> Are we going to need this elsewhere? Or be static in intel_display.c?
> Not that people are 100% happy of stuffing everything in there.
> 
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> > +		if (new_crtc_state->mst_master_transcoder == master)
> 
> So we need to modeset everything when either the master or any
> other slave wants to modeset? That is, we can't just modeset slaves
> independently and only force the modesets for all slaves if the master
> needs a modeset?

Hmm. Even if we technically could not sure we should with fastset
potentially making minced meat of the state. Probably best to at
least start with your version here.

I'll leave it to you whether to address any nits I had:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > +			return drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > +
> > +	return false;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index 854724f68f09..72cb486f32ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -8,6 +8,7 @@
> >  
> >  #include <linux/types.h>
> >  
> > +struct intel_atomic_state;
> >  struct intel_digital_port;
> >  struct intel_crtc_state;
> >  
> > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
> >  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
> >  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
> >  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
> > +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> > +					     enum transcoder master);
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > -- 
> > 2.24.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/tgl: Select master transcoder for MST stream

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 08:06:34PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 21:24 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 18, 2019 at 10:59:06AM -0800, José Roberto de Souza
> > wrote:
> > > On TGL the blending of all the streams have moved from DDI to
> > > transcoder, so now every transcoder working over the same MST port
> > > must
> > > send its stream to a master transcoder and master will send to DDI
> > > respecting the time slots.
> > > 
> > > So here adding all the CRTCs that shares the same MST stream if
> > > needed and computing their state again, it will pick the lowest
> > > pipe/transcoder among the ones in the same stream to be master.
> > > 
> > > Most of the time skl_commit_modeset_enables() enables pipes in a
> > > crescent order but due DDB overlapping it might not happen, this
> > > scenarios will be handled in the next patch.
> > > 
> > > v2:
> > > - Using recently added intel_crtc_state_reset() to set
> > > mst_master_transcoder to invalid transcoder for all non gen12 & MST
> > > code paths
> > > - Setting lowest pipe/transcoder as master, previously it was the
> > > first one but setting a predictable one will help in future MST e
> > > port sync integration
> > > - Moving to intel type as much as we can
> > > 
> > > v3:
> > > - Now intel_dp_mst_master_trans_compute() returns the MST master
> > > transcoder
> > > - Replaced stdbool.h by linux/types.h
> > > - Skip the connector being checked in
> > > intel_dp_mst_atomic_master_trans_check()
> > > - Using pipe instead of transcoder to compute MST master
> > > 
> > > v4:
> > > - renamed connector_state to conn_state
> > > 
> > > BSpec: 50493
> > > BSpec: 49190
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
> > >  drivers/gpu/drm/i915/display/intel_atomic.h   |   4 +
> > >  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  13 +-
> > >  .../drm/i915/display/intel_display_types.h    |   3 +
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 143
> > > ++++++++++++++++--
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
> > >  7 files changed, 183 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > index b7dda18b6f29..0eb973f65977 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > @@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct
> > > intel_atomic_state *state,
> > >  									
> > >     new_conn_state->crtc)));
> > >  }
> > >  
> > > +struct intel_digital_connector_state *
> > > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > > *state,
> > > +					 struct intel_connector
> > > *connector)
> > > +{
> > > +	struct drm_connector_state *conn_state;
> > > +
> > > +	conn_state = drm_atomic_get_connector_state(&state->base,
> > > +						    &connector->base);
> > > +	if (IS_ERR(conn_state))
> > > +		return ERR_CAST(conn_state);
> > > +
> > > +	return to_intel_digital_connector_state(conn_state);
> > > +}
> > > +
> > >  /**
> > >   * intel_crtc_duplicate_state - duplicate crtc state
> > >   * @crtc: drm crtc
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > index a7d1a8576c48..74c749dbfb4f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > > @@ -17,6 +17,7 @@ struct drm_device;
> > >  struct drm_i915_private;
> > >  struct drm_property;
> > >  struct intel_atomic_state;
> > > +struct intel_connector;
> > >  struct intel_crtc;
> > >  struct intel_crtc_state;
> > >  
> > > @@ -34,6 +35,9 @@ struct drm_connector_state *
> > >  intel_digital_connector_duplicate_state(struct drm_connector
> > > *connector);
> > >  bool intel_connector_needs_modeset(struct intel_atomic_state
> > > *state,
> > >  				   struct drm_connector *connector);
> > > +struct intel_digital_connector_state *
> > > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > > *state,
> > > +					 struct intel_connector
> > > *connector);
> > >  
> > >  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc
> > > *crtc);
> > >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index fa40ba7cbcad..9d99ec82d072 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -1899,8 +1899,13 @@ intel_ddi_transcoder_func_reg_val_get(const
> > > struct intel_crtc_state *crtc_state)
> > >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > >  
> > > -		if (INTEL_GEN(dev_priv) >= 12)
> > > -			temp |=
> > > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > +			enum transcoder master;
> > > +
> > > +			master = crtc_state->mst_master_transcoder;
> > > +			WARN_ON(master == INVALID_TRANSCODER);
> > > +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > > +		}
> > >  	} else {
> > >  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > @@ -4400,6 +4405,11 @@ void intel_ddi_get_config(struct
> > > intel_encoder *encoder,
> > >  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> > >  		pipe_config->lane_count =
> > >  			((temp & DDI_PORT_WIDTH_MASK) >>
> > > DDI_PORT_WIDTH_SHIFT) + 1;
> > > +
> > > +		if (INTEL_GEN(dev_priv) >= 12)
> > > +			pipe_config->mst_master_transcoder =
> > > +					REG_FIELD_GET(TRANS_DDI_MST_TRA
> > > NSPORT_SELECT_MASK, temp);
> > > +
> > >  		intel_dp_get_m_n(intel_crtc, pipe_config);
> > >  		break;
> > >  	default:
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 25bf084427bf..59b3bfe8b721 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -46,6 +46,7 @@
> > >  #include "display/intel_crt.h"
> > >  #include "display/intel_ddi.h"
> > >  #include "display/intel_dp.h"
> > > +#include "display/intel_dp_mst.h"
> > >  #include "display/intel_dsi.h"
> > >  #include "display/intel_dvo.h"
> > >  #include "display/intel_gmbus.h"
> > > @@ -11630,6 +11631,7 @@ static void intel_crtc_state_reset(struct
> > > intel_crtc_state *crtc_state,
> > >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> > >  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> > >  	crtc_state->scaler_state.scaler_id = -1;
> > > +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> > >  }
> > >  
> > >  /* Returns the currently programmed mode of the given encoder. */
> > > @@ -12477,6 +12479,9 @@ static void intel_dump_pipe_config(const
> > > struct intel_crtc_state *pipe_config,
> > >  			      pipe_config->csc_mode, pipe_config-
> > > >gamma_mode,
> > >  			      pipe_config->gamma_enable, pipe_config-
> > > >csc_enable);
> > >  
> > > +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > > +		      transcoder_name(pipe_config-
> > > >mst_master_transcoder));
> > > +
> > >  dump_planes:
> > >  	if (!state)
> > >  		return;
> > > @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct
> > > intel_crtc_state *crtc_state)
> > >  	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> > >  	       sizeof(saved_state->icl_port_dplls));
> > >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> > > +	saved_state->mst_master_transcoder = INVALID_TRANSCODER;
> > 
> > That's redundant now?
> 
> No, we don't call intel_crtc_state_reset() when computing state.
> Did not changed that because we are copying a bunch of stuff from the
> saved_state.

Doh. I think we should be able just call the reset thing in saved_state
here?

Oh, and we seem to have another one in vlv_force_pll_on(). Maybe we
should even add a intel_crtc_state_alloc() or something which would
give us a properly initialized fresh state.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Souza, Jose]

On Wed, 2019-12-18 at 21:39 +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> wrote:
> > Check if fastset is allowed by external dependencies like other
> > pipes
> > and transcoders.
> > 
> > Right now this patch only forces a fullmodeset in MST slaves of MST
> > masters that needs a fullmodeset but it will be needed for port
> > sync
> > as well.
> > 
> > v3:
> > - moved handling to intel_atomic_check() this way is guarantee that
> > all pipes will have its state computed
> > 
> > v4:
> > - added a function to return if MST master neeeds modeset to simply
> > code in intel_atomic_check()
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-
> > ----
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> >  3 files changed, 57 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a4f252d05b37..2a406891567b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const
> > struct intel_crtc_state *old_crtc_sta
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> >  
> >  static int intel_crtc_add_planes_to_state(struct
> > intel_atomic_state *state,
> > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  			any_ms = true;
> >  	}
> >  
> > +	/**
> > +	 * Check if fastset is allowed by external dependencies like
> > other
> > +	 * pipes and transcoders.
> > +	 *
> > +	 * Right now it only forces a fullmodeset when the MST master
> > +	 * transcoder did not changed but the pipe of the master
> > transcoder
> > +	 * needs a fullmodeset so all slaves also needs to do a
> > fullmodeset.
> > +	 */
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i) {
> > +		enum transcoder master = new_crtc_state-
> > >mst_master_transcoder;
> > +
> > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > +		    needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		if (intel_dp_mst_master_trans_needs_modeset(state,
> > master)) {
> 
> I think this has the loops the opposite way of what I was thinking,
> but should work fine I think... OK. I'm convinced your way is in fact
> better.
> 
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> > +	}
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (needs_modeset(new_crtc_state))
> > +			continue;
> 
> I suppose there isn't any way we should have crtcs in the state that
> neither have update_pipe or needs_modeset flagged here. Could maybe
> WARN_ON(!update_pipe) here if we're being paranoid.

Adding the WARN_ON

> 
> > +
> > +		/*
> > +		 * If we're not doing the full modeset we want to
> > +		 * keep the current M/N values as they may be
> > +		 * sufficiently different to the computed values
> > +		 * to cause problems.
> > +		 *
> > +		 * FIXME: should really copy more fuzzy state here
> > +		 */
> > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> 
> Still a bit unhappy having this state copy inlined in
> intel_atomic_check().
> 
> > +	}
> > +
> >  	if (any_ms && !check_digital_port_conflicts(state)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port
> > configuration\n");
> >  		ret = EINVAL;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index efd14b0b507b..4aba1d702a83 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct
> > intel_crtc_state *crtc_state)
> >  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > &&
> >  	       crtc_state->mst_master_transcoder != crtc_state-
> > >cpu_transcoder;
> >  }
> > +
> > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > intel_atomic_state *state,
> > +					     enum transcoder master)
> 
> Are we going to need this elsewhere? Or be static in intel_display.c?
> Not that people are 100% happy of stuffing everything in there.

Not needed elsewhere, I'm also trying to not put more stuff in
intel_display.c.

> 
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i)
> > +		if (new_crtc_state->mst_master_transcoder == master)
> 
> So we need to modeset everything when either the master or any
> other slave wants to modeset? That is, we can't just modeset slaves
> independently and only force the modesets for all slaves if the
> master
> needs a modeset?

If a slave needs a modeset we don't need to do a modeset in master or
in other slaves.

This is doing independent modeset in slaves and forcing modeset to all
slaves when master needs as modeset.

> 
> > +			return
> > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > +
> > +	return false;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index 854724f68f09..72cb486f32ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -8,6 +8,7 @@
> >  
> >  #include <linux/types.h>
> >  
> > +struct intel_atomic_state;
> >  struct intel_digital_port;
> >  struct intel_crtc_state;
> >  
> > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct
> > intel_digital_port *intel_dig_port);
> >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > *intel_dig_port);
> >  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state);
> >  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state);
> > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > intel_atomic_state *state,
> > +					     enum transcoder master);
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > -- 
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 08:19:15PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 21:39 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> > wrote:
> > > Check if fastset is allowed by external dependencies like other
> > > pipes
> > > and transcoders.
> > > 
> > > Right now this patch only forces a fullmodeset in MST slaves of MST
> > > masters that needs a fullmodeset but it will be needed for port
> > > sync
> > > as well.
> > > 
> > > v3:
> > > - moved handling to intel_atomic_check() this way is guarantee that
> > > all pipes will have its state computed
> > > 
> > > v4:
> > > - added a function to return if MST master neeeds modeset to simply
> > > code in intel_atomic_check()
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-
> > > ----
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> > >  3 files changed, 57 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index a4f252d05b37..2a406891567b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const
> > > struct intel_crtc_state *old_crtc_sta
> > >  
> > >  	new_crtc_state->uapi.mode_changed = false;
> > >  	new_crtc_state->update_pipe = true;
> > > -
> > > -	/*
> > > -	 * If we're not doing the full modeset we want to
> > > -	 * keep the current M/N values as they may be
> > > -	 * sufficiently different to the computed values
> > > -	 * to cause problems.
> > > -	 *
> > > -	 * FIXME: should really copy more fuzzy state here
> > > -	 */
> > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > >  }
> > >  
> > >  static int intel_crtc_add_planes_to_state(struct
> > > intel_atomic_state *state,
> > > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  			any_ms = true;
> > >  	}
> > >  
> > > +	/**
> > > +	 * Check if fastset is allowed by external dependencies like
> > > other
> > > +	 * pipes and transcoders.
> > > +	 *
> > > +	 * Right now it only forces a fullmodeset when the MST master
> > > +	 * transcoder did not changed but the pipe of the master
> > > transcoder
> > > +	 * needs a fullmodeset so all slaves also needs to do a
> > > fullmodeset.
> > > +	 */
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i) {
> > > +		enum transcoder master = new_crtc_state-
> > > >mst_master_transcoder;
> > > +
> > > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > +		    needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		if (intel_dp_mst_master_trans_needs_modeset(state,
> > > master)) {
> > 
> > I think this has the loops the opposite way of what I was thinking,
> > but should work fine I think... OK. I'm convinced your way is in fact
> > better.
> > 
> > > +			new_crtc_state->uapi.mode_changed = true;
> > > +			new_crtc_state->update_pipe = false;
> > > +		}
> > > +	}
> > > +
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		if (needs_modeset(new_crtc_state))
> > > +			continue;
> > 
> > I suppose there isn't any way we should have crtcs in the state that
> > neither have update_pipe or needs_modeset flagged here. Could maybe
> > WARN_ON(!update_pipe) here if we're being paranoid.
> 
> Adding the WARN_ON
> 
> > 
> > > +
> > > +		/*
> > > +		 * If we're not doing the full modeset we want to
> > > +		 * keep the current M/N values as they may be
> > > +		 * sufficiently different to the computed values
> > > +		 * to cause problems.
> > > +		 *
> > > +		 * FIXME: should really copy more fuzzy state here
> > > +		 */
> > > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > 
> > Still a bit unhappy having this state copy inlined in
> > intel_atomic_check().
> > 
> > > +	}
> > > +
> > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > >  		DRM_DEBUG_KMS("rejecting conflicting digital port
> > > configuration\n");
> > >  		ret = EINVAL;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index efd14b0b507b..4aba1d702a83 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct
> > > intel_crtc_state *crtc_state)
> > >  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > > &&
> > >  	       crtc_state->mst_master_transcoder != crtc_state-
> > > >cpu_transcoder;
> > >  }
> > > +
> > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > intel_atomic_state *state,
> > > +					     enum transcoder master)
> > 
> > Are we going to need this elsewhere? Or be static in intel_display.c?
> > Not that people are 100% happy of stuffing everything in there.
> 
> Not needed elsewhere, I'm also trying to not put more stuff in
> intel_display.c.
> 
> > 
> > > +{
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i)
> > > +		if (new_crtc_state->mst_master_transcoder == master)
> > 
> > So we need to modeset everything when either the master or any
> > other slave wants to modeset? That is, we can't just modeset slaves
> > independently and only force the modesets for all slaves if the
> > master
> > needs a modeset?
> 
> If a slave needs a modeset we don't need to do a modeset in master or
> in other slaves.
> 
> This is doing independent modeset in slaves and forcing modeset to all
> slaves when master needs as modeset.

If you just wanted to check if the master needs a modeset then shouldn't
it just compare crtc_state->cpu_transcoder == master?

And in that case it's not at all MST specific and we could just call it
intel_cpu_transcoder_needs_modeset() or something.

> 
> > 
> > > +			return
> > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > > +
> > > +	return false;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > index 854724f68f09..72cb486f32ab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > @@ -8,6 +8,7 @@
> > >  
> > >  #include <linux/types.h>
> > >  
> > > +struct intel_atomic_state;
> > >  struct intel_digital_port;
> > >  struct intel_crtc_state;
> > >  
> > > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct
> > > intel_digital_port *intel_dig_port);
> > >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > > *intel_dig_port);
> > >  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > > *crtc_state);
> > >  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > *crtc_state);
> > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > intel_atomic_state *state,
> > > +					     enum transcoder master);
> > >  
> > >  #endif /* __INTEL_DP_MST_H__ */
> > > -- 
> > > 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Souza, Jose]

On Wed, 2019-12-18 at 22:26 +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 08:19:15PM +0000, Souza, Jose wrote:
> > On Wed, 2019-12-18 at 21:39 +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> > > wrote:
> > > > Check if fastset is allowed by external dependencies like other
> > > > pipes
> > > > and transcoders.
> > > > 
> > > > Right now this patch only forces a fullmodeset in MST slaves of
> > > > MST
> > > > masters that needs a fullmodeset but it will be needed for port
> > > > sync
> > > > as well.
> > > > 
> > > > v3:
> > > > - moved handling to intel_atomic_check() this way is guarantee
> > > > that
> > > > all pipes will have its state computed
> > > > 
> > > > v4:
> > > > - added a function to return if MST master neeeds modeset to
> > > > simply
> > > > code in intel_atomic_check()
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 53
> > > > +++++++++++++++-
> > > > ----
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> > > >  3 files changed, 57 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index a4f252d05b37..2a406891567b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -13903,19 +13903,6 @@ static void
> > > > intel_crtc_check_fastset(const
> > > > struct intel_crtc_state *old_crtc_sta
> > > >  
> > > >  	new_crtc_state->uapi.mode_changed = false;
> > > >  	new_crtc_state->update_pipe = true;
> > > > -
> > > > -	/*
> > > > -	 * If we're not doing the full modeset we want to
> > > > -	 * keep the current M/N values as they may be
> > > > -	 * sufficiently different to the computed values
> > > > -	 * to cause problems.
> > > > -	 *
> > > > -	 * FIXME: should really copy more fuzzy state here
> > > > -	 */
> > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > >  }
> > > >  
> > > >  static int intel_crtc_add_planes_to_state(struct
> > > > intel_atomic_state *state,
> > > > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > > > drm_device *dev,
> > > >  			any_ms = true;
> > > >  	}
> > > >  
> > > > +	/**
> > > > +	 * Check if fastset is allowed by external dependencies
> > > > like
> > > > other
> > > > +	 * pipes and transcoders.
> > > > +	 *
> > > > +	 * Right now it only forces a fullmodeset when the MST
> > > > master
> > > > +	 * transcoder did not changed but the pipe of the
> > > > master
> > > > transcoder
> > > > +	 * needs a fullmodeset so all slaves also needs to do a
> > > > fullmodeset.
> > > > +	 */
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > new_crtc_state,
> > > > i) {
> > > > +		enum transcoder master = new_crtc_state-
> > > > > mst_master_transcoder;
> > > > +
> > > > +		if
> > > > (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > > +		    needs_modeset(new_crtc_state))
> > > > +			continue;
> > > > +
> > > > +		if
> > > > (intel_dp_mst_master_trans_needs_modeset(state,
> > > > master)) {
> > > 
> > > I think this has the loops the opposite way of what I was
> > > thinking,
> > > but should work fine I think... OK. I'm convinced your way is in
> > > fact
> > > better.
> > > 
> > > > +			new_crtc_state->uapi.mode_changed =
> > > > true;
> > > > +			new_crtc_state->update_pipe = false;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > old_crtc_state,
> > > > +					    new_crtc_state, i)
> > > > {
> > > > +		if (needs_modeset(new_crtc_state))
> > > > +			continue;
> > > 
> > > I suppose there isn't any way we should have crtcs in the state
> > > that
> > > neither have update_pipe or needs_modeset flagged here. Could
> > > maybe
> > > WARN_ON(!update_pipe) here if we're being paranoid.
> > 
> > Adding the WARN_ON
> > 
> > > > +
> > > > +		/*
> > > > +		 * If we're not doing the full modeset we want
> > > > to
> > > > +		 * keep the current M/N values as they may be
> > > > +		 * sufficiently different to the computed
> > > > values
> > > > +		 * to cause problems.
> > > > +		 *
> > > > +		 * FIXME: should really copy more fuzzy state
> > > > here
> > > > +		 */
> > > > +		new_crtc_state->fdi_m_n = old_crtc_state-
> > > > >fdi_m_n;
> > > > +		new_crtc_state->dp_m_n = old_crtc_state-
> > > > >dp_m_n;
> > > > +		new_crtc_state->dp_m2_n2 = old_crtc_state-
> > > > >dp_m2_n2;
> > > > +		new_crtc_state->has_drrs = old_crtc_state-
> > > > >has_drrs;
> > > 
> > > Still a bit unhappy having this state copy inlined in
> > > intel_atomic_check().
> > > 
> > > > +	}
> > > > +
> > > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > > >  		DRM_DEBUG_KMS("rejecting conflicting digital
> > > > port
> > > > configuration\n");
> > > >  		ret = EINVAL;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > index efd14b0b507b..4aba1d702a83 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const
> > > > struct
> > > > intel_crtc_state *crtc_state)
> > > >  	return crtc_state->mst_master_transcoder !=
> > > > INVALID_TRANSCODER
> > > > &&
> > > >  	       crtc_state->mst_master_transcoder != crtc_state-
> > > > > cpu_transcoder;
> > > >  }
> > > > +
> > > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > > intel_atomic_state *state,
> > > > +					     enum transcoder
> > > > master)
> > > 
> > > Are we going to need this elsewhere? Or be static in
> > > intel_display.c?
> > > Not that people are 100% happy of stuffing everything in there.
> > 
> > Not needed elsewhere, I'm also trying to not put more stuff in
> > intel_display.c.
> > 
> > > > +{
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > new_crtc_state,
> > > > i)
> > > > +		if (new_crtc_state->mst_master_transcoder ==
> > > > master)
> > > 
> > > So we need to modeset everything when either the master or any
> > > other slave wants to modeset? That is, we can't just modeset
> > > slaves
> > > independently and only force the modesets for all slaves if the
> > > master
> > > needs a modeset?
> > 
> > If a slave needs a modeset we don't need to do a modeset in master
> > or
> > in other slaves.
> > 
> > This is doing independent modeset in slaves and forcing modeset to
> > all
> > slaves when master needs as modeset.
> 
> If you just wanted to check if the master needs a modeset then
> shouldn't
> it just compare crtc_state->cpu_transcoder == master?

Good catch, thanks. 

> 
> And in that case it's not at all MST specific and we could just call
> it
> intel_cpu_transcoder_needs_modeset() or something.

That is true, moving to intel_display then.

> 
> > > > +			return
> > > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > > > +
> > > > +	return false;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > index 854724f68f09..72cb486f32ab 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > @@ -8,6 +8,7 @@
> > > >  
> > > >  #include <linux/types.h>
> > > >  
> > > > +struct intel_atomic_state;
> > > >  struct intel_digital_port;
> > > >  struct intel_crtc_state;
> > > >  
> > > > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct
> > > > intel_digital_port *intel_dig_port);
> > > >  int intel_dp_mst_encoder_active_links(struct
> > > > intel_digital_port
> > > > *intel_dig_port);
> > > >  bool intel_dp_mst_is_master_trans(const struct
> > > > intel_crtc_state
> > > > *crtc_state);
> > > >  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > > *crtc_state);
> > > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > > intel_atomic_state *state,
> > > > +					     enum transcoder
> > > > master);
> > > >  
> > > >  #endif /* __INTEL_DP_MST_H__ */
> > > > -- 
> > > > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Manasi Navare]

On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza wrote:
> Check if fastset is allowed by external dependencies like other pipes
> and transcoders.
> 
> Right now this patch only forces a fullmodeset in MST slaves of MST
> masters that needs a fullmodeset but it will be needed for port sync
> as well.
> 
> v3:
> - moved handling to intel_atomic_check() this way is guarantee that
> all pipes will have its state computed
> 
> v4:
> - added a function to return if MST master neeeds modeset to simply
> code in intel_atomic_check()
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
>  3 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a4f252d05b37..2a406891567b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
> -
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }
>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}
>  
> +	/**
> +	 * Check if fastset is allowed by external dependencies like other
> +	 * pipes and transcoders.
> +	 *
> +	 * Right now it only forces a fullmodeset when the MST master
> +	 * transcoder did not changed but the pipe of the master transcoder
> +	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
> +	 */
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		enum transcoder master = new_crtc_state->mst_master_transcoder;
> +
> +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    needs_modeset(new_crtc_state))
> +			continue;
> +
> +		if (intel_dp_mst_master_trans_needs_modeset(state, master)) {
> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}
> +	}
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (needs_modeset(new_crtc_state))
> +			continue;
> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> +	}

Jose, Ville, this is exactly how i would need to modeset all port synced crtcs
after the fastset check?

Ville had suggsted adding mdoeset_synced_crtcs() even before the compute_config or before the call
where port sync assignments happen, so in that function, it would look at the modeset_synced_old_crtc_states ?

Manasi


Manasi

> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index efd14b0b507b..4aba1d702a83 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
>  	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
>  }
> +
> +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> +					     enum transcoder master)
> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> +		if (new_crtc_state->mst_master_transcoder == master)
> +			return drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> +
> +	return false;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index 854724f68f09..72cb486f32ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/types.h>
>  
> +struct intel_atomic_state;
>  struct intel_digital_port;
>  struct intel_crtc_state;
>  
> @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
>  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
>  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
>  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
> +bool intel_dp_mst_master_trans_needs_modeset(struct intel_atomic_state *state,
> +					     enum transcoder master);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.24.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Souza, Jose]

On Wed, 2019-12-18 at 12:41 -0800, Manasi Navare wrote:
> On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> wrote:
> > Check if fastset is allowed by external dependencies like other
> > pipes
> > and transcoders.
> > 
> > Right now this patch only forces a fullmodeset in MST slaves of MST
> > masters that needs a fullmodeset but it will be needed for port
> > sync
> > as well.
> > 
> > v3:
> > - moved handling to intel_atomic_check() this way is guarantee that
> > all pipes will have its state computed
> > 
> > v4:
> > - added a function to return if MST master neeeds modeset to simply
> > code in intel_atomic_check()
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-
> > ----
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> >  3 files changed, 57 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a4f252d05b37..2a406891567b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const
> > struct intel_crtc_state *old_crtc_sta
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> >  
> >  static int intel_crtc_add_planes_to_state(struct
> > intel_atomic_state *state,
> > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  			any_ms = true;
> >  	}
> >  
> > +	/**
> > +	 * Check if fastset is allowed by external dependencies like
> > other
> > +	 * pipes and transcoders.
> > +	 *
> > +	 * Right now it only forces a fullmodeset when the MST master
> > +	 * transcoder did not changed but the pipe of the master
> > transcoder
> > +	 * needs a fullmodeset so all slaves also needs to do a
> > fullmodeset.
> > +	 */
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i) {
> > +		enum transcoder master = new_crtc_state-
> > >mst_master_transcoder;
> > +
> > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > +		    needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		if (intel_dp_mst_master_trans_needs_modeset(state,
> > master)) {
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> > +	}
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		/*
> > +		 * If we're not doing the full modeset we want to
> > +		 * keep the current M/N values as they may be
> > +		 * sufficiently different to the computed values
> > +		 * to cause problems.
> > +		 *
> > +		 * FIXME: should really copy more fuzzy state here
> > +		 */
> > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > +	}
> 
> Jose, Ville, this is exactly how i would need to modeset all port
> synced crtcs
> after the fastset check?

Yes, in the loop that I check if MST master needs a modeset you should
force modeset to port sync CRTCs when needed.

> 
> Ville had suggsted adding mdoeset_synced_crtcs() even before the
> compute_config or before the call
> where port sync assignments happen, so in that function, it would
> look at the modeset_synced_old_crtc_states ?

Before compute() and intel_crtc_check_fastset() the uapi.mode_changed
would be override.

> 
> Manasi
> 
> 
> Manasi
> 
> > +
> >  	if (any_ms && !check_digital_port_conflicts(state)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port
> > configuration\n");
> >  		ret = EINVAL;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index efd14b0b507b..4aba1d702a83 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct
> > intel_crtc_state *crtc_state)
> >  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > &&
> >  	       crtc_state->mst_master_transcoder != crtc_state-
> > >cpu_transcoder;
> >  }
> > +
> > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > intel_atomic_state *state,
> > +					     enum transcoder master)
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i)
> > +		if (new_crtc_state->mst_master_transcoder == master)
> > +			return
> > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > +
> > +	return false;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index 854724f68f09..72cb486f32ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -8,6 +8,7 @@
> >  
> >  #include <linux/types.h>
> >  
> > +struct intel_atomic_state;
> >  struct intel_digital_port;
> >  struct intel_crtc_state;
> >  
> > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct
> > intel_digital_port *intel_dig_port);
> >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > *intel_dig_port);
> >  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state);
> >  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state);
> > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > intel_atomic_state *state,
> > +					     enum transcoder master);
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > -- 
> > 2.24.1
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v4,1/6] drm/i915/display: Share intel_connector_needs_modeset()

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/6] drm/i915/display: Share intel_connector_needs_modeset()
URL   : https://patchwork.freedesktop.org/series/71129/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7599 -> Patchwork_15831
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15831 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15831, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_15831:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_busy@busy-all:
    - fi-byt-n2820:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-byt-n2820/igt@gem_busy@busy-all.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-byt-n2820/igt@gem_busy@busy-all.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-cfl-guc:         [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-cfl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-cfl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-cfl-8700k:       [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-cfl-8700k/igt@kms_busy@basic-flip-pipe-b.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-cfl-8700k/igt@kms_busy@basic-flip-pipe-b.html

  * igt@kms_busy@basic-flip-pipe-c:
    - fi-skl-guc:         [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-skl-guc/igt@kms_busy@basic-flip-pipe-c.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-skl-guc/igt@kms_busy@basic-flip-pipe-c.html

  * igt@runner@aborted:
    - fi-whl-u:           NOTRUN -> [FAIL][9]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-whl-u/igt@runner@aborted.html
    - fi-bxt-dsi:         NOTRUN -> [FAIL][10]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-bxt-dsi/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_15831 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-j1900:       [PASS][11] -> [TIMEOUT][12] ([i915#816])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-byt-j1900/igt@gem_close_race@basic-threads.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-byt-j1900/igt@gem_close_race@basic-threads.html

  * igt@gem_exec_suspend@basic-s0:
    - fi-cml-s:           [PASS][13] -> [FAIL][14] ([fdo#103375])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-cml-s/igt@gem_exec_suspend@basic-s0.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-cml-s/igt@gem_exec_suspend@basic-s0.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-skl-6770hq:      [PASS][15] -> [INCOMPLETE][16] ([i915#198])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
    - fi-skl-lmem:        [PASS][17] -> [INCOMPLETE][18] ([i915#198])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-skl-lmem/igt@kms_busy@basic-flip-pipe-b.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-skl-lmem/igt@kms_busy@basic-flip-pipe-b.html
    - fi-cml-u2:          [PASS][19] -> [TIMEOUT][20] ([i915#109] / [i915#449])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-cml-u2/igt@kms_busy@basic-flip-pipe-b.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-cml-u2/igt@kms_busy@basic-flip-pipe-b.html
    - fi-apl-guc:         [PASS][21] -> [INCOMPLETE][22] ([fdo#103927])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-apl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-apl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-bxt-dsi:         [PASS][23] -> [TIMEOUT][24] ([i915#109] / [i915#449])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-bxt-dsi/igt@kms_busy@basic-flip-pipe-b.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-bxt-dsi/igt@kms_busy@basic-flip-pipe-b.html
    - fi-whl-u:           [PASS][25] -> [TIMEOUT][26] ([i915#109] / [i915#449])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-whl-u/igt@kms_busy@basic-flip-pipe-b.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-whl-u/igt@kms_busy@basic-flip-pipe-b.html
    - fi-icl-y:           [PASS][27] -> [TIMEOUT][28] ([i915#109] / [i915#449])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-icl-y/igt@kms_busy@basic-flip-pipe-b.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-icl-y/igt@kms_busy@basic-flip-pipe-b.html
    - fi-glk-dsi:         [PASS][29] -> [TIMEOUT][30] ([i915#109] / [i915#449])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-glk-dsi/igt@kms_busy@basic-flip-pipe-b.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-glk-dsi/igt@kms_busy@basic-flip-pipe-b.html
    - fi-tgl-y:           [PASS][31] -> [TIMEOUT][32] ([i915#109] / [i915#449] / [i915#561])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-tgl-y/igt@kms_busy@basic-flip-pipe-b.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-tgl-y/igt@kms_busy@basic-flip-pipe-b.html
    - fi-kbl-soraka:      [PASS][33] -> [TIMEOUT][34] ([i915#109] / [i915#449])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-kbl-soraka/igt@kms_busy@basic-flip-pipe-b.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-kbl-soraka/igt@kms_busy@basic-flip-pipe-b.html
    - fi-icl-guc:         [PASS][35] -> [TIMEOUT][36] ([i915#109] / [i915#449])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-icl-guc/igt@kms_busy@basic-flip-pipe-b.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-icl-guc/igt@kms_busy@basic-flip-pipe-b.html
    - fi-kbl-r:           [PASS][37] -> [TIMEOUT][38] ([i915#109] / [i915#449])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-kbl-r/igt@kms_busy@basic-flip-pipe-b.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-kbl-r/igt@kms_busy@basic-flip-pipe-b.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-n2820:       [TIMEOUT][39] ([i915#816]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-byt-n2820/igt@gem_close_race@basic-threads.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-byt-n2820/igt@gem_close_race@basic-threads.html

  * igt@gem_exec_gttfill@basic:
    - {fi-tgl-u}:         [INCOMPLETE][41] ([fdo#111593]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-tgl-u/igt@gem_exec_gttfill@basic.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-tgl-u/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-cml-s:           [DMESG-WARN][43] ([fdo#111764]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-cml-s/igt@gem_exec_suspend@basic-s3.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-cml-s/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][45] ([i915#725]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-hsw-4770/igt@i915_selftest@live_blt.html

  
#### Warnings ####

  * igt@i915_selftest@live_blt:
    - fi-ivb-3770:        [DMESG-FAIL][47] ([i915#725]) -> [DMESG-FAIL][48] ([i915#770])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-ivb-3770/igt@i915_selftest@live_blt.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-ivb-3770/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-hsw-peppy:       [DMESG-FAIL][49] ([i915#722]) -> [INCOMPLETE][50] ([i915#694])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-kbl-x1275:       [DMESG-WARN][51] ([i915#62] / [i915#92]) -> [INCOMPLETE][52] ([i915#605])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html

  * igt@runner@aborted:
    - fi-icl-guc:         [FAIL][53] ([fdo#110943] / [fdo#111093]) -> [FAIL][54] ([fdo#111093] / [i915#287] / [k.org#203557])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7599/fi-icl-guc/igt@runner@aborted.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/fi-icl-guc/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#110943]: https://bugs.freedesktop.org/show_bug.cgi?id=110943
  [fdo#111093]: https://bugs.freedesktop.org/show_bug.cgi?id=111093
  [fdo#111593]: https://bugs.freedesktop.org/show_bug.cgi?id=111593
  [fdo#111764]: https://bugs.freedesktop.org/show_bug.cgi?id=111764
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#287]: https://gitlab.freedesktop.org/drm/intel/issues/287
  [i915#449]: https://gitlab.freedesktop.org/drm/intel/issues/449
  [i915#561]: https://gitlab.freedesktop.org/drm/intel/issues/561
  [i915#605]: https://gitlab.freedesktop.org/drm/intel/issues/605
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#722]: https://gitlab.freedesktop.org/drm/intel/issues/722
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#770]: https://gitlab.freedesktop.org/drm/intel/issues/770
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [k.org#203557]: https://bugzilla.kernel.org/show_bug.cgi?id=203557


Participating hosts (48 -> 43)
------------------------------

  Additional (2): fi-snb-2520m fi-skl-6600u 
  Missing    (7): fi-ilk-m540 fi-bdw-5557u fi-hsw-4200u fi-bsw-cyan fi-kbl-8809g fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7599 -> Patchwork_15831

  CI-20190529: 20190529
  CI_DRM_7599: 03dfaf2e5f39b632d0187544f3c988b8596f11b0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5351: e7fdcef72d1d6b3bb9f3003bbc37571959e6e8bb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15831: 2a2e8a0e7a473566b6014001142352682360acbf @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2a2e8a0e7a47 drm/i915/display: Add comment to a function that probably can be removed
f0682bef2dce drm/i915/display: Check if pipe fastset is allowed by external dependencies
aafd5f147577 drm/i915/dp: Fix MST disable sequences
fd29285dc8bc drm/i915/display: Always enables MST master pipe first
0904a031fbab drm/i915/tgl: Select master transcoder for MST stream
58a9550588ee drm/i915/display: Share intel_connector_needs_modeset()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15831/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 4/6] drm/i915/dp: Fix MST disable sequences

[Souza, Jose]

On Wed, 2019-12-18 at 22:08 +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 10:59:08AM -0800, José Roberto de Souza
> wrote:
> > The disable sequence after wait for transcoder off was not
> > correctly
> > implemented.
> > The MST disable sequence is basically the same for HSW, SKL, ICL
> > and
> > TGL, with just minor changes for TGL.
> > 
> > With this last patch we finally fixed the hotplugs triggered by MST
> > sinks during the disable/enable sequence, those were causing source
> > to try to do a link training while it was not ready causing CPU
> > pipe
> > FIFO underrrus on TGL.
> > 
> > v2: Only unsetting TGL_TRANS_DDI_PORT_MASK for TGL on the post
> > disable sequence
> > 
> > v4: Rebased, moved MST sequences to intel_mst_post_disable_dp()
> > 
> > BSpec: 4231
> > BSpec: 4163
> > BSpec: 22243
> > BSpec: 49190
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c    | 33 +++++++++++++++
> > ------
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 33 +++++++++++++--
> > ------
> >  2 files changed, 44 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 9d99ec82d072..94ca26be2fee 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -34,6 +34,7 @@
> >  #include "intel_ddi.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp.h"
> > +#include "intel_dp_mst.h"
> >  #include "intel_dp_link_training.h"
> >  #include "intel_dpio_phy.h"
> >  #include "intel_dsi.h"
> > @@ -1949,17 +1950,19 @@ void
> > intel_ddi_disable_transcoder_func(const struct intel_crtc_state
> > *crtc_state
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > -	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
> > -	u32 val = I915_READ(reg);
> > +	u32 val;
> > +
> > +	val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > +	val &= ~TRANS_DDI_FUNC_ENABLE;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 12) {
> > -		val &= ~(TRANS_DDI_FUNC_ENABLE |
> > TGL_TRANS_DDI_PORT_MASK |
> > -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > +		if (!intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_DP_MST) ||
> > +		    intel_dp_mst_is_slave_trans(crtc_state))
> 
> if (!intel_dp_mst_is_master_trans())
> 	val &= ...;

Makes cleaner, thanks

> 
> ?
> 
> > +			val &= ~TGL_TRANS_DDI_PORT_MASK;
> >  	} else {
> > -		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
> > -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > +		val &= ~TRANS_DDI_PORT_MASK;
> >  	}
> > -	I915_WRITE(reg, val);
> > +	I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
> >  
> >  	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
> >  	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> > @@ -3808,8 +3811,20 @@ static void intel_ddi_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	 */
> >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  
> > -	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
> > -		intel_ddi_disable_pipe_clock(old_crtc_state);
> > +	if (INTEL_GEN(dev_priv) >= 12) {
> > +		if (is_mst) {
> 
> if (intel_dp_mst_is_master_trans()) {

If you are talking about replace "if (is_mst)" by "if
(intel_dp_mst_is_master_trans())" both are the same as this function
will only be called for master. But if you are thinking in replace "if
(INTEL_GEN(dev_priv) >= 12)" too it will not work.

Will keep is_mst as we already have it the stack of this functions and
is shorter

> 
> ?
> 
> > +			enum transcoder cpu_transcoder;
> > +			u32 val;
> > +
> > +			cpu_transcoder = old_crtc_state-
> > >cpu_transcoder;
> 
> Assignment can be done when declaring the variable.

Okay it is less than 100col.

> 
> > +			val =
> > I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > +			val &= ~TGL_TRANS_DDI_PORT_MASK;
> > +			I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder),
> > val);
> > +		}
> > +	} else {
> > +		if (!is_mst)
> > +			intel_ddi_disable_pipe_clock(old_crtc_state);
> > +	}
> >  
> >  	intel_disable_ddi_buf(encoder, old_crtc_state);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 710137984c71..efd14b0b507b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -347,6 +347,7 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  		to_intel_connector(old_conn_state->connector);
> >  	struct drm_i915_private *dev_priv = to_i915(connector-
> > >base.dev);
> >  	bool last_mst_stream;
> > +	u32 val;
> >  
> >  	intel_dp->active_mst_links--;
> >  	last_mst_stream = intel_dp->active_mst_links == 0;
> > @@ -357,6 +358,19 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  
> >  	intel_disable_pipe(old_crtc_state);
> >  
> > +	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> 
> Hmm. I'm having hard to figuring out what these things do. But from a
> cursory glance it almost looks like part1 is the one that does the
> AUX
> stuff to deallocate stuff. So feels like even that part should be
> here.

part2 also do aux stuff.
I also did not deep dive into this part but other drivers also split
part1 and part2. 

> 
> > +
> > +	val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state-
> > >cpu_transcoder));
> > +	val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC;
> > +	I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder),
> > val);
> > +
> > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > >regs.dp_tp_status,
> > +				  DP_TP_STATUS_ACT_SENT, 1))
> > +		DRM_ERROR("Timed out waiting for ACT sent when
> > disabling\n");
> 
> I guess we were missing this step entirely. Dunno if we want ERROR
> for
> this. Not sure how much noise it'll generate when someone forcefully
> yanks the cable...

This waits the HW send the ACT, it do not wait for any sink
confirmation. We dont get any warning when removing the cable.

> 
> > +	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > +
> > +	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector-
> > >port);
> 
> This seems to be pure sw stuff so not so critical where it is.
> Keeping
> it next to the hw payload deallocation seems like a good approach.
> 
> > +
> >  	intel_ddi_disable_transcoder_func(old_crtc_state);
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9)
> > @@ -364,6 +378,12 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	else
> >  		ironlake_pfit_disable(old_crtc_state);
> >  
> > +	/*
> > +	 * Power down mst path before disabling the port, otherwise we
> > end
> > +	 * up getting interrupts from the sink upon detecting link
> > loss.
> > +	 */
> > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > >port,
> > +				     false);
> 
> And I guess this was needed to avoid the link loss screams from the
> sink?

Yes, we were doing it too late and sink was not happy.

> 
> Seems to match the spec better than before at least. Still a bit
> unsure
> about he part1/2 stuff for the deallocation. But if that should be
> changed we can do it as a followup.

Sure

> 
> The code movement into .post_disable() really did make for a much
> nicer experience when comparing with the spec. Or at least I didn't
> lose as much hair as with the previous version :)

Thanks for that, I had imagined a way bigger change when you suggested
it.

> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> >  	/*
> >  	 * From TGL spec: "If multi-stream slave transcoder: Configure
> >  	 * Transcoder Clock Select to direct no clock to the
> > transcoder"
> > @@ -374,19 +394,6 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream)
> >  		intel_ddi_disable_pipe_clock(old_crtc_state);
> >  
> > -	/* this can fail */
> > -	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > -	/* and this can also fail */
> > -	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> > -
> > -	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector-
> > >port);
> > -
> > -	/*
> > -	 * Power down mst path before disabling the port, otherwise we
> > end
> > -	 * up getting interrupts from the sink upon detecting link
> > loss.
> > -	 */
> > -	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > >port,
> > -				     false);
> >  
> >  	intel_mst->connector = NULL;
> >  	if (last_mst_stream)
> > -- 
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 4/6] drm/i915/dp: Fix MST disable sequences

[Ville Syrjälä]

On Wed, Dec 18, 2019 at 09:25:33PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 22:08 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 18, 2019 at 10:59:08AM -0800, José Roberto de Souza
> > wrote:
> > > The disable sequence after wait for transcoder off was not
> > > correctly
> > > implemented.
> > > The MST disable sequence is basically the same for HSW, SKL, ICL
> > > and
> > > TGL, with just minor changes for TGL.
> > > 
> > > With this last patch we finally fixed the hotplugs triggered by MST
> > > sinks during the disable/enable sequence, those were causing source
> > > to try to do a link training while it was not ready causing CPU
> > > pipe
> > > FIFO underrrus on TGL.
> > > 
> > > v2: Only unsetting TGL_TRANS_DDI_PORT_MASK for TGL on the post
> > > disable sequence
> > > 
> > > v4: Rebased, moved MST sequences to intel_mst_post_disable_dp()
> > > 
> > > BSpec: 4231
> > > BSpec: 4163
> > > BSpec: 22243
> > > BSpec: 49190
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c    | 33 +++++++++++++++
> > > ------
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 33 +++++++++++++--
> > > ------
> > >  2 files changed, 44 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 9d99ec82d072..94ca26be2fee 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -34,6 +34,7 @@
> > >  #include "intel_ddi.h"
> > >  #include "intel_display_types.h"
> > >  #include "intel_dp.h"
> > > +#include "intel_dp_mst.h"
> > >  #include "intel_dp_link_training.h"
> > >  #include "intel_dpio_phy.h"
> > >  #include "intel_dsi.h"
> > > @@ -1949,17 +1950,19 @@ void
> > > intel_ddi_disable_transcoder_func(const struct intel_crtc_state
> > > *crtc_state
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > -	i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder);
> > > -	u32 val = I915_READ(reg);
> > > +	u32 val;
> > > +
> > > +	val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > > +	val &= ~TRANS_DDI_FUNC_ENABLE;
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 12) {
> > > -		val &= ~(TRANS_DDI_FUNC_ENABLE |
> > > TGL_TRANS_DDI_PORT_MASK |
> > > -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > > +		if (!intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_DP_MST) ||
> > > +		    intel_dp_mst_is_slave_trans(crtc_state))
> > 
> > if (!intel_dp_mst_is_master_trans())
> > 	val &= ...;
> 
> Makes cleaner, thanks
> 
> > 
> > ?
> > 
> > > +			val &= ~TGL_TRANS_DDI_PORT_MASK;
> > >  	} else {
> > > -		val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK |
> > > -			 TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > > +		val &= ~TRANS_DDI_PORT_MASK;
> > >  	}
> > > -	I915_WRITE(reg, val);
> > > +	I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
> > >  
> > >  	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME &&
> > >  	    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> > > @@ -3808,8 +3811,20 @@ static void intel_ddi_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	 */
> > >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > >  
> > > -	if (INTEL_GEN(dev_priv) < 12 && !is_mst)
> > > -		intel_ddi_disable_pipe_clock(old_crtc_state);
> > > +	if (INTEL_GEN(dev_priv) >= 12) {
> > > +		if (is_mst) {
> > 
> > if (intel_dp_mst_is_master_trans()) {
> 
> If you are talking about replace "if (is_mst)" by "if
> (intel_dp_mst_is_master_trans())" both are the same as this function
> will only be called for master. But if you are thinking in replace "if
> (INTEL_GEN(dev_priv) >= 12)" too it will not work.
> 
> Will keep is_mst as we already have it the stack of this functions and
> is shorter

Was just trying to match against the spec. is_master_trans() is
1:1 with the spec so least amount of thinking required.

> 
> > 
> > ?
> > 
> > > +			enum transcoder cpu_transcoder;
> > > +			u32 val;
> > > +
> > > +			cpu_transcoder = old_crtc_state-
> > > >cpu_transcoder;
> > 
> > Assignment can be done when declaring the variable.
> 
> Okay it is less than 100col.
> 
> > 
> > > +			val =
> > > I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > > +			val &= ~TGL_TRANS_DDI_PORT_MASK;
> > > +			I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder),
> > > val);
> > > +		}
> > > +	} else {
> > > +		if (!is_mst)
> > > +			intel_ddi_disable_pipe_clock(old_crtc_state);
> > > +	}
> > >  
> > >  	intel_disable_ddi_buf(encoder, old_crtc_state);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index 710137984c71..efd14b0b507b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -347,6 +347,7 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  		to_intel_connector(old_conn_state->connector);
> > >  	struct drm_i915_private *dev_priv = to_i915(connector-
> > > >base.dev);
> > >  	bool last_mst_stream;
> > > +	u32 val;
> > >  
> > >  	intel_dp->active_mst_links--;
> > >  	last_mst_stream = intel_dp->active_mst_links == 0;
> > > @@ -357,6 +358,19 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  
> > >  	intel_disable_pipe(old_crtc_state);
> > >  
> > > +	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> > 
> > Hmm. I'm having hard to figuring out what these things do. But from a
> > cursory glance it almost looks like part1 is the one that does the
> > AUX
> > stuff to deallocate stuff. So feels like even that part should be
> > here.
> 
> part2 also do aux stuff.
> I also did not deep dive into this part but other drivers also split
> part1 and part2. 
> 
> > 
> > > +
> > > +	val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state-
> > > >cpu_transcoder));
> > > +	val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC;
> > > +	I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder),
> > > val);
> > > +
> > > +	if (intel_de_wait_for_set(dev_priv, intel_dp-
> > > >regs.dp_tp_status,
> > > +				  DP_TP_STATUS_ACT_SENT, 1))
> > > +		DRM_ERROR("Timed out waiting for ACT sent when
> > > disabling\n");
> > 
> > I guess we were missing this step entirely. Dunno if we want ERROR
> > for
> > this. Not sure how much noise it'll generate when someone forcefully
> > yanks the cable...
> 
> This waits the HW send the ACT, it do not wait for any sink
> confirmation. We dont get any warning when removing the cable.

Not really sure what it waits for. Some internal state machine I guess.
Just wasn't sure that wasn't tied to some actual wiggling of some pins
which would require the other end of the cable to be awake.

> 
> > 
> > > +	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > > +
> > > +	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector-
> > > >port);
> > 
> > This seems to be pure sw stuff so not so critical where it is.
> > Keeping
> > it next to the hw payload deallocation seems like a good approach.
> > 
> > > +
> > >  	intel_ddi_disable_transcoder_func(old_crtc_state);
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9)
> > > @@ -364,6 +378,12 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	else
> > >  		ironlake_pfit_disable(old_crtc_state);
> > >  
> > > +	/*
> > > +	 * Power down mst path before disabling the port, otherwise we
> > > end
> > > +	 * up getting interrupts from the sink upon detecting link
> > > loss.
> > > +	 */
> > > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > > >port,
> > > +				     false);
> > 
> > And I guess this was needed to avoid the link loss screams from the
> > sink?
> 
> Yes, we were doing it too late and sink was not happy.
> 
> > 
> > Seems to match the spec better than before at least. Still a bit
> > unsure
> > about he part1/2 stuff for the deallocation. But if that should be
> > changed we can do it as a followup.
> 
> Sure
> 
> > 
> > The code movement into .post_disable() really did make for a much
> > nicer experience when comparing with the spec. Or at least I didn't
> > lose as much hair as with the previous version :)
> 
> Thanks for that, I had imagined a way bigger change when you suggested
> it.
> 
> > 
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > >  	/*
> > >  	 * From TGL spec: "If multi-stream slave transcoder: Configure
> > >  	 * Transcoder Clock Select to direct no clock to the
> > > transcoder"
> > > @@ -374,19 +394,6 @@ static void intel_mst_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream)
> > >  		intel_ddi_disable_pipe_clock(old_crtc_state);
> > >  
> > > -	/* this can fail */
> > > -	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > > -	/* and this can also fail */
> > > -	drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> > > -
> > > -	drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector-
> > > >port);
> > > -
> > > -	/*
> > > -	 * Power down mst path before disabling the port, otherwise we
> > > end
> > > -	 * up getting interrupts from the sink upon detecting link
> > > loss.
> > > -	 */
> > > -	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > > >port,
> > > -				     false);
> > >  
> > >  	intel_mst->connector = NULL;
> > >  	if (last_mst_stream)
> > > -- 
> > > 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Manasi Navare]

On Wed, Dec 18, 2019 at 12:57:04PM -0800, Souza, Jose wrote:
> On Wed, 2019-12-18 at 12:41 -0800, Manasi Navare wrote:
> > On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> > wrote:
> > > Check if fastset is allowed by external dependencies like other
> > > pipes
> > > and transcoders.
> > > 
> > > Right now this patch only forces a fullmodeset in MST slaves of MST
> > > masters that needs a fullmodeset but it will be needed for port
> > > sync
> > > as well.
> > > 
> > > v3:
> > > - moved handling to intel_atomic_check() this way is guarantee that
> > > all pipes will have its state computed
> > > 
> > > v4:
> > > - added a function to return if MST master neeeds modeset to simply
> > > code in intel_atomic_check()
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 53 +++++++++++++++-
> > > ----
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> > >  3 files changed, 57 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index a4f252d05b37..2a406891567b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13903,19 +13903,6 @@ static void intel_crtc_check_fastset(const
> > > struct intel_crtc_state *old_crtc_sta
> > >  
> > >  	new_crtc_state->uapi.mode_changed = false;
> > >  	new_crtc_state->update_pipe = true;
> > > -
> > > -	/*
> > > -	 * If we're not doing the full modeset we want to
> > > -	 * keep the current M/N values as they may be
> > > -	 * sufficiently different to the computed values
> > > -	 * to cause problems.
> > > -	 *
> > > -	 * FIXME: should really copy more fuzzy state here
> > > -	 */
> > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > >  }
> > >  
> > >  static int intel_crtc_add_planes_to_state(struct
> > > intel_atomic_state *state,
> > > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  			any_ms = true;
> > >  	}
> > >  
> > > +	/**
> > > +	 * Check if fastset is allowed by external dependencies like
> > > other
> > > +	 * pipes and transcoders.
> > > +	 *
> > > +	 * Right now it only forces a fullmodeset when the MST master
> > > +	 * transcoder did not changed but the pipe of the master
> > > transcoder
> > > +	 * needs a fullmodeset so all slaves also needs to do a
> > > fullmodeset.
> > > +	 */
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i) {
> > > +		enum transcoder master = new_crtc_state-
> > > >mst_master_transcoder;
> > > +
> > > +		if (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > +		    needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		if (intel_dp_mst_master_trans_needs_modeset(state,
> > > master)) {
> > > +			new_crtc_state->uapi.mode_changed = true;
> > > +			new_crtc_state->update_pipe = false;
> > > +		}
> > > +	}
> > > +
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		if (needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		/*
> > > +		 * If we're not doing the full modeset we want to
> > > +		 * keep the current M/N values as they may be
> > > +		 * sufficiently different to the computed values
> > > +		 * to cause problems.
> > > +		 *
> > > +		 * FIXME: should really copy more fuzzy state here
> > > +		 */
> > > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > +	}
> > 
> > Jose, Ville, this is exactly how i would need to modeset all port
> > synced crtcs
> > after the fastset check?
> 
> Yes, in the loop that I check if MST master needs a modeset you should
> force modeset to port sync CRTCs when needed.
> 
> > 
> > Ville had suggsted adding mdoeset_synced_crtcs() even before the
> > compute_config or before the call
> > where port sync assignments happen, so in that function, it would
> > look at the modeset_synced_old_crtc_states ?
> 
> Before compute() and intel_crtc_check_fastset() the uapi.mode_changed
> would be override.

You mean before compute() and after intel_crtc_check_fastset() we would override
uapi.mode_changed() ?

Manasi

> 
> > 
> > Manasi
> > 
> > 
> > Manasi
> > 
> > > +
> > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > >  		DRM_DEBUG_KMS("rejecting conflicting digital port
> > > configuration\n");
> > >  		ret = EINVAL;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index efd14b0b507b..4aba1d702a83 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const struct
> > > intel_crtc_state *crtc_state)
> > >  	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > > &&
> > >  	       crtc_state->mst_master_transcoder != crtc_state-
> > > >cpu_transcoder;
> > >  }
> > > +
> > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > intel_atomic_state *state,
> > > +					     enum transcoder master)
> > > +{
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i)
> > > +		if (new_crtc_state->mst_master_transcoder == master)
> > > +			return
> > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > > +
> > > +	return false;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > index 854724f68f09..72cb486f32ab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > @@ -8,6 +8,7 @@
> > >  
> > >  #include <linux/types.h>
> > >  
> > > +struct intel_atomic_state;
> > >  struct intel_digital_port;
> > >  struct intel_crtc_state;
> > >  
> > > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct
> > > intel_digital_port *intel_dig_port);
> > >  int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > > *intel_dig_port);
> > >  bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > > *crtc_state);
> > >  bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > > *crtc_state);
> > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > intel_atomic_state *state,
> > > +					     enum transcoder master);
> > >  
> > >  #endif /* __INTEL_DP_MST_H__ */
> > > -- 
> > > 2.24.1
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Souza, Jose]

On Wed, 2019-12-18 at 20:33 +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 22:26 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 18, 2019 at 08:19:15PM +0000, Souza, Jose wrote:
> > > On Wed, 2019-12-18 at 21:39 +0200, Ville Syrjälä wrote:
> > > > On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> > > > wrote:
> > > > > Check if fastset is allowed by external dependencies like
> > > > > other
> > > > > pipes
> > > > > and transcoders.
> > > > > 
> > > > > Right now this patch only forces a fullmodeset in MST slaves
> > > > > of
> > > > > MST
> > > > > masters that needs a fullmodeset but it will be needed for
> > > > > port
> > > > > sync
> > > > > as well.
> > > > > 
> > > > > v3:
> > > > > - moved handling to intel_atomic_check() this way is
> > > > > guarantee
> > > > > that
> > > > > all pipes will have its state computed
> > > > > 
> > > > > v4:
> > > > > - added a function to return if MST master neeeds modeset to
> > > > > simply
> > > > > code in intel_atomic_check()
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 53
> > > > > +++++++++++++++-
> > > > > ----
> > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> > > > >  3 files changed, 57 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index a4f252d05b37..2a406891567b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -13903,19 +13903,6 @@ static void
> > > > > intel_crtc_check_fastset(const
> > > > > struct intel_crtc_state *old_crtc_sta
> > > > >  
> > > > >  	new_crtc_state->uapi.mode_changed = false;
> > > > >  	new_crtc_state->update_pipe = true;
> > > > > -
> > > > > -	/*
> > > > > -	 * If we're not doing the full modeset we want to
> > > > > -	 * keep the current M/N values as they may be
> > > > > -	 * sufficiently different to the computed values
> > > > > -	 * to cause problems.
> > > > > -	 *
> > > > > -	 * FIXME: should really copy more fuzzy state here
> > > > > -	 */
> > > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > > >  }
> > > > >  
> > > > >  static int intel_crtc_add_planes_to_state(struct
> > > > > intel_atomic_state *state,
> > > > > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > > > > drm_device *dev,
> > > > >  			any_ms = true;
> > > > >  	}
> > > > >  
> > > > > +	/**
> > > > > +	 * Check if fastset is allowed by external dependencies
> > > > > like
> > > > > other
> > > > > +	 * pipes and transcoders.
> > > > > +	 *
> > > > > +	 * Right now it only forces a fullmodeset when the MST
> > > > > master
> > > > > +	 * transcoder did not changed but the pipe of the
> > > > > master
> > > > > transcoder
> > > > > +	 * needs a fullmodeset so all slaves also needs to do a
> > > > > fullmodeset.
> > > > > +	 */
> > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > new_crtc_state,
> > > > > i) {
> > > > > +		enum transcoder master = new_crtc_state-
> > > > > > mst_master_transcoder;
> > > > > +
> > > > > +		if
> > > > > (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > > > +		    needs_modeset(new_crtc_state))
> > > > > +			continue;
> > > > > +
> > > > > +		if
> > > > > (intel_dp_mst_master_trans_needs_modeset(state,
> > > > > master)) {
> > > > 
> > > > I think this has the loops the opposite way of what I was
> > > > thinking,
> > > > but should work fine I think... OK. I'm convinced your way is
> > > > in
> > > > fact
> > > > better.
> > > > 
> > > > > +			new_crtc_state->uapi.mode_changed =
> > > > > true;
> > > > > +			new_crtc_state->update_pipe = false;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > > old_crtc_state,
> > > > > +					    new_crtc_state, i)
> > > > > {
> > > > > +		if (needs_modeset(new_crtc_state))
> > > > > +			continue;
> > > > 
> > > > I suppose there isn't any way we should have crtcs in the state
> > > > that
> > > > neither have update_pipe or needs_modeset flagged here. Could
> > > > maybe
> > > > WARN_ON(!update_pipe) here if we're being paranoid.
> > > 
> > > Adding the WARN_ON

Ah we can't add this WARN_ON(), is most atomic states the previous CRTC
states is added even if it dont need any modification.

Those are handled here:

	for_each_oldnew_intel_crtc_in_state(state, crtc,
old_crtc_state,
					    new_crtc_state, i) {
		if (!needs_modeset(new_crtc_state)) {
			/* Light copy */
			intel_crtc_copy_uapi_to_hw_state_nomodeset(new_
crtc_state);

			continue;
		}



> > > 
> > > > > +
> > > > > +		/*
> > > > > +		 * If we're not doing the full modeset we want
> > > > > to
> > > > > +		 * keep the current M/N values as they may be
> > > > > +		 * sufficiently different to the computed
> > > > > values
> > > > > +		 * to cause problems.
> > > > > +		 *
> > > > > +		 * FIXME: should really copy more fuzzy state
> > > > > here
> > > > > +		 */
> > > > > +		new_crtc_state->fdi_m_n = old_crtc_state-
> > > > > > fdi_m_n;
> > > > > +		new_crtc_state->dp_m_n = old_crtc_state-
> > > > > > dp_m_n;
> > > > > +		new_crtc_state->dp_m2_n2 = old_crtc_state-
> > > > > > dp_m2_n2;
> > > > > +		new_crtc_state->has_drrs = old_crtc_state-
> > > > > > has_drrs;
> > > > 
> > > > Still a bit unhappy having this state copy inlined in
> > > > intel_atomic_check().
> > > > 
> > > > > +	}
> > > > > +
> > > > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > > > >  		DRM_DEBUG_KMS("rejecting conflicting digital
> > > > > port
> > > > > configuration\n");
> > > > >  		ret = EINVAL;
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > index efd14b0b507b..4aba1d702a83 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > @@ -854,3 +854,17 @@ bool intel_dp_mst_is_slave_trans(const
> > > > > struct
> > > > > intel_crtc_state *crtc_state)
> > > > >  	return crtc_state->mst_master_transcoder !=
> > > > > INVALID_TRANSCODER
> > > > > &&
> > > > >  	       crtc_state->mst_master_transcoder != crtc_state-
> > > > > > cpu_transcoder;
> > > > >  }
> > > > > +
> > > > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > > > intel_atomic_state *state,
> > > > > +					     enum transcoder
> > > > > master)
> > > > 
> > > > Are we going to need this elsewhere? Or be static in
> > > > intel_display.c?
> > > > Not that people are 100% happy of stuffing everything in there.
> > > 
> > > Not needed elsewhere, I'm also trying to not put more stuff in
> > > intel_display.c.
> > > 
> > > > > +{
> > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > +	struct intel_crtc *crtc;
> > > > > +	int i;
> > > > > +
> > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > new_crtc_state,
> > > > > i)
> > > > > +		if (new_crtc_state->mst_master_transcoder ==
> > > > > master)
> > > > 
> > > > So we need to modeset everything when either the master or any
> > > > other slave wants to modeset? That is, we can't just modeset
> > > > slaves
> > > > independently and only force the modesets for all slaves if the
> > > > master
> > > > needs a modeset?
> > > 
> > > If a slave needs a modeset we don't need to do a modeset in
> > > master
> > > or
> > > in other slaves.
> > > 
> > > This is doing independent modeset in slaves and forcing modeset
> > > to
> > > all
> > > slaves when master needs as modeset.
> > 
> > If you just wanted to check if the master needs a modeset then
> > shouldn't
> > it just compare crtc_state->cpu_transcoder == master?
> 
> Good catch, thanks. 
> 
> > And in that case it's not at all MST specific and we could just
> > call
> > it
> > intel_cpu_transcoder_needs_modeset() or something.
> 
> That is true, moving to intel_display then.
> 
> > > > > +			return
> > > > > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi);
> > > > > +
> > > > > +	return false;
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > > index 854724f68f09..72cb486f32ab 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > > > > @@ -8,6 +8,7 @@
> > > > >  
> > > > >  #include <linux/types.h>
> > > > >  
> > > > > +struct intel_atomic_state;
> > > > >  struct intel_digital_port;
> > > > >  struct intel_crtc_state;
> > > > >  
> > > > > @@ -16,5 +17,7 @@ void intel_dp_mst_encoder_cleanup(struct
> > > > > intel_digital_port *intel_dig_port);
> > > > >  int intel_dp_mst_encoder_active_links(struct
> > > > > intel_digital_port
> > > > > *intel_dig_port);
> > > > >  bool intel_dp_mst_is_master_trans(const struct
> > > > > intel_crtc_state
> > > > > *crtc_state);
> > > > >  bool intel_dp_mst_is_slave_trans(const struct
> > > > > intel_crtc_state
> > > > > *crtc_state);
> > > > > +bool intel_dp_mst_master_trans_needs_modeset(struct
> > > > > intel_atomic_state *state,
> > > > > +					     enum transcoder
> > > > > master);
> > > > >  
> > > > >  #endif /* __INTEL_DP_MST_H__ */
> > > > > -- 
> > > > > 2.24.1
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 5/6] drm/i915/display: Check if pipe fastset is allowed by external dependencies

[Ville Syrjälä]

On Thu, Dec 19, 2019 at 08:39:26PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 20:33 +0000, Souza, Jose wrote:
> > On Wed, 2019-12-18 at 22:26 +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 18, 2019 at 08:19:15PM +0000, Souza, Jose wrote:
> > > > On Wed, 2019-12-18 at 21:39 +0200, Ville Syrjälä wrote:
> > > > > On Wed, Dec 18, 2019 at 10:59:09AM -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > Check if fastset is allowed by external dependencies like
> > > > > > other
> > > > > > pipes
> > > > > > and transcoders.
> > > > > > 
> > > > > > Right now this patch only forces a fullmodeset in MST slaves
> > > > > > of
> > > > > > MST
> > > > > > masters that needs a fullmodeset but it will be needed for
> > > > > > port
> > > > > > sync
> > > > > > as well.
> > > > > > 
> > > > > > v3:
> > > > > > - moved handling to intel_atomic_check() this way is
> > > > > > guarantee
> > > > > > that
> > > > > > all pipes will have its state computed
> > > > > > 
> > > > > > v4:
> > > > > > - added a function to return if MST master neeeds modeset to
> > > > > > simply
> > > > > > code in intel_atomic_check()
> > > > > > 
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_display.c | 53
> > > > > > +++++++++++++++-
> > > > > > ----
> > > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 14 ++++++
> > > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
> > > > > >  3 files changed, 57 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index a4f252d05b37..2a406891567b 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -13903,19 +13903,6 @@ static void
> > > > > > intel_crtc_check_fastset(const
> > > > > > struct intel_crtc_state *old_crtc_sta
> > > > > >  
> > > > > >  	new_crtc_state->uapi.mode_changed = false;
> > > > > >  	new_crtc_state->update_pipe = true;
> > > > > > -
> > > > > > -	/*
> > > > > > -	 * If we're not doing the full modeset we want to
> > > > > > -	 * keep the current M/N values as they may be
> > > > > > -	 * sufficiently different to the computed values
> > > > > > -	 * to cause problems.
> > > > > > -	 *
> > > > > > -	 * FIXME: should really copy more fuzzy state here
> > > > > > -	 */
> > > > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > > > >  }
> > > > > >  
> > > > > >  static int intel_crtc_add_planes_to_state(struct
> > > > > > intel_atomic_state *state,
> > > > > > @@ -14083,6 +14070,46 @@ static int intel_atomic_check(struct
> > > > > > drm_device *dev,
> > > > > >  			any_ms = true;
> > > > > >  	}
> > > > > >  
> > > > > > +	/**
> > > > > > +	 * Check if fastset is allowed by external dependencies
> > > > > > like
> > > > > > other
> > > > > > +	 * pipes and transcoders.
> > > > > > +	 *
> > > > > > +	 * Right now it only forces a fullmodeset when the MST
> > > > > > master
> > > > > > +	 * transcoder did not changed but the pipe of the
> > > > > > master
> > > > > > transcoder
> > > > > > +	 * needs a fullmodeset so all slaves also needs to do a
> > > > > > fullmodeset.
> > > > > > +	 */
> > > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > > new_crtc_state,
> > > > > > i) {
> > > > > > +		enum transcoder master = new_crtc_state-
> > > > > > > mst_master_transcoder;
> > > > > > +
> > > > > > +		if
> > > > > > (!intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > > > > +		    needs_modeset(new_crtc_state))
> > > > > > +			continue;
> > > > > > +
> > > > > > +		if
> > > > > > (intel_dp_mst_master_trans_needs_modeset(state,
> > > > > > master)) {
> > > > > 
> > > > > I think this has the loops the opposite way of what I was
> > > > > thinking,
> > > > > but should work fine I think... OK. I'm convinced your way is
> > > > > in
> > > > > fact
> > > > > better.
> > > > > 
> > > > > > +			new_crtc_state->uapi.mode_changed =
> > > > > > true;
> > > > > > +			new_crtc_state->update_pipe = false;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > > > old_crtc_state,
> > > > > > +					    new_crtc_state, i)
> > > > > > {
> > > > > > +		if (needs_modeset(new_crtc_state))
> > > > > > +			continue;
> > > > > 
> > > > > I suppose there isn't any way we should have crtcs in the state
> > > > > that
> > > > > neither have update_pipe or needs_modeset flagged here. Could
> > > > > maybe
> > > > > WARN_ON(!update_pipe) here if we're being paranoid.
> > > > 
> > > > Adding the WARN_ON
> 
> Ah we can't add this WARN_ON(), is most atomic states the previous CRTC
> states is added even if it dont need any modification.
> 
> Those are handled here:
> 
> 	for_each_oldnew_intel_crtc_in_state(state, crtc,
> old_crtc_state,
> 					    new_crtc_state, i) {
> 		if (!needs_modeset(new_crtc_state)) {
> 			/* Light copy */
> 			intel_crtc_copy_uapi_to_hw_state_nomodeset(new_
> crtc_state);
> 
> 			continue;
> 		}

After further consideration we only want the copy for fastset crtcs
(ie. ones with update_pipe==true). Crtcs added for pure plane updates
should not do the copy (well, in that case the old and new states
really should match, but I still think it's a bit dubious). So
I guess we should check for '!needs_modeset() && update_pipe'.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx