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

[Souza, Jose]

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 at linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at 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.

> 
> 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.

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)) 

> 
> 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