Re: [PATCH v2 13/16] drm/i915/ddi: start distinguishing 128b/132b SST and MST at state readout

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

On Thu, 02 Jan 2025, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Dec 19, 2024 at 11:34:02PM +0200, Jani Nikula wrote:
>> We'll want to distinguish 128b/132b SST and MST modes at state
>> readout. There's a catch, though. From the hardware perspective,
>> 128b/132b SST and MST programming are pretty much the same. And we can't
>> really ask the sink at this point.
>> 
>> If we have more than one transcoder in 128b/132b mode associated with
>> the port, we can safely assume it's MST. But for MST with only a single
>> stream enabled, we are pretty much out of luck. Let's fall back to
>> looking at the software state, i.e. intel_dp->is_mst. It should be fine
>> for the state checker, but for hardware takeover at probe, we'll have to
>> trust the GOP has only enabled SST.
>> 
>> TODO: Not sure how this *or* our current code handles 128b/132b enabled
>> by GOP.
>> 
>> Cc: Imre Deak <imre.deak@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_ddi.c | 29 +++++++++++++++++++-----
>>  1 file changed, 23 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 7b739b9c5a06..04118f2eea94 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -786,7 +786,7 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  	intel_wakeref_t wakeref;
>>  	enum pipe p;
>>  	u32 tmp;
>> -	u8 mst_pipe_mask;
>> +	u8 mst_pipe_mask = 0, dp128b132b_pipe_mask = 0;
>>  
>>  	*pipe_mask = 0;
>>  	*is_dp_mst = false;
>> @@ -823,7 +823,6 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  		goto out;
>>  	}
>>  
>> -	mst_pipe_mask = 0;
>>  	for_each_pipe(dev_priv, p) {
>>  		enum transcoder cpu_transcoder = (enum transcoder)p;
>>  		u32 port_mask, ddi_select, ddi_mode;
>> @@ -852,9 +851,10 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  
>>  		ddi_mode = tmp & TRANS_DDI_MODE_SELECT_MASK;
>>  
>> -		if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST ||
>> -		    (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display)))
>> +		if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST)
>>  			mst_pipe_mask |= BIT(p);
>> +		else if (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display))
>> +			dp128b132b_pipe_mask |= BIT(p);
>>  
>>  		*pipe_mask |= BIT(p);
>>  	}
>> @@ -864,6 +864,23 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  			    "No pipe for [ENCODER:%d:%s] found\n",
>>  			    encoder->base.base.id, encoder->base.name);
>>  
>> +	if (!mst_pipe_mask && dp128b132b_pipe_mask) {
>> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> +
>
> 8b10b and 128b132b can't be mixed on one link, so the above could make
> this clear (and more robust) by
>
> 	if (dp128b132b_pipe_mask) {
> 		if (WARN(mst_pipe_mask))
> 			mst_pipe_mask = 0;

They can't be mixed, but doesn't mean the registers can't contain that
stuff!

The point is any goof-ups like that get caught in one place below...

> In any case the patch is correct, so either way:
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
>> +		/*
>> +		 * If we don't have 8b/10b MST, but have more than one
>> +		 * transcoder in 128b/132b mode, we know it must be 128b/132b
>> +		 * MST.
>> +		 *
>> +		 * Otherwise, we fall back to checking the current MST
>> +		 * state. It's not accurate for hardware takeover at probe, but
>> +		 * we don't expect MST to have been enabled at that point, and
>> +		 * can assume it's SST.
>> +		 */
>> +		if (hweight8(dp128b132b_pipe_mask) > 1 || intel_dp->is_mst)
>> +			mst_pipe_mask = dp128b132b_pipe_mask;
>> +	}
>> +
>>  	if (!mst_pipe_mask && hweight8(*pipe_mask) > 1) {
>>  		drm_dbg_kms(&dev_priv->drm,
>>  			    "Multiple pipes for [ENCODER:%d:%s] (pipe_mask %02x)\n",
>> @@ -874,9 +891,9 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  
>>  	if (mst_pipe_mask && mst_pipe_mask != *pipe_mask)

...here. If both mst_pipe_mask != 0 and dp128b132b_pipe_mask != 0, then
mst_pipe_mask != *pipe_mask.

BR,
Jani.

>>  		drm_dbg_kms(&dev_priv->drm,
>> -			    "Conflicting MST and non-MST state for [ENCODER:%d:%s] (pipe_mask %02x mst_pipe_mask %02x)\n",
>> +			    "Conflicting MST and non-MST state for [ENCODER:%d:%s] (pipe masks: all %02x, MST %02x, 128b/132b %02x)\n",
>>  			    encoder->base.base.id, encoder->base.name,
>> -			    *pipe_mask, mst_pipe_mask);
>> +			    *pipe_mask, mst_pipe_mask, dp128b132b_pipe_mask);
>>  	else
>>  		*is_dp_mst = mst_pipe_mask;
>>  
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel