[PATCH v2 0/6] drm/i915/mst: enable MST mode for 128b/132b single-stream sideband

[PATCH v2 0/6] drm/i915/mst: enable MST mode for 128b/132b single-stream sideband

[Jani Nikula]

Sort of successor to [1], but revamped.

BR,
Jani.


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

Jani Nikula (6):
  drm/mst: read sideband messaging cap
  drm/i915/mst: improve debug logging of DP MST mode detect
  drm/i915/mst: abstract choosing the MST mode to use
  drm/i915/mst: use the MST mode detected previously
  drm/i915/mst: add intel_dp_mst_disconnect()
  drm/i915/mst: enable MST mode for 128b/132b single-stream sideband

 drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++--
 .../drm/i915/display/intel_display_types.h    |  1 +
 drivers/gpu/drm/i915/display/intel_dp.c       | 90 +++++++++++++------
 drivers/gpu/drm/nouveau/nouveau_dp.c          |  2 +-
 include/drm/display/drm_dp_mst_helper.h       | 23 ++++-
 5 files changed, 99 insertions(+), 37 deletions(-)

-- 
2.39.2

[PATCH v2 1/6] drm/mst: read sideband messaging cap

[Jani Nikula]

Amend drm_dp_read_mst_cap() to return an enum, indicating "SST", "SST
with sideband messaging", or "MST". Modify all call sites to take the
new return value into account.

v2:
- Rename enumerators (Ville)

Cc: Arun R Murthy <arun.r.murthy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 ++++++++++------
 drivers/gpu/drm/i915/display/intel_dp.c       |  4 ++--
 drivers/gpu/drm/nouveau/nouveau_dp.c          |  2 +-
 include/drm/display/drm_dp_mst_helper.h       | 23 ++++++++++++++++++-
 4 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 03d528209426..c193be3577f7 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3608,24 +3608,30 @@ fixed20_12 drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
 EXPORT_SYMBOL(drm_dp_get_vc_payload_bw);
 
 /**
- * drm_dp_read_mst_cap() - check whether or not a sink supports MST
+ * drm_dp_read_mst_cap() - Read the sink's MST mode capability
  * @aux: The DP AUX channel to use
  * @dpcd: A cached copy of the DPCD capabilities for this sink
  *
- * Returns: %True if the sink supports MST, %false otherwise
+ * Returns: enum drm_dp_mst_mode to indicate MST mode capability
  */
-bool drm_dp_read_mst_cap(struct drm_dp_aux *aux,
-			 const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux,
+					 const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
 	u8 mstm_cap;
 
 	if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
-		return false;
+		return DRM_DP_SST;
 
 	if (drm_dp_dpcd_readb(aux, DP_MSTM_CAP, &mstm_cap) != 1)
-		return false;
+		return DRM_DP_SST;
+
+	if (mstm_cap & DP_MST_CAP)
+		return DRM_DP_MST;
+
+	if (mstm_cap & DP_SINGLE_STREAM_SIDEBAND_MSG)
+		return DRM_DP_SST_SIDEBAND_MSG;
 
-	return mstm_cap & DP_MST_CAP;
+	return DRM_DP_SST;
 }
 EXPORT_SYMBOL(drm_dp_read_mst_cap);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 5045c34a16be..a1c304f451bd 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4014,7 +4014,7 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
 
 	return i915->display.params.enable_dp_mst &&
 		intel_dp_mst_source_support(intel_dp) &&
-		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
+		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
 }
 
 static void
@@ -4023,7 +4023,7 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_encoder *encoder =
 		&dp_to_dig_port(intel_dp)->base;
-	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
+	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
 
 	drm_dbg_kms(&i915->drm,
 		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s\n",
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 7de7707ec6a8..fb06ee17d9e5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -181,7 +181,7 @@ nouveau_dp_probe_dpcd(struct nouveau_connector *nv_connector,
 	if (nouveau_mst) {
 		mstm = outp->dp.mstm;
 		if (mstm)
-			mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd);
+			mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd) == DRM_DP_MST;
 	}
 
 	if (nouveau_dp_has_sink_count(connector, outp)) {
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 9b19d8bd520a..3c9e128c444a 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -818,7 +818,28 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 
 void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr);
 
-bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
+/**
+ * enum drm_dp_mst_mode - sink's MST mode capability
+ */
+enum drm_dp_mst_mode {
+	/**
+	 * @DRM_DP_SST: The sink does not support MST nor single stream sideband
+	 * messaging.
+	 */
+	DRM_DP_SST,
+	/**
+	 * @DRM_DP_MST: Sink supports MST, more than one stream and single
+	 * stream sideband messaging.
+	 */
+	DRM_DP_MST,
+	/**
+	 * @DRM_DP_SST_SIDEBAND_MSG: Sink supports only one stream and single
+	 * stream sideband messaging.
+	 */
+	DRM_DP_SST_SIDEBAND_MSG,
+};
+
+enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state);
 
 int drm_dp_mst_hpd_irq_handle_event(struct drm_dp_mst_topology_mgr *mgr,
-- 
2.39.2

[PATCH v2 2/6] drm/i915/mst: improve debug logging of DP MST mode detect

[Jani Nikula]

Rename intel_dp_can_mst() to intel_dp_mst_detect(), and move all DP MST
detect debug logging there. Debug log the sink's MST capability,
including single-stream sideband messaging support, and the decision
whether to enable MST mode or not. Do this regardless of whether we're
actually enabling MST or not.

Cc: Arun R Murthy <arun.r.murthy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 45 +++++++++++++++++--------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a1c304f451bd..944f566525dd 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4007,31 +4007,48 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 					   intel_dp->downstream_ports) == 0;
 }
 
+static const char *intel_dp_mst_mode_str(enum drm_dp_mst_mode mst_mode)
+{
+	if (mst_mode == DRM_DP_SST_SIDEBAND_MSG)
+		return "single-stream sideband messaging";
+	else
+		return str_yes_no(mst_mode == DRM_DP_MST);
+}
+
 static bool
