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

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

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