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

[Jani Nikula <jani.nikula@intel.com>]

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.

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