-intel_dp_can_mst(struct intel_dp *intel_dp)
+intel_dp_mst_detect(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+	enum drm_dp_mst_mode sink_mst_mode;
+	enum drm_dp_mst_mode mst_detect;
+
+	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
+
+	if (i915->display.params.enable_dp_mst &&
+	    intel_dp_mst_source_support(intel_dp) &&
+	    sink_mst_mode == DRM_DP_MST)
+		mst_detect = DRM_DP_MST;
+	else
+		mst_detect = DRM_DP_SST;
 
-	return i915->display.params.enable_dp_mst &&
-		intel_dp_mst_source_support(intel_dp) &&
-		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
+	drm_dbg_kms(&i915->drm,
+		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
+		    encoder->base.base.id, encoder->base.name,
+		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
+		    intel_dp_mst_mode_str(sink_mst_mode),
+		    str_yes_no(i915->display.params.enable_dp_mst),
+		    intel_dp_mst_mode_str(mst_detect));
+
+	return mst_detect != DRM_DP_SST;
 }
 
 static void
 intel_dp_configure_mst(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	struct intel_encoder *encoder =
-		&dp_to_dig_port(intel_dp)->base;
 	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
 
-	drm_dbg_kms(&i915->drm,
-		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s\n",
-		    encoder->base.base.id, encoder->base.name,
-		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
-		    str_yes_no(sink_can_mst),
-		    str_yes_no(i915->display.params.enable_dp_mst));
-
 	if (!intel_dp_mst_source_support(intel_dp))
 		return;
 
@@ -5390,7 +5407,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 		connector_status_connected : connector_status_disconnected;
 	}
 
-	if (intel_dp_can_mst(intel_dp))
+	if (intel_dp_mst_detect(intel_dp))
 		return connector_status_connected;
 
 	/* If no HPD, poke DDC gently */
-- 
2.39.2

[PATCH v2 3/6] drm/i915/mst: abstract choosing the MST mode to use

[Jani Nikula]

Clarify the conditions for choosing the MST mode to use by adding a new
function intel_dp_mst_mode_choose(). This also prepares for being able
to extend the MST modes to single-stream sideband messaging.

Cc: Arun R Murthy <arun.r.murthy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 944f566525dd..007cb2a04e38 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4015,6 +4015,24 @@ static const char *intel_dp_mst_mode_str(enum drm_dp_mst_mode mst_mode)
 		return str_yes_no(mst_mode == DRM_DP_MST);
 }
 
+static enum drm_dp_mst_mode
+intel_dp_mst_mode_choose(struct intel_dp *intel_dp,
+			 enum drm_dp_mst_mode sink_mst_mode)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+	if (!i915->display.params.enable_dp_mst)
+		return DRM_DP_SST;
+
+	if (!intel_dp_mst_source_support(intel_dp))
+		return DRM_DP_SST;
+
+	if (sink_mst_mode == DRM_DP_SST_SIDEBAND_MSG)
+		return DRM_DP_SST;
+
+	return sink_mst_mode;
+}
+
 static bool
 intel_dp_mst_detect(struct intel_dp *intel_dp)
 {
@@ -4025,12 +4043,7 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
 
 	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
 
-	if (i915->display.params.enable_dp_mst &&
-	    intel_dp_mst_source_support(intel_dp) &&
-	    sink_mst_mode == DRM_DP_MST)
-		mst_detect = DRM_DP_MST;
-	else
-		mst_detect = DRM_DP_SST;
+	mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
 
 	drm_dbg_kms(&i915->drm,
 		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
-- 
2.39.2

[PATCH v2 4/6] drm/i915/mst: use the MST mode detected previously

[Jani Nikula]

Drop the duplicate read of DP_MSTM_CAP DPCD register, and the duplicate
logic for choosing MST mode, and store the chosen mode in struct
intel_dp. Rename intel_dp_configure_mst() to intel_dp_mst_configure()
while at it.

Cc: Arun R Murthy <arun.r.murthy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  1 +
 drivers/gpu/drm/i915/display/intel_dp.c       | 23 ++++++++-----------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 01eb6e4e6049..4a8440a3a812 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1780,6 +1780,7 @@ struct intel_dp {
 
 	bool is_mst;
 	int active_mst_links;
+	enum drm_dp_mst_mode mst_detect;
 
 	/* connector directly attached - won't be use for modeset in mst world */
 	struct intel_connector *attached_connector;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 007cb2a04e38..72e91322e310 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4039,11 +4039,10 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	enum drm_dp_mst_mode sink_mst_mode;
-	enum drm_dp_mst_mode mst_detect;
 
 	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
 
-	mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
+	intel_dp->mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
 
 	drm_dbg_kms(&i915->drm,
 		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
@@ -4051,25 +4050,23 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
 		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
 		    intel_dp_mst_mode_str(sink_mst_mode),
 		    str_yes_no(i915->display.params.enable_dp_mst),
-		    intel_dp_mst_mode_str(mst_detect));
+		    intel_dp_mst_mode_str(intel_dp->mst_detect));
 
-	return mst_detect != DRM_DP_SST;
+	return intel_dp->mst_detect != DRM_DP_SST;
 }
 
 static void
-intel_dp_configure_mst(struct intel_dp *intel_dp)
+intel_dp_mst_configure(struct intel_dp *intel_dp)
 {
-	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
-
 	if (!intel_dp_mst_source_support(intel_dp))
 		return;
 
-	intel_dp->is_mst = sink_can_mst &&
-		i915->display.params.enable_dp_mst;
+	intel_dp->is_mst = intel_dp->mst_detect != DRM_DP_SST;
+
+	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
 
-	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
-					intel_dp->is_mst);
+	/* Avoid stale info on the next detect cycle. */
+	intel_dp->mst_detect = DRM_DP_SST;
 }
 
 static bool
@@ -5739,7 +5736,7 @@ intel_dp_detect(struct drm_connector *connector,
 
 	intel_dp_detect_dsc_caps(intel_dp, intel_connector);
 
-	intel_dp_configure_mst(intel_dp);
+	intel_dp_mst_configure(intel_dp);
 
 	/*
 	 * TODO: Reset link params when switching to MST mode, until MST
-- 
2.39.2

[PATCH v2 5/6] drm/i915/mst: add intel_dp_mst_disconnect()

[Jani Nikula]

Abstract the MST mode disconnect to a separate function.

Cc: Arun R Murthy <arun.r.murthy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 72e91322e310..7f97d5512a3e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4069,6 +4069,20 @@ intel_dp_mst_configure(struct intel_dp *intel_dp)
 	intel_dp->mst_detect = DRM_DP_SST;
 }
 
+static void
+intel_dp_mst_disconnect(struct intel_dp *intel_dp)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+	if (!intel_dp->is_mst)
+		return;
+
+	drm_dbg_kms(&i915->drm, "MST device may have disappeared %d vs %d\n",
+		    intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+	intel_dp->is_mst = false;
+	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+}
+
 static bool
 intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *esi)
 {
@@ -5721,15 +5735,7 @@ intel_dp_detect(struct drm_connector *connector,
 		memset(intel_connector->dp.dsc_dpcd, 0, sizeof(intel_connector->dp.dsc_dpcd));
 		intel_dp->psr.sink_panel_replay_support = false;
 
-		if (intel_dp->is_mst) {
-			drm_dbg_kms(&dev_priv->drm,
-				    "MST device may have disappeared %d vs %d\n",
-				    intel_dp->is_mst,
-				    intel_dp->mst_mgr.mst_state);
-			intel_dp->is_mst = false;
-			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
-							intel_dp->is_mst);
-		}
+		intel_dp_mst_disconnect(intel_dp);
 
 		goto out;
 	}
-- 
2.39.2

[PATCH v2 6/6] drm/i915/mst: enable MST mode for 128b/132b single-stream sideband

[Jani Nikula]

If the sink supports 128b/132b and single-stream sideband messaging,
enable MST mode.

With this, the topology manager will still write DP_MSTM_CTRL, which
should be ignored by the sink. In the future, the topology manager
should probably only set the sideband messaging related parts of the
register.

Cc: Arun R Murthy <arun.r.murthy@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7f97d5512a3e..689d5c8ba6b0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4027,7 +4027,8 @@ intel_dp_mst_mode_choose(struct intel_dp *intel_dp,
 	if (!intel_dp_mst_source_support(intel_dp))
 		return DRM_DP_SST;
 
-	if (sink_mst_mode == DRM_DP_SST_SIDEBAND_MSG)
+	if (sink_mst_mode == DRM_DP_SST_SIDEBAND_MSG &&
+	    !(intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B))
 		return DRM_DP_SST;
 
 	return sink_mst_mode;
-- 
2.39.2

RE: [PATCH v2 1/6] drm/mst: read sideband messaging cap

[Murthy, Arun R]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Tuesday, February 13, 2024 5:01 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; Nikula, Jani
> <jani.nikula@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>; Ville
> Syrjälä <ville.syrjala@linux.intel.com>
> Subject: [PATCH v2 1/6] drm/mst: read sideband messaging cap
> 
> Amend drm_dp_read_mst_cap() to return an enum, indicating "SST", "SST with
> sideband messaging", or "MST". Modify all call sites to take the new return
> value into account.
> 
> v2:
> - Rename enumerators (Ville)
> 
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 ++++++++++------
>  drivers/gpu/drm/i915/display/intel_dp.c       |  4 ++--
>  drivers/gpu/drm/nouveau/nouveau_dp.c          |  2 +-
>  include/drm/display/drm_dp_mst_helper.h       | 23 ++++++++++++++++++-
>  4 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 03d528209426..c193be3577f7 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3608,24 +3608,30 @@ fixed20_12 drm_dp_get_vc_payload_bw(const
> struct drm_dp_mst_topology_mgr *mgr,
> EXPORT_SYMBOL(drm_dp_get_vc_payload_bw);
> 
>  /**
> - * drm_dp_read_mst_cap() - check whether or not a sink supports MST
> + * drm_dp_read_mst_cap() - Read the sink's MST mode capability
>   * @aux: The DP AUX channel to use
>   * @dpcd: A cached copy of the DPCD capabilities for this sink
>   *
> - * Returns: %True if the sink supports MST, %false otherwise
> + * Returns: enum drm_dp_mst_mode to indicate MST mode capability
>   */
> -bool drm_dp_read_mst_cap(struct drm_dp_aux *aux,
> -			 const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux,
> +					 const u8
> dpcd[DP_RECEIVER_CAP_SIZE])
>  {
>  	u8 mstm_cap;
> 
>  	if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
> -		return false;
> +		return DRM_DP_SST;
> 
>  	if (drm_dp_dpcd_readb(aux, DP_MSTM_CAP, &mstm_cap) != 1)
> -		return false;
> +		return DRM_DP_SST;
> +
> +	if (mstm_cap & DP_MST_CAP)
> +		return DRM_DP_MST;
> +
> +	if (mstm_cap & DP_SINGLE_STREAM_SIDEBAND_MSG)
> +		return DRM_DP_SST_SIDEBAND_MSG;
Bit[1] of MSTM_CAP indicates sideband messaging is supported or not and nothing to do with MST/SST. So would it make more sense to have it as DRM_DP_SIDEBAND_MSG?

Thanks and Regards,
Arun R Murthy
--------------------
> 
> -	return mstm_cap & DP_MST_CAP;
> +	return DRM_DP_SST;
>  }
>  EXPORT_SYMBOL(drm_dp_read_mst_cap);
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5045c34a16be..a1c304f451bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4014,7 +4014,7 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
> 
>  	return i915->display.params.enable_dp_mst &&
>  		intel_dp_mst_source_support(intel_dp) &&
> -		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
> +		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) ==
> DRM_DP_MST;
>  }
> 
>  static void
> @@ -4023,7 +4023,7 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	struct intel_encoder *encoder =
>  		&dp_to_dig_port(intel_dp)->base;
> -	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp-
> >dpcd);
> +	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux,
> +intel_dp->dpcd) == DRM_DP_MST;
> 
>  	drm_dbg_kms(&i915->drm,
>  		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s,
> modparam: %s\n", diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
> b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 7de7707ec6a8..fb06ee17d9e5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -181,7 +181,7 @@ nouveau_dp_probe_dpcd(struct nouveau_connector
> *nv_connector,
>  	if (nouveau_mst) {
>  		mstm = outp->dp.mstm;
>  		if (mstm)
> -			mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd);
> +			mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd)
> == DRM_DP_MST;
>  	}
> 
>  	if (nouveau_dp_has_sink_count(connector, outp)) { diff --git
> a/include/drm/display/drm_dp_mst_helper.h
> b/include/drm/display/drm_dp_mst_helper.h
> index 9b19d8bd520a..3c9e128c444a 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -818,7 +818,28 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> 
>  void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr
> *mgr);
> 
> -bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8
> dpcd[DP_RECEIVER_CAP_SIZE]);
> +/**
> + * enum drm_dp_mst_mode - sink's MST mode capability  */ enum
> +drm_dp_mst_mode {
> +	/**
> +	 * @DRM_DP_SST: The sink does not support MST nor single stream
> sideband
> +	 * messaging.
> +	 */
> +	DRM_DP_SST,
> +	/**
> +	 * @DRM_DP_MST: Sink supports MST, more than one stream and
> single
> +	 * stream sideband messaging.
> +	 */
> +	DRM_DP_MST,
> +	/**
> +	 * @DRM_DP_SST_SIDEBAND_MSG: Sink supports only one stream and
> single
> +	 * stream sideband messaging.
> +	 */
> +	DRM_DP_SST_SIDEBAND_MSG,
> +};
> +
> +enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux,
> const
> +u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr
> *mgr, bool mst_state);
> 
>  int drm_dp_mst_hpd_irq_handle_event(struct drm_dp_mst_topology_mgr
> *mgr,
> --
> 2.39.2

RE: [PATCH v2 1/6] drm/mst: read sideband messaging cap

[Jani Nikula]

On Tue, 13 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Tuesday, February 13, 2024 5:01 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; Nikula, Jani
>> <jani.nikula@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>; Ville
>> Syrjälä <ville.syrjala@linux.intel.com>
>> Subject: [PATCH v2 1/6] drm/mst: read sideband messaging cap
>>
>> Amend drm_dp_read_mst_cap() to return an enum, indicating "SST", "SST with
>> sideband messaging", or "MST". Modify all call sites to take the new return
>> value into account.
>>
>> v2:
>> - Rename enumerators (Ville)
>>
>> Cc: Arun R Murthy <arun.r.murthy@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 ++++++++++------
>>  drivers/gpu/drm/i915/display/intel_dp.c       |  4 ++--
>>  drivers/gpu/drm/nouveau/nouveau_dp.c          |  2 +-
>>  include/drm/display/drm_dp_mst_helper.h       | 23 ++++++++++++++++++-
>>  4 files changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> index 03d528209426..c193be3577f7 100644
>> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> @@ -3608,24 +3608,30 @@ fixed20_12 drm_dp_get_vc_payload_bw(const
>> struct drm_dp_mst_topology_mgr *mgr,
>> EXPORT_SYMBOL(drm_dp_get_vc_payload_bw);
>>
>>  /**
>> - * drm_dp_read_mst_cap() - check whether or not a sink supports MST
>> + * drm_dp_read_mst_cap() - Read the sink's MST mode capability
>>   * @aux: The DP AUX channel to use
>>   * @dpcd: A cached copy of the DPCD capabilities for this sink
>>   *
>> - * Returns: %True if the sink supports MST, %false otherwise
>> + * Returns: enum drm_dp_mst_mode to indicate MST mode capability
>>   */
>> -bool drm_dp_read_mst_cap(struct drm_dp_aux *aux,
>> -                      const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>> +enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux,
>> +                                      const u8
>> dpcd[DP_RECEIVER_CAP_SIZE])
>>  {
>>       u8 mstm_cap;
>>
>>       if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
>> -             return false;
>> +             return DRM_DP_SST;
>>
>>       if (drm_dp_dpcd_readb(aux, DP_MSTM_CAP, &mstm_cap) != 1)
>> -             return false;
>> +             return DRM_DP_SST;
>> +
>> +     if (mstm_cap & DP_MST_CAP)
>> +             return DRM_DP_MST;
>> +
>> +     if (mstm_cap & DP_SINGLE_STREAM_SIDEBAND_MSG)
>> +             return DRM_DP_SST_SIDEBAND_MSG;
> Bit[1] of MSTM_CAP indicates sideband messaging is supported or not
> and nothing to do with MST/SST. So would it make more sense to have it
> as DRM_DP_SIDEBAND_MSG?

Bit 1 is literally described as SINGLE_STREAM_SIDEBAND_MSG_SUPPORT in
the spec, "Supports Sideband MSG while not supporting multi-stream
transport".

Bit 1 is also only valid when bit 0 says, "Does not support MST mode".


BR,
Jani.


>
> Thanks and Regards,
> Arun R Murthy
> --------------------
>>
>> -     return mstm_cap & DP_MST_CAP;
>> +     return DRM_DP_SST;
>>  }
>>  EXPORT_SYMBOL(drm_dp_read_mst_cap);
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 5045c34a16be..a1c304f451bd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4014,7 +4014,7 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
>>
>>       return i915->display.params.enable_dp_mst &&
>>               intel_dp_mst_source_support(intel_dp) &&
>> -             drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
>> +             drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) ==
>> DRM_DP_MST;
>>  }
>>
>>  static void
>> @@ -4023,7 +4023,7 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>>       struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>       struct intel_encoder *encoder =
>>               &dp_to_dig_port(intel_dp)->base;
>> -     bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp-
>> >dpcd);
>> +     bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux,
>> +intel_dp->dpcd) == DRM_DP_MST;
>>
>>       drm_dbg_kms(&i915->drm,
>>                   "[ENCODER:%d:%s] MST support: port: %s, sink: %s,
>> modparam: %s\n", diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
>> b/drivers/gpu/drm/nouveau/nouveau_dp.c
>> index 7de7707ec6a8..fb06ee17d9e5 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
>> @@ -181,7 +181,7 @@ nouveau_dp_probe_dpcd(struct nouveau_connector
>> *nv_connector,
>>       if (nouveau_mst) {
>>               mstm = outp->dp.mstm;
>>               if (mstm)
>> -                     mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd);
>> +                     mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd)
>> == DRM_DP_MST;
>>       }
>>
>>       if (nouveau_dp_has_sink_count(connector, outp)) { diff --git
>> a/include/drm/display/drm_dp_mst_helper.h
>> b/include/drm/display/drm_dp_mst_helper.h
>> index 9b19d8bd520a..3c9e128c444a 100644
>> --- a/include/drm/display/drm_dp_mst_helper.h
>> +++ b/include/drm/display/drm_dp_mst_helper.h
>> @@ -818,7 +818,28 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>>
>>  void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr
>> *mgr);
>>
>> -bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8
>> dpcd[DP_RECEIVER_CAP_SIZE]);
>> +/**
>> + * enum drm_dp_mst_mode - sink's MST mode capability  */ enum
>> +drm_dp_mst_mode {
>> +     /**
>> +      * @DRM_DP_SST: The sink does not support MST nor single stream
>> sideband
>> +      * messaging.
>> +      */
>> +     DRM_DP_SST,
>> +     /**
>> +      * @DRM_DP_MST: Sink supports MST, more than one stream and
>> single
>> +      * stream sideband messaging.
>> +      */
>> +     DRM_DP_MST,
>> +     /**
>> +      * @DRM_DP_SST_SIDEBAND_MSG: Sink supports only one stream and
>> single
>> +      * stream sideband messaging.
>> +      */
>> +     DRM_DP_SST_SIDEBAND_MSG,
>> +};
>> +
>> +enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux,
>> const
>> +u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>>  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr
>> *mgr, bool mst_state);
>>
>>  int drm_dp_mst_hpd_irq_handle_event(struct drm_dp_mst_topology_mgr
>> *mgr,
>> --
>> 2.39.2
>

-- 
Jani Nikula, Intel

Re: [PATCH v2 1/6] drm/mst: read sideband messaging cap

[Ville Syrjälä]

On Tue, Feb 13, 2024 at 01:30:56PM +0200, Jani Nikula wrote:
> Amend drm_dp_read_mst_cap() to return an enum, indicating "SST", "SST
> with sideband messaging", or "MST". Modify all call sites to take the
> new return value into account.
> 
> v2:
> - Rename enumerators (Ville)
> 
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 ++++++++++------
>  drivers/gpu/drm/i915/display/intel_dp.c       |  4 ++--
>  drivers/gpu/drm/nouveau/nouveau_dp.c          |  2 +-
>  include/drm/display/drm_dp_mst_helper.h       | 23 ++++++++++++++++++-
>  4 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 03d528209426..c193be3577f7 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3608,24 +3608,30 @@ fixed20_12 drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
>  EXPORT_SYMBOL(drm_dp_get_vc_payload_bw);
>  
>  /**
> - * drm_dp_read_mst_cap() - check whether or not a sink supports MST
> + * drm_dp_read_mst_cap() - Read the sink's MST mode capability
>   * @aux: The DP AUX channel to use
>   * @dpcd: A cached copy of the DPCD capabilities for this sink
>   *
> - * Returns: %True if the sink supports MST, %false otherwise
> + * Returns: enum drm_dp_mst_mode to indicate MST mode capability
>   */
> -bool drm_dp_read_mst_cap(struct drm_dp_aux *aux,
> -			 const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux,
> +					 const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>  {
>  	u8 mstm_cap;
>  
>  	if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_12)
> -		return false;
> +		return DRM_DP_SST;
>  
>  	if (drm_dp_dpcd_readb(aux, DP_MSTM_CAP, &mstm_cap) != 1)
> -		return false;
> +		return DRM_DP_SST;
> +
> +	if (mstm_cap & DP_MST_CAP)
> +		return DRM_DP_MST;
> +
> +	if (mstm_cap & DP_SINGLE_STREAM_SIDEBAND_MSG)
> +		return DRM_DP_SST_SIDEBAND_MSG;
>  
> -	return mstm_cap & DP_MST_CAP;
> +	return DRM_DP_SST;
>  }
>  EXPORT_SYMBOL(drm_dp_read_mst_cap);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5045c34a16be..a1c304f451bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4014,7 +4014,7 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
>  
>  	return i915->display.params.enable_dp_mst &&
>  		intel_dp_mst_source_support(intel_dp) &&
> -		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
> +		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
>  }
>  
>  static void
> @@ -4023,7 +4023,7 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	struct intel_encoder *encoder =
>  		&dp_to_dig_port(intel_dp)->base;
> -	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
> +	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
>  
>  	drm_dbg_kms(&i915->drm,
>  		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s\n",
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 7de7707ec6a8..fb06ee17d9e5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -181,7 +181,7 @@ nouveau_dp_probe_dpcd(struct nouveau_connector *nv_connector,
>  	if (nouveau_mst) {
>  		mstm = outp->dp.mstm;
>  		if (mstm)
> -			mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd);
> +			mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd) == DRM_DP_MST;
>  	}
>  
>  	if (nouveau_dp_has_sink_count(connector, outp)) {
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 9b19d8bd520a..3c9e128c444a 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -818,7 +818,28 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>  
>  void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr);
>  
> -bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
> +/**
> + * enum drm_dp_mst_mode - sink's MST mode capability
> + */
> +enum drm_dp_mst_mode {
> +	/**
> +	 * @DRM_DP_SST: The sink does not support MST nor single stream sideband
> +	 * messaging.
> +	 */
> +	DRM_DP_SST,
> +	/**
> +	 * @DRM_DP_MST: Sink supports MST, more than one stream and single
> +	 * stream sideband messaging.
> +	 */
> +	DRM_DP_MST,
> +	/**
> +	 * @DRM_DP_SST_SIDEBAND_MSG: Sink supports only one stream and single
> +	 * stream sideband messaging.
> +	 */
> +	DRM_DP_SST_SIDEBAND_MSG,
> +};
> +
> +enum drm_dp_mst_mode drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state);
>  
>  int drm_dp_mst_hpd_irq_handle_event(struct drm_dp_mst_topology_mgr *mgr,
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 2/6] drm/i915/mst: improve debug logging of DP MST mode detect

