[Intel-gfx] [PATCH 1/7] drm/i915/display: Refactor intel_commit_modeset_disables()

[Souza, Jose]

On Tue, 2019-11-26 at 21:40 +0200, Ville Syrjälä wrote:
> On Fri, Nov 22, 2019 at 04:54:53PM -0800, José Roberto de Souza
> wrote:
> > Commit 9c722e17c1b9 ("drm/i915: Disable pipes in reverse order")
> > reverted the order that pipes gets disabled because of TGL
> > master/slave relationship between transcoders in MST mode.
> > 
> > But as stated in a comment in skl_commit_modeset_enables() the
> > enabling order is not always crescent, possibly causing previously
> > selected slave transcoder being enabled before master so another
> > approach will be needed to select a transcoder to master in MST
> > mode.
> > It will be similar to the approach taken in port sync.
> > 
> > But instead of implement something like
> > intel_trans_port_sync_modeset_disables() to MST lets simply it and
> > iterate over all pipes 2 times, the first one disabling any slave
> > and
> > then disabling everything else.
> > The MST bits will be added in another patch.
> > 
> > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 79 ++++++--------
> > ------
> >  1 file changed, 22 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 53dc310a5f6d..1b1fbb6d8acc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14443,53 +14443,16 @@ static void
> > intel_old_crtc_state_disables(struct intel_atomic_state *state,
> >  		dev_priv->display.initial_watermarks(state, crtc);
> >  }
> >  
> > -static void intel_trans_port_sync_modeset_disables(struct
> > intel_atomic_state *state,
> > -						   struct intel_crtc
> > *crtc,
> > -						   struct
> > intel_crtc_state *old_crtc_state,
> > -						   struct
> > intel_crtc_state *new_crtc_state)
> > -{
> > -	struct intel_crtc *slave_crtc =
> > intel_get_slave_crtc(new_crtc_state);
> > -	struct intel_crtc_state *new_slave_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, slave_crtc);
> > -	struct intel_crtc_state *old_slave_crtc_state =
> > -		intel_atomic_get_old_crtc_state(state, slave_crtc);
> > -
> > -	WARN_ON(!slave_crtc || !new_slave_crtc_state ||
> > -		!old_slave_crtc_state);
> > -
> > -	/* Disable Slave first */
> > -	intel_pre_plane_update(old_slave_crtc_state,
> > new_slave_crtc_state);
> > -	if (old_slave_crtc_state->hw.active)
> > -		intel_old_crtc_state_disables(state,
> > -					      old_slave_crtc_state,
> > -					      new_slave_crtc_state,
> > -					      slave_crtc);
> > -
> > -	/* Disable Master */
> > -	intel_pre_plane_update(old_crtc_state, new_crtc_state);
> > -	if (old_crtc_state->hw.active)
> > -		intel_old_crtc_state_disables(state,
> > -					      old_crtc_state,
> > -					      new_crtc_state,
> > -					      crtc);
> > -}
> > -
> >  static void intel_commit_modeset_disables(struct
> > intel_atomic_state *state)
> >  {
> >  	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> >  	struct intel_crtc *crtc;
> >  	int i;
> >  
> > -	/*
> > -	 * Disable CRTC/pipes in reverse order because some
> > features(MST in
> > -	 * TGL+) requires master and slave relationship between pipes,
> > so it
> > -	 * should always pick the lowest pipe as master as it will be
> > enabled
> > -	 * first and disable in the reverse order so the master will be
> > the
> > -	 * last one to be disabled.
> > -	 */
> > -	for_each_oldnew_intel_crtc_in_state_reverse(state, crtc,
> > old_crtc_state,
> > -						    new_crtc_state, i)
> > {
> > -		if (!needs_modeset(new_crtc_state))
> > +	/* Only disable port sync slaves */
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (!needs_modeset(new_crtc_state) || !crtc->active)
> 
> What's the deal with these crtc->active checks?

With just one loop we were using "old_crtc_state->hw.active" but as we
should not modify the computed state in this phase and
intel_old_crtc_state_disables() sets crtc->active = false, using it
instead.

> 
> >  			continue;
> >  
> >  		/* In case of Transcoder port Sync master slave CRTCs
> > can be
> > @@ -14497,23 +14460,25 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  		 * slave CRTCs are disabled first and then master CRTC
> > since
> >  		 * Slave vblanks are masked till Master Vblanks.
> >  		 */
> > -		if (is_trans_port_sync_mode(new_crtc_state)) {
> > -			if (is_trans_port_sync_master(new_crtc_state))
> > -				intel_trans_port_sync_modeset_disables(
> > state,
> > -								       
> > crtc,
> > -								       
> > old_crtc_state,
> > -								       
> > new_crtc_state);
> > -			else
> > -				continue;
> > -		} else {
> > -			intel_pre_plane_update(old_crtc_state,
> > new_crtc_state);
> > +		if (!is_trans_port_sync_mode(new_crtc_state))
> > +			continue;
> > +		if (is_trans_port_sync_master(new_crtc_state))
> > +			continue;
> 
> We don't have is_trans_sync_slave()?

We don't.

> 
> >  
> > -			if (old_crtc_state->hw.active)
> > -				intel_old_crtc_state_disables(state,
> > -							      old_crtc_
> > state,
> > -							      new_crtc_
> > state,
> > -							      crtc);
> > -		}
> > +		intel_pre_plane_update(old_crtc_state, new_crtc_state);
> > +		intel_old_crtc_state_disables(state, old_crtc_state,
> > +					      new_crtc_state, crtc);
> > +	}
> > +
> > +	/* Disable everything else left on */
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (!needs_modeset(new_crtc_state) || !crtc->active)
> > +			continue;
> > +
> > +		intel_pre_plane_update(old_crtc_state, new_crtc_state);
> > +		intel_old_crtc_state_disables(state, old_crtc_state,
> > +					      new_crtc_state, crtc);
> 
> Pondering if there's any chance of some odd fail if we have two ports
> running in port sync mode. That will now lead to
> disable_slave(0)->disable_slave(1)->disable_master(0)-
> >disable_master(1)...

Thoughts Manasi?

> 
> >  	}
> >  }
> >  
> > -- 
> > 2.24.0