Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/display: Refactor intel_commit_modeset_disables()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Dec 05, 2019 at 08:28:51PM +0000, Souza, Jose wrote:
> On Thu, 2019-12-05 at 12:38 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 04, 2019 at 12:55:10PM -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.
> > > 
> > > v2:
> > > Not using crtc->active as it is deprecated
> > > 
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 86 +++++++---------
> > > ----
> > >  1 file changed, 30 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 68575457d40e..a9f5aaf8df9b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14393,77 +14393,51 @@ 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(state, slave_crtc);
> > > -	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(state, crtc);
> > > -	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;
> > > +	u32 handled = 0;
> > >  	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)
> > > {
> > > +	/* 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))
> > >  			continue;
> > >  
> > > +		/* If it wasn't active no need to check the special
> > > cases */
> > 
> > This comment seems a bit redundant.
> 
> Okay, removing it.
> 
> > 
> > > +		if (!old_crtc_state->hw.active)
> > > +			continue;
> > > +
> > >  		/* In case of Transcoder port Sync master slave CRTCs
> > > can be
> > >  		 * assigned in any order and we need to make sure that
> > >  		 * slave CRTCs are disabled first and then master CRTC
> > > since
> > >  		 * Slave vblanks are masked till Master Vblanks.
> > >  		 */
> > > -		if (is_trans_port_sync_mode(old_crtc_state)) {
> > > -			if (is_trans_port_sync_master(old_crtc_state))
> > > -				intel_trans_port_sync_modeset_disables(
> > > state,
> > > -								       
> > > crtc,
> > > -								       
> > > old_crtc_state,
> > > -								       
> > > new_crtc_state);
> > > -			else
> > > -				continue;
> > > -		} else {
> > > -			intel_pre_plane_update(state, crtc);
> > > +		if (!is_trans_port_sync_mode(old_crtc_state))
> > > +			continue;
> > >  
> > > -			if (old_crtc_state->hw.active)
> > > -				intel_old_crtc_state_disables(state,
> > > -							      old_crtc_
> > > state,
> > > -							      new_crtc_
> > > state,
> > > -							      crtc);
> > > -		}
> > > +		if (is_trans_port_sync_master(old_crtc_state))
> > > +			continue;
> > 
> > I'd still like to see is_trans_port_sync_slave() so we could
> > eliminate
> > the double check here.
> 
> Thinking again, we only need
> 
> if (is_trans_port_sync_master(old_crtc_state))
> 	continue;
> 
> here, no is_trans_port_sync_master() required.

Only if we want to also handle the non-sync pipes here. Which I don't
think we want.

> 
> Adding the is_trans_port_sync_slave() would leave use with
> if (!is_trans_port_sync_master(old_crtc_state))
> 	continue;
> 
> or with a indented block.
> 
> Fixing it...
> 
> > 
> > > +
> > > +		intel_pre_plane_update(state, crtc);
> > > +		intel_old_crtc_state_disables(state, old_crtc_state,
> > > +					      new_crtc_state, crtc);
> > > +		handled |= BIT(crtc->pipe);
> > > +	}
> > > +
> > > +	/* 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) ||
> > > +		    (handled & BIT(crtc->pipe)))
> > > +			continue;
> > > +
> > > +		intel_pre_plane_update(state, crtc);
> > > +		if (old_crtc_state->hw.active)
> > > +			intel_old_crtc_state_disables(state,
> > > old_crtc_state,
> > > +						      new_crtc_state,
> > > crtc);
> > >  	}
> > 
> > The only concern left is the new ordering when we have multiple
> > sets of genlocked pipes. Assuming the hardware is sane I don't
> > think anything *should* break. But who knows.
> > 
> > Apart from that:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> 
> Thanks, will send a new version with the small changes above and keep
> your rvb.
> 
> > >  }
> > >  
> > > -- 
> > > 2.24.0

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx