Re: [RFC 2/4] drm/i915/dp: refactor DP MST detection and configuration

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Feb 12, 2024 at 05:25:43PM +0200, Jani Nikula wrote:
> On Fri, 02 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 02, 2024 at 04:05:32PM +0200, Jani Nikula wrote:
> >> Currently we've split MST capability detection in two places,
> >> intel_dp_can_mst() and intel_dp_configure_mst(). They check essentially
> >> the same things.
> >> 
> >> Move bulk of the work, including logging, to intel_dp_can_mst() and
> >> rename it intel_dp_mst_detect(). Set intel_dp->is_mst there to avoid
> >> duplicate work.
> >
> > This seems confusing. is_mst is supposed to reflect the state
> > of the topology manager, nothing more.
> 
> We'll, that's still exactly what we're going to set the topology manager
> state to. It's just that now we figure it out just a little earlier, so
> we don't have to duplicate the dpcd reads and logic to two places.

That may be desiresable, but I don't think the current is_mst
flag is really the correct thing to track that. We now set it
somewhere deep down in the detect path wihtout configuring
the mst manager to match, but we clear it much higher up
and there we do assume it 100% matches the state of the topology
manager.

Eg. what happens now if we were previosuly using MST, and now
intel_dp_mst_detect() clears the flag? It looks to me like
we're going to completely forget to call
drm_dp_mst_topology_mgr_set_mst(...,false).

> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> Rename intel_dp_configure_mst() to intel_dp_mst_configure(), and only
> >> set the topology manager state there.
> >> 
> >> The main functional difference is that the DP_MSTM_CAP DPCD register is
> >> now only read once at detect, unconditionally, and the MST support is
> >> always logged. Everything else should remain the same.
> >> 
> >> 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 | 36 ++++++++++---------------
> >>  1 file changed, 14 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index 7af09f2c008d..e0b8ee6bde79 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -4004,23 +4004,15 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >>  					   intel_dp->downstream_ports) == 0;
> >>  }
> >>  
> >> -static bool
> >> -intel_dp_can_mst(struct intel_dp *intel_dp)
> >> +static bool 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;
> >> +	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DP_MST_CAPABLE;
> >>  
> >> -	return i915->display.params.enable_dp_mst &&
> >> +	intel_dp->is_mst = i915->display.params.enable_dp_mst &&
> >>  		intel_dp_mst_source_support(intel_dp) &&
> >> -		drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd) == DP_MST_CAPABLE;
> >> -}
> >> -
> >> -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) == DP_MST_CAPABLE;
> >> +		sink_can_mst;
> >>  
> >>  	drm_dbg_kms(&i915->drm,
> >>  		    "[ENCODER:%d:%s] MST support: port: %s, sink: %s, modparam: %s\n",
> >> @@ -4029,14 +4021,14 @@ intel_dp_configure_mst(struct intel_dp *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;
> >> -
> >> -	intel_dp->is_mst = sink_can_mst &&
> >> -		i915->display.params.enable_dp_mst;
> >> +	return intel_dp->is_mst;
> >> +}
> >>  
> >> -	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> >> -					intel_dp->is_mst);
> >> +static void intel_dp_mst_configure(struct intel_dp *intel_dp)
> >> +{
> >> +	if (intel_dp_mst_source_support(intel_dp))
> >> +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> >> +						intel_dp->is_mst);
> >>  }
> >>  
> >>  static bool
> >> @@ -5387,7 +5379,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 */
> >> @@ -5706,7 +5698,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

-- 
Ville Syrjälä
Intel