Re: [PATCH 2/2] drm/i915/dp: Fix disabling the transcoder function in 128b/132b mode

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

On Tue, Feb 18, 2025 at 03:03:35PM +0200, Jani Nikula wrote:
> On Tue, 18 Feb 2025, Imre Deak <imre.deak@intel.com> wrote:
> > During disabling the transcoder in DP 128b/132b mode (both in case of an
> > MST master transcoder and in case of SST) the transcoder function must
> > be first disabled without changing any other field in the register (in
> > particular leaving the DDI port and mode select fields unchanged) and
> > clearing the DDI port and mode select fields separately, later during
> > the disabling sequences. Fix the sequence accordingly.
> >
> > Bspec: 54128, 65448, 68849
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Fixes: 79a6734cd56e ("drm/i915/ddi: disable trancoder port select for 128b/132b SST")
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> Looks like I've intentionally done it this way. I think I've stumbled on
> the bspec text "DP v2.0/128b Primary" and the "primary" has convinced me
> this means MST. In most cases one should just read all things MST as
> being true for MTP, regardless of 8b/10b or 128b/132b, no matter what
> the text actually says. :p

Right, I also understood the spec the same way, i.e. that the 128b/132b
MST and SST modeset sequences would be different. It's clearer now that
this can't be the case, as far as the HW is concerned there is no
difference. The only difference is the extra side-band messaging for the
MST case, which the HW doesn't "care" about, since it's not aware of
anything about those SB messages.

Btw, I'm guessing intel_dp_mst_is_master_trans() could be renamed
accordingly, intel_dp_is_mst_master_or_uhbr_trans(), or something (and
crtc_state->mst_master_transcoder accordingly).

> Thanks for debugging and fixing these!
> 
> BR,
> Jani.
> 
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 5082f38b0a02e..7937f4de66cb4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -681,7 +681,6 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > -	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> >  	u32 ctl;
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > @@ -701,8 +700,7 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state
> >  			 TRANS_DDI_PORT_SYNC_MASTER_SELECT_MASK);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 12) {
> > -		if (!intel_dp_mst_is_master_trans(crtc_state) ||
> > -		    (!is_mst && intel_dp_is_uhbr(crtc_state))) {
> > +		if (!intel_dp_mst_is_master_trans(crtc_state)) {
> >  			ctl &= ~(TGL_TRANS_DDI_PORT_MASK |
> >  				 TRANS_DDI_MODE_SELECT_MASK);
> >  		}
> > @@ -3138,7 +3136,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
> >  	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 12) {
> > -		if (is_mst) {
> > +		if (is_mst || intel_dp_is_uhbr(old_crtc_state)) {
> >  			enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> >  
> >  			intel_de_rmw(dev_priv,
> 
> -- 
> Jani Nikula, Intel