Re: [Intel-gfx] [PATCH v3 08/11] drm/i915/display: Always enables MST master pipe first

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

On Wed, Dec 18, 2019 at 05:27:00PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-18 at 18:43 +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 02:07:39PM -0800, José Roberto de Souza
> > wrote:
> > > Due to DDB overlaps the pipe enabling sequence is not always
> > > crescent.
> > > As the previous patch selects the smallest pipe/transcoder in the
> > > MST
> > > stream to be master and it needs to be enabled first this changes
> > > were needed to guarantee that.
> > > 
> > > So first lets enable all pipes that did not needed a fullmodeset so
> > > it don't have any external dependency, this ones can overlap with
> > > each other ddb allocations.
> > > 
> > > Then on the second loop it will enable all the pipes that needs a
> > > modeset and don't depends on other pipes like MST master
> > > pipe/transcoder.
> > > 
> > > Then finally all the pipes that needs a modeset and have dependency
> > > on other pipes.
> > > 
> > > v3: rebased
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 88 +++++++++++++++-
> > > ----
> > >  1 file changed, 65 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index e976f82a0db7..176e31de68e7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14553,15 +14553,20 @@ static void
> > > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> > >  	u8 required_slices = state->wm_results.ddb.enabled_slices;
> > >  	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> > > -	u8 dirty_pipes = 0;
> > > +	u8 update_pipes = 0, modeset_pipes = 0;
> > >  	int i;
> > >  
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state, new_crtc_state, i) {
> > > +		if (!new_crtc_state->hw.active)
> > > +			continue;
> > > +
> > >  		/* ignore allocations for crtc's that have been turned
> > > off. */
> > > -		if (!needs_modeset(new_crtc_state) && new_crtc_state-
> > > >hw.active)
> > > +		if (!needs_modeset(new_crtc_state)) {
> > >  			entries[i] = old_crtc_state->wm.skl.ddb;
> > > -		if (new_crtc_state->hw.active)
> > > -			dirty_pipes |= BIT(crtc->pipe);
> > > +			update_pipes |= BIT(crtc->pipe);
> > > +		} else {
> > > +			modeset_pipes |= BIT(modeset_pipes);
> > > +		}
> > >  	}
> > >  
> > >  	/* If 2nd DBuf slice required, enable it here */
> > > @@ -14571,16 +14576,18 @@ static void
> > > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  	/*
> > >  	 * Whenever the number of active pipes changes, we need to make
> > > sure we
> > >  	 * update the pipes in the right order so that their ddb
> > > allocations
> > > -	 * never overlap with eachother inbetween CRTC updates.
> > > Otherwise we'll
> > > +	 * never overlap with each other between CRTC updates.
> > > Otherwise we'll
> > >  	 * cause pipe underruns and other bad stuff.
> > > +	 *
> > > +	 * So first lets enable all pipes that did not needed a
> > > fullmodeset so
> > > +	 * it don't have any external dependency
> > >  	 */
> > > -	while (dirty_pipes) {
> > > +	while (update_pipes) {
> > >  		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > >  						    new_crtc_state, i)
> > > {
> > >  			enum pipe pipe = crtc->pipe;
> > > -			bool modeset = needs_modeset(new_crtc_state);
> > >  
> > > -			if ((dirty_pipes & BIT(pipe)) == 0)
> > > +			if ((update_pipes & BIT(pipe)) == 0)
> > >  				continue;
> > >  
> > >  			if
> > > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > > @@ -14589,20 +14596,10 @@ static void
> > > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  				continue;
> > >  
> > >  			entries[i] = new_crtc_state->wm.skl.ddb;
> > > -			dirty_pipes &= ~BIT(pipe);
> > > -
> > > -			if (modeset &&
> > > is_trans_port_sync_mode(new_crtc_state)) {
> > > -				if
> > > (is_trans_port_sync_master(new_crtc_state))
> > > -					intel_update_trans_port_sync_cr
> > > tcs(crtc,
> > > -									
> > >    state,
> > > -									
> > >    old_crtc_state,
> > > -									
> > >    new_crtc_state);
> > > -				else
> > > -					continue;
> > > -			} else {
> > > -				intel_update_crtc(crtc, state,
> > > old_crtc_state,
> > > -						  new_crtc_state);
> > > -			}
> > > +			update_pipes &= ~BIT(pipe);
> > > +
> > > +			intel_update_crtc(crtc, state, old_crtc_state,
> > > +					  new_crtc_state);
> > >  
> > >  			/*
> > >  			 * If this is an already active pipe, it's DDB
> > > changed,
> > > @@ -14612,11 +14609,56 @@ static void
> > > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  			 */
> > >  			if (!skl_ddb_entry_equal(&new_crtc_state-
> > > >wm.skl.ddb,
> > >  						 &old_crtc_state-
> > > >wm.skl.ddb) &&
> > > -			    !modeset && dirty_pipes)
> > > +			    update_pipes)
> > 
> > Needs to be update_pipes|modeset_pipes.
> 
> Okay
> 
> > 
> > >  				intel_wait_for_vblank(dev_priv, pipe);
> > > +
> > 
> > Extra newline.
> > 
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * Enabling all pipes that needs a modeset and do not depends
> > > on other
> > > +	 * pipes
> > > +	 */
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		enum pipe pipe = crtc->pipe;
> > > +
> > > +		if ((modeset_pipes & BIT(pipe)) == 0)
> > > +			continue;
> > > +
> > > +		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > +		    is_trans_port_sync_slave(new_crtc_state))
> > > +			continue;
> > > +
> > > +		modeset_pipes &= ~BIT(pipe);
> > > +
> > > +		if (is_trans_port_sync_mode(new_crtc_state))
> > > +			intel_update_trans_port_sync_crtcs(crtc, state,
> > > +							   old_crtc_sta
> > > te,
> > > +							   new_crtc_sta
> > > te);
> > > +		else
> > > +			intel_update_crtc(crtc, state, old_crtc_state,
> > > +					  new_crtc_state);
> > > +	}
> > > +
> > > +	/*
> > > +	 * Finally enable all pipes that needs a modeset and depends on
> > > +	 * other pipes, right now it is only MST slaves as both port
> > > sync slave
> > > +	 * and master are enabled together
> > > +	 */
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		enum pipe pipe = crtc->pipe;
> > > +
> > > +		if ((modeset_pipes & BIT(pipe)) == 0)
> > > +			continue;
> > > +
> > > +		if (is_trans_port_sync_slave(new_crtc_state))
> > > +			continue;
> > 
> > If we clear the modeset_pipes bit for these in the previous loop we
> > could avoid this check here entirely.
> 
> For me this is cleaner and remind us to rework
> intel_update_trans_port_sync_crtcs() otherwise to do what you suggested
> the loop above will need.
> 
> struct intel_crtc *slave_crtc = intel_get_slave_crtc(new_crtc_state);
> 
> modeset_pipes &= ~BIT(slave_crtc->pipe);
> I'm fine with both ways.

I can live with either one. Though we should perhaps remove the bit
from the mask at some point, and then we could
WARN_ON(modeset_pipes != 0) at the very end.

> 
> > 
> > Still missing the DDB overlap WARNs that I think would be good to
> > have in these two enable loops.
> 
> Why add the warning in those 2 loops? Those are pipes that are disabled at this point so they will not overlap.

They could overlap with already enabled pipes if we either somehow
failed to update all already enabled pipes, or we miscomputed the
ddb entirely. I think for this a bit of self checking is in order.

> 
> If you really want, this will be enough?
> 
> WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> entries, INTEL_NUM_PIPES(dev_priv), i)) 

+ entries[i] = new_crtc_state->ddb; 

so we'll also catch any overlaps between pipes undergoing modesets.

> 
> > 
> > Looks nice otherwise.
> > 
> > > +
> > > +		intel_update_crtc(crtc, state, old_crtc_state,
> > > new_crtc_state);
> > > +	}
> > > +
> > >  	/* If 2nd DBuf slice is no more required disable it */
> > >  	if (INTEL_GEN(dev_priv) >= 11 && required_slices <
> > > hw_enabled_slices)
> > >  		icl_dbuf_slices_update(dev_priv, required_slices);
> > > -- 
> > > 2.24.1

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