[Ville Syrjälä]

On Tue, Feb 13, 2024 at 01:30:57PM +0200, Jani Nikula wrote:
> Rename intel_dp_can_mst() to intel_dp_mst_detect(), and move all DP MST
> detect debug logging there. Debug log the sink's MST capability,
> including single-stream sideband messaging support, and the decision
> whether to enable MST mode or not. Do this regardless of whether we're
> actually enabling MST or not.
> 
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 45 +++++++++++++++++--------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index a1c304f451bd..944f566525dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4007,31 +4007,48 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  					   intel_dp->downstream_ports) == 0;
>  }
>  
> +static const char *intel_dp_mst_mode_str(enum drm_dp_mst_mode mst_mode)
> +{
> +	if (mst_mode == DRM_DP_SST_SIDEBAND_MSG)
> +		return "single-stream sideband messaging";
> +	else
> +		return str_yes_no(mst_mode == DRM_DP_MST);

I wonder if this should also just say "sst"/"mst"/"sst sideband" etc.
Shrug.

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

> +}
> +
>  static bool
> -intel_dp_can_mst(struct intel_dp *intel_dp)
> +intel_dp_mst_detect(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	enum drm_dp_mst_mode sink_mst_mode;
> +	enum drm_dp_mst_mode mst_detect;
> +
> +	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
> +
> +	if (i915->display.params.enable_dp_mst &&
> +	    intel_dp_mst_source_support(intel_dp) &&
> +	    sink_mst_mode == DRM_DP_MST)
> +		mst_detect = DRM_DP_MST;
> +	else
> +		mst_detect = DRM_DP_SST;
>  
> -	return i915->display.params.enable_dp_mst &&
> -		intel_dp_mst_source_support(intel_dp) &&
> -		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
> +	drm_dbg_kms(&i915->drm,
> +		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
> +		    encoder->base.base.id, encoder->base.name,
> +		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
> +		    intel_dp_mst_mode_str(sink_mst_mode),
> +		    str_yes_no(i915->display.params.enable_dp_mst),
> +		    intel_dp_mst_mode_str(mst_detect));
> +
> +	return mst_detect != DRM_DP_SST;
>  }
>  
>  static void
>  intel_dp_configure_mst(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	struct intel_encoder *encoder =
> -		&dp_to_dig_port(intel_dp)->base;
>  	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
>  
> -	drm_dbg_kms(&i915->drm,
> -		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s\n",
> -		    encoder->base.base.id, encoder->base.name,
> -		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
> -		    str_yes_no(sink_can_mst),
> -		    str_yes_no(i915->display.params.enable_dp_mst));
> -
>  	if (!intel_dp_mst_source_support(intel_dp))
>  		return;
>  
> @@ -5390,7 +5407,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  		connector_status_connected : connector_status_disconnected;
>  	}
>  
> -	if (intel_dp_can_mst(intel_dp))
> +	if (intel_dp_mst_detect(intel_dp))
>  		return connector_status_connected;
>  
>  	/* If no HPD, poke DDC gently */
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 3/6] drm/i915/mst: abstract choosing the MST mode to use

