Re: [PATCH] drm/i915/dp_mst: Fix encoder HW state readout for UHBR MST

[Imre Deak <imre.deak@intel.com>]

On Mon, Feb 24, 2025 at 12:34:36PM +0200, Jani Nikula wrote:
> On Mon, 24 Feb 2025, Imre Deak <imre.deak@intel.com> wrote:
> > The encoder HW/SW state verification should use a SW state which stays
> > unchanged while the encoder/output is active. The intel_dp::is_mst flag
> > used during state computation to choose between the DP SST/MST modes can
> > change while the output is active, if the sink gets disconnected or the
> > MST topology is removed for another reason. A subsequent state
> > verification using intel_dp::is_mst leads then to a mismatch if the
> > output is disabled/re-enabled without recomputing its state.
> >
> > Use the encoder's active MST link count instead, which will be always
> > non-zero for an active MST output and will be zero for SST.
> >
> > Fixes: 35d2e4b75649 ("drm/i915/ddi: start distinguishing 128b/132b SST and MST at state readout")
> > Fixes: 40d489fac0e8 ("drm/i915/ddi: handle 128b/132b SST in intel_ddi_read_func_ctl()")
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Do we have a bug for this?

I haven't found a bug filed for it.

> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 7937f4de66cb4..8c8b53414da67 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -890,7 +890,7 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
> >  			    encoder->base.base.id, encoder->base.name);
> >  
> >  	if (!mst_pipe_mask && dp128b132b_pipe_mask) {
> > -		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +		struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >  
> >  		/*
> >  		 * If we don't have 8b/10b MST, but have more than one
> > @@ -902,7 +902,8 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
> >  		 * 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)
> > +		if (hweight8(dp128b132b_pipe_mask) > 1 ||
> > +		    intel_dp_mst_encoder_active_links(dig_port))
> 
> I think the reasoning here is valid, but I think we should probably do
> something about the intel_dp_mst_encoder_active_links() naming as
> follow-up. This is confusing. What is an "mst encoder"? The above
> expects the primary digital port, i.e. it assumes enc_to_dig_port()
> already did the right thing. It's all really subtle. Also wrt which
> encoders the intel_ddi_get_encoder_pipes() gets called on.

Only a primary encoder object has an embedded digital port
(intel_digital_port), so the only way to call the function is passing
the primary digital port to it (IOW there is no fake/stream encoder
digital port to pass to it). IMO referring to the encoder or the encoder
type isn't necessary and the function could be renamed to

intel_dp_mst_active_streams()

also clarifying that it's about streams not links?

> Maybe we need s/intel_dp_mst_encoder/intel_dp_mst_primary/ for this and
> some other functions? They're not about the "fake" mst stream encoders.
> 
> Anyway,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >  			mst_pipe_mask = dp128b132b_pipe_mask;
> >  	}
> >  
> > @@ -4131,13 +4132,13 @@ static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
> >  	} else if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST) {
> >  		intel_ddi_read_func_ctl_dp_mst(encoder, pipe_config, ddi_func_ctl);
> >  	} else if (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display)) {
> > -		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +		struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >  
> >  		/*
> >  		 * If this is true, we know we're being called from mst stream
> >  		 * encoder's ->get_config().
> >  		 */
> > -		if (intel_dp->is_mst)
> > +		if (intel_dp_mst_encoder_active_links(dig_port))
> >  			intel_ddi_read_func_ctl_dp_mst(encoder, pipe_config, ddi_func_ctl);
> >  		else
> >  			intel_ddi_read_func_ctl_dp_sst(encoder, pipe_config, ddi_func_ctl);
> 
> -- 
> Jani Nikula, Intel