[Ville Syrjälä]

On Tue, Feb 13, 2024 at 01:30:58PM +0200, Jani Nikula wrote:
> Clarify the conditions for choosing the MST mode to use by adding a new
> function intel_dp_mst_mode_choose(). This also prepares for being able
> to extend the MST modes to single-stream sideband messaging.
> 
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 944f566525dd..007cb2a04e38 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4015,6 +4015,24 @@ static const char *intel_dp_mst_mode_str(enum drm_dp_mst_mode mst_mode)
>  		return str_yes_no(mst_mode == DRM_DP_MST);
>  }
>  
> +static enum drm_dp_mst_mode
> +intel_dp_mst_mode_choose(struct intel_dp *intel_dp,
> +			 enum drm_dp_mst_mode sink_mst_mode)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (!i915->display.params.enable_dp_mst)
> +		return DRM_DP_SST;
> +
> +	if (!intel_dp_mst_source_support(intel_dp))
> +		return DRM_DP_SST;
> +
> +	if (sink_mst_mode == DRM_DP_SST_SIDEBAND_MSG)
> +		return DRM_DP_SST;
> +
> +	return sink_mst_mode;
> +}
> +
>  static bool
>  intel_dp_mst_detect(struct intel_dp *intel_dp)
>  {
> @@ -4025,12 +4043,7 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
>  
>  	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
>  
> -	if (i915->display.params.enable_dp_mst &&
> -	    intel_dp_mst_source_support(intel_dp) &&
> -	    sink_mst_mode == DRM_DP_MST)
> -		mst_detect = DRM_DP_MST;
> -	else
> -		mst_detect = DRM_DP_SST;
> +	mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
>  
>  	drm_dbg_kms(&i915->drm,
>  		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 4/6] drm/i915/mst: use the MST mode detected previously

[Ville Syrjälä]

On Tue, Feb 13, 2024 at 01:30:59PM +0200, Jani Nikula wrote:
> Drop the duplicate read of DP_MSTM_CAP DPCD register, and the duplicate
> logic for choosing MST mode, and store the chosen mode in struct
> intel_dp. Rename intel_dp_configure_mst() to intel_dp_mst_configure()
> while at it.
> 
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  1 +
>  drivers/gpu/drm/i915/display/intel_dp.c       | 23 ++++++++-----------
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 01eb6e4e6049..4a8440a3a812 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1780,6 +1780,7 @@ struct intel_dp {
>  
>  	bool is_mst;
>  	int active_mst_links;
> +	enum drm_dp_mst_mode mst_detect;
>  
>  	/* connector directly attached - won't be use for modeset in mst world */
>  	struct intel_connector *attached_connector;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 007cb2a04e38..72e91322e310 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4039,11 +4039,10 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	enum drm_dp_mst_mode sink_mst_mode;
> -	enum drm_dp_mst_mode mst_detect;
>  
>  	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
>  
> -	mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
> +	intel_dp->mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
>  
>  	drm_dbg_kms(&i915->drm,
>  		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
> @@ -4051,25 +4050,23 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
>  		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
>  		    intel_dp_mst_mode_str(sink_mst_mode),
>  		    str_yes_no(i915->display.params.enable_dp_mst),
> -		    intel_dp_mst_mode_str(mst_detect));
> +		    intel_dp_mst_mode_str(intel_dp->mst_detect));
>  
> -	return mst_detect != DRM_DP_SST;
> +	return intel_dp->mst_detect != DRM_DP_SST;
>  }
>  
>  static void
> -intel_dp_configure_mst(struct intel_dp *intel_dp)
> +intel_dp_mst_configure(struct intel_dp *intel_dp)
>  {
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
> -
>  	if (!intel_dp_mst_source_support(intel_dp))
>  		return;

I was wondering if we even need that, but it looks to be just a
check to see if we actually initialized the mst_mgt or not.
We should probably rename it or something... Or perhaps we could
tweak the topology manager a bit so we wouldn't need to check...

>  
> -	intel_dp->is_mst = sink_can_mst &&
> -		i915->display.params.enable_dp_mst;
> +	intel_dp->is_mst = intel_dp->mst_detect != DRM_DP_SST;
> +
> +	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>  
> -	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -					intel_dp->is_mst);
> +	/* Avoid stale info on the next detect cycle. */
> +	intel_dp->mst_detect = DRM_DP_SST;

Hmm. Not sure I like having ephemeral stuff like this in intel_dp,
but I guess the alternative would be plumb it up from detect_dpcd()
by hand, which might not be super pretty either. Oh well.

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

>  }
>  
>  static bool
> @@ -5739,7 +5736,7 @@ intel_dp_detect(struct drm_connector *connector,
>  
>  	intel_dp_detect_dsc_caps(intel_dp, intel_connector);
>  
> -	intel_dp_configure_mst(intel_dp);
> +	intel_dp_mst_configure(intel_dp);
>  
>  	/*
>  	 * TODO: Reset link params when switching to MST mode, until MST
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 5/6] drm/i915/mst: add intel_dp_mst_disconnect()

[Ville Syrjälä]

On Tue, Feb 13, 2024 at 01:31:00PM +0200, Jani Nikula wrote:
> Abstract the MST mode disconnect to a separate function.
> 
> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 72e91322e310..7f97d5512a3e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4069,6 +4069,20 @@ intel_dp_mst_configure(struct intel_dp *intel_dp)
>  	intel_dp->mst_detect = DRM_DP_SST;
>  }
>  
> +static void
> +intel_dp_mst_disconnect(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (!intel_dp->is_mst)
> +		return;
> +
> +	drm_dbg_kms(&i915->drm, "MST device may have disappeared %d vs %d\n",
> +		    intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> +	intel_dp->is_mst = false;
> +	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> +}
> +
>  static bool
>  intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *esi)
>  {
> @@ -5721,15 +5735,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		memset(intel_connector->dp.dsc_dpcd, 0, sizeof(intel_connector->dp.dsc_dpcd));
>  		intel_dp->psr.sink_panel_replay_support = false;
>  
> -		if (intel_dp->is_mst) {
> -			drm_dbg_kms(&dev_priv->drm,
> -				    "MST device may have disappeared %d vs %d\n",
> -				    intel_dp->is_mst,
> -				    intel_dp->mst_mgr.mst_state);
> -			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -							intel_dp->is_mst);
> -		}
> +		intel_dp_mst_disconnect(intel_dp);
>  
>  		goto out;
>  	}
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 4/6] drm/i915/mst: use the MST mode detected previously

[Jani Nikula]

On Wed, 14 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Feb 13, 2024 at 01:30:59PM +0200, Jani Nikula wrote:
>> Drop the duplicate read of DP_MSTM_CAP DPCD register, and the duplicate
>> logic for choosing MST mode, and store the chosen mode in struct
>> intel_dp. Rename intel_dp_configure_mst() to intel_dp_mst_configure()
>> while at it.
>> 
>> Cc: Arun R Murthy <arun.r.murthy@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  .../drm/i915/display/intel_display_types.h    |  1 +
>>  drivers/gpu/drm/i915/display/intel_dp.c       | 23 ++++++++-----------
>>  2 files changed, 11 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 01eb6e4e6049..4a8440a3a812 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1780,6 +1780,7 @@ struct intel_dp {
>>  
>>  	bool is_mst;
>>  	int active_mst_links;
>> +	enum drm_dp_mst_mode mst_detect;
>>  
>>  	/* connector directly attached - won't be use for modeset in mst world */
>>  	struct intel_connector *attached_connector;
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 007cb2a04e38..72e91322e310 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4039,11 +4039,10 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
>>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>>  	enum drm_dp_mst_mode sink_mst_mode;
>> -	enum drm_dp_mst_mode mst_detect;
>>  
>>  	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
>>  
>> -	mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
>> +	intel_dp->mst_detect = intel_dp_mst_mode_choose(intel_dp, sink_mst_mode);
>>  
>>  	drm_dbg_kms(&i915->drm,
>>  		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
>> @@ -4051,25 +4050,23 @@ intel_dp_mst_detect(struct intel_dp *intel_dp)
>>  		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
>>  		    intel_dp_mst_mode_str(sink_mst_mode),
>>  		    str_yes_no(i915->display.params.enable_dp_mst),
>> -		    intel_dp_mst_mode_str(mst_detect));
>> +		    intel_dp_mst_mode_str(intel_dp->mst_detect));
>>  
>> -	return mst_detect != DRM_DP_SST;
>> +	return intel_dp->mst_detect != DRM_DP_SST;
>>  }
>>  
>>  static void
>> -intel_dp_configure_mst(struct intel_dp *intel_dp)
>> +intel_dp_mst_configure(struct intel_dp *intel_dp)
>>  {
>> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> -	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
>> -
>>  	if (!intel_dp_mst_source_support(intel_dp))
>>  		return;
>
> I was wondering if we even need that, but it looks to be just a
> check to see if we actually initialized the mst_mgt or not.
> We should probably rename it or something... Or perhaps we could
> tweak the topology manager a bit so we wouldn't need to check...

Yeah, I figured we should address this in
follow-up. intel_dp_mst_suspend() and intel_dp_mst_resume() would
benefit too.

>
>>  
>> -	intel_dp->is_mst = sink_can_mst &&
>> -		i915->display.params.enable_dp_mst;
>> +	intel_dp->is_mst = intel_dp->mst_detect != DRM_DP_SST;
>> +
>> +	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>>  
>> -	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
>> -					intel_dp->is_mst);
>> +	/* Avoid stale info on the next detect cycle. */
>> +	intel_dp->mst_detect = DRM_DP_SST;
>
> Hmm. Not sure I like having ephemeral stuff like this in intel_dp,
> but I guess the alternative would be plumb it up from detect_dpcd()
> by hand, which might not be super pretty either. Oh well.

Trust me, I went back and forth with this, and this felt least ugly. The
main reason for resetting it back here was to ensure nobody tries to use
it for anything else, and to ensure it gets reset.

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

Thanks!

>
>>  }
>>  
>>  static bool
>> @@ -5739,7 +5736,7 @@ intel_dp_detect(struct drm_connector *connector,
>>  
>>  	intel_dp_detect_dsc_caps(intel_dp, intel_connector);
>>  
>> -	intel_dp_configure_mst(intel_dp);
>> +	intel_dp_mst_configure(intel_dp);
>>  
>>  	/*
>>  	 * TODO: Reset link params when switching to MST mode, until MST
>> -- 
>> 2.39.2

-- 
Jani Nikula, Intel

Re: [PATCH v2 2/6] drm/i915/mst: improve debug logging of DP MST mode detect

[Jani Nikula]

On Wed, 14 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Feb 13, 2024 at 01:30:57PM +0200, Jani Nikula wrote:
>> Rename intel_dp_can_mst() to intel_dp_mst_detect(), and move all DP MST
>> detect debug logging there. Debug log the sink's MST capability,
>> including single-stream sideband messaging support, and the decision
>> whether to enable MST mode or not. Do this regardless of whether we're
>> actually enabling MST or not.
>> 
>> Cc: Arun R Murthy <arun.r.murthy@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c | 45 +++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index a1c304f451bd..944f566525dd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4007,31 +4007,48 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>  					   intel_dp->downstream_ports) == 0;
>>  }
>>  
>> +static const char *intel_dp_mst_mode_str(enum drm_dp_mst_mode mst_mode)
>> +{
>> +	if (mst_mode == DRM_DP_SST_SIDEBAND_MSG)
>> +		return "single-stream sideband messaging";
>> +	else
>> +		return str_yes_no(mst_mode == DRM_DP_MST);
>
> I wonder if this should also just say "sst"/"mst"/"sst sideband" etc.
> Shrug.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I realize there's an issue here.

intel_dp_detect_dpcd() bails out early for !drm_dp_is_branch(), before
the intel_dp_can_mst() call. (Renamed intel_dp_mst_detect() here.)

We'll still happily call intel_dp_configure_mst() later also for
!branch.

We'll need to call intel_dp_mst_detect() earlier and/or somehow combine
these together. I don't think branch devices need to support MST, but I
think MST devices need to support branching. And single-stream sideband
support does not mean branching.

The intention of this patch was to improve MST debug logging, but the
end result is that it reduces it! Auch.

I wonder if we should branch (eh) the detect earlier for eDP, SST and
MST/branch paths. Just to make it easier for our poor brains to follow.


BR,
Jani.


>
>> +}
>> +
>>  static bool
>> -intel_dp_can_mst(struct intel_dp *intel_dp)
>> +intel_dp_mst_detect(struct intel_dp *intel_dp)
>>  {
>>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>> +	enum drm_dp_mst_mode sink_mst_mode;
>> +	enum drm_dp_mst_mode mst_detect;
>> +
>> +	sink_mst_mode = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
>> +
>> +	if (i915->display.params.enable_dp_mst &&
>> +	    intel_dp_mst_source_support(intel_dp) &&
>> +	    sink_mst_mode == DRM_DP_MST)
>> +		mst_detect = DRM_DP_MST;
>> +	else
>> +		mst_detect = DRM_DP_SST;
>>  
>> -	return i915->display.params.enable_dp_mst &&
>> -		intel_dp_mst_source_support(intel_dp) &&
>> -		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
>> +	drm_dbg_kms(&i915->drm,
>> +		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s -> enable: %s\n",
>> +		    encoder->base.base.id, encoder->base.name,
>> +		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
>> +		    intel_dp_mst_mode_str(sink_mst_mode),
>> +		    str_yes_no(i915->display.params.enable_dp_mst),
>> +		    intel_dp_mst_mode_str(mst_detect));
>> +
>> +	return mst_detect != DRM_DP_SST;
>>  }
>>  
>>  static void
>>  intel_dp_configure_mst(struct intel_dp *intel_dp)
>>  {
>>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> -	struct intel_encoder *encoder =
>> -		&dp_to_dig_port(intel_dp)->base;
>>  	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DRM_DP_MST;
>>  
>> -	drm_dbg_kms(&i915->drm,
>> -		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s\n",
>> -		    encoder->base.base.id, encoder->base.name,
>> -		    str_yes_no(intel_dp_mst_source_support(intel_dp)),
>> -		    str_yes_no(sink_can_mst),
>> -		    str_yes_no(i915->display.params.enable_dp_mst));
>> -
>>  	if (!intel_dp_mst_source_support(intel_dp))
>>  		return;
>>  
>> @@ -5390,7 +5407,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>  		connector_status_connected : connector_status_disconnected;
>>  	}
>>  
>> -	if (intel_dp_can_mst(intel_dp))
>> +	if (intel_dp_mst_detect(intel_dp))
>>  		return connector_status_connected;
>>  
>>  	/* If no HPD, poke DDC gently */
>> -- 
>> 2.39.2

-- 
Jani Nikula, Intel

Re: [PATCH v2 2/6] drm/i915/mst: improve debug logging of DP MST mode detect

[Ville Syrjälä]

On Thu, Feb 15, 2024 at 01:46:48PM +0200, Jani Nikula wrote:
> On Wed, 14 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Feb 13, 2024 at 01:30:57PM +0200, Jani Nikula wrote:
> >> Rename intel_dp_can_mst() to intel_dp_mst_detect(), and move all DP MST
> >> detect debug logging there. Debug log the sink's MST capability,
> >> including single-stream sideband messaging support, and the decision
> >> whether to enable MST mode or not. Do this regardless of whether we're
> >> actually enabling MST or not.
> >> 
> >> Cc: Arun R Murthy <arun.r.murthy@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_dp.c | 45 +++++++++++++++++--------
> >>  1 file changed, 31 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index a1c304f451bd..944f566525dd 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -4007,31 +4007,48 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >>  					   intel_dp->downstream_ports) == 0;
> >>  }
> >>  
> >> +static const char *intel_dp_mst_mode_str(enum drm_dp_mst_mode mst_mode)
> >> +{
> >> +	if (mst_mode == DRM_DP_SST_SIDEBAND_MSG)
> >> +		return "single-stream sideband messaging";
> >> +	else
> >> +		return str_yes_no(mst_mode == DRM_DP_MST);
> >
> > I wonder if this should also just say "sst"/"mst"/"sst sideband" etc.
> > Shrug.
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I realize there's an issue here.
> 
> intel_dp_detect_dpcd() bails out early for !drm_dp_is_branch(), before
> the intel_dp_can_mst() call. (Renamed intel_dp_mst_detect() here.)
> 
> We'll still happily call intel_dp_configure_mst() later also for
> !branch.
> 
> We'll need to call intel_dp_mst_detect() earlier and/or somehow combine
> these together. I don't think branch devices need to support MST, but I
> think MST devices need to support branching. And single-stream sideband
> support does not mean branching.
> 
> The intention of this patch was to improve MST debug logging, but the
> end result is that it reduces it! Auch.
> 
> I wonder if we should branch (eh) the detect earlier for eDP, SST and
> MST/branch paths. Just to make it easier for our poor brains to follow.

Hmm. The sink count check is another case as well. If the device
has a local sink, or somehting connected to its DFP(s) it should
declare sink count >= 1.

-- 
Ville Syrjälä
Intel