Re: [Intel-gfx] [PATCH 2/4] drm/i915/display: Always enables MST master pipe first

From: "Souza, Jose" <jose.souza@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915/display: Always enables MST master pipe first
Date: Mon, 9 Dec 2019 18:47:48 +0000	[thread overview]
Message-ID: <fe50485e04ea4ea729ca754c1ecf3f39f6b9dff9.camel@intel.com> (raw)
In-Reply-To: <20191209171928.GV1208@intel.com>

On Mon, 2019-12-09 at 19:19 +0200, Ville Syrjälä wrote:
> On Fri, Dec 06, 2019 at 05:18:30PM -0800, José Roberto de Souza
> wrote:
> > Due to DDB overlaps the pipe enabling sequence is not always
> > crescent.
> > As the previous patch selects the first pipe/transcoder in the MST
> > stream to the MST master and it needs to be enabled first this
> > changes were needed to guarantee that.
> > 
> > So here it will first loop through all the MST masters and other
> > pipes that do not have pipe dependencies and enabling then, as when
> > the master is being enabled all the slaves are also going to a full
> > modeset they will not overlap with each other.
> > Then on the second loop it will enable all the MST slaves.
> > 
> > I have tried to put port sync pipes into those two loops but
> > intel_update_trans_port_sync_crtcs() is doing way more than just
> > enable pipes, reading spec I guess it could be accomplish but I
> > will
> > leave it to people working on port sync.
> > At least now the port sync pipes are enabled by last so the slave
> > DDB
> > allocation will not overlap with other pipes.
> > 
> > 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 | 120
> > ++++++++++++++++---
> >  1 file changed, 105 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index f89494c849ce..2f74c0bfb2a8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14576,6 +14576,39 @@ static void
> > intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc,
> >  				       state);
> >  }
> >  
> > +static void
> > +skl_commit_modeset_enable_pipe(struct intel_crtc *crtc,
> > +			       struct intel_crtc_state *old_crtc_state,
> > +			       struct intel_crtc_state *new_crtc_state,
> > +			       unsigned int *updated, bool *progress,
> > +			       struct skl_ddb_entry *entry)
> > +{
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(old_crtc_state->uapi.state);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	bool vbl_wait = false;
> > +
> > +	*updated = *updated | BIT(crtc->pipe);
> > +	*progress = true;
> > +	*entry = new_crtc_state->wm.skl.ddb;
> > +
> > +	/*
> > +	 * If this is an already active pipe, it's DDB changed,
> > +	 * and this isn't the last pipe that needs updating
> > +	 * then we need to wait for a vblank to pass for the
> > +	 * new ddb allocation to take effect.
> > +	 */
> > +	if (!needs_modeset(new_crtc_state) &&
> > +	    state->wm_results.dirty_pipes != *updated &&
> > +	    !skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
> > +				 &old_crtc_state->wm.skl.ddb))
> > +		vbl_wait = true;
> > +
> > +	intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
> > +
> > +	if (vbl_wait)
> > +		intel_wait_for_vblank(dev_priv, crtc->pipe);
> > +}
> > +
> >  static void skl_commit_modeset_enables(struct intel_atomic_state
> > *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > @@ -14600,18 +14633,84 @@ 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.
> > +	 *
> > +	 * First enable all the pipes that do not depends on other
> > pipes while
> > +	 * respecting the DDB allocation overlaps.
> > +	 *
> > +	 * TODO: integrate port sync to the loops bellow.
> > +	 * Port sync is not respecting the DDB allocation overlaps as
> > it
> > +	 * was not checking for the slave port overlaps and there is
> > more than
> > +	 * just a pipe enable in intel_update_trans_port_sync_crtcs()
> >  	 */
> >  	do {
> >  		progress = false;
> >  
> > -		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state, new_crtc_state, i) {
> > +		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +						    new_crtc_state, i)
> > {
> > +			if (updated & BIT(crtc->pipe) ||
> > +			    !new_crtc_state->hw.active)
> > +				continue;
> > +
> > +			if (intel_dp_mst_is_slave_trans(new_crtc_state)
> > ||
> > +			    is_trans_port_sync_mode(new_crtc_state))
> > +				continue;
> > +
> > +			if
> > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > +							entries,
> > +							INTEL_NUM_PIPES
> > (dev_priv), i))
> > +				continue;
> > +
> > +			skl_commit_modeset_enable_pipe(crtc,
> > old_crtc_state,
> > +						       new_crtc_state,
> > &updated,
> > +						       &progress,
> > &entries[i]);
> > +		}
> > +	} while (progress);
> > +
> > +	/*
> > +	 * Now enable all the pipes that depends on other pipe aka MST
> > slaves
> > +	 */
> > +	do {
> > +		progress = false;
> 
> I think we probably want to split the modeset from the other updates
> now so that we can avoid repeating all this stuff for modesets.
> 
> Something like:
> 
> do {
> 	for_each_crtc() {
> 		if (needs_modeset()
> 			continue;
> 		// current thing
> 	}
> } while (progress);
> 
> for_each_crtc() {
> 	if (!needs_modeset())
> 		continue;
> 
> 	if (port_sync || mst_slave)
> 		continue;
> 
> 	WARN_ON(allocaiton_overlaps());
> 	enable_crtc();
> }
> 
> for_each_crtc() {
> 	if (!needs_modeset())
> 		continue;
> 
> 	if (port_sync)
> 		continue;
> 
> 	WARN_ON(allocaiton_overlaps());
> 	enable_crtc();
> }
> 
> for_each_crtc() {
> 	if (!needs_modeset())
> 		continue;
> 
> 	if (!port_sync_master)
> 		continue;
> 
> 	WARN_ON(allocaiton_overlaps());
> 	enable_port_sync();
> }
> 
> Still repetitive, but at leeast we don't have to repeat all the ddb
> overlap avoidance stuff.

Looks good, will update it.

> 
> > +
> > +		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +						    new_crtc_state, i)
> > {
> > +			if (updated & BIT(crtc->pipe) ||
> > +			    !new_crtc_state->hw.active)
> > +				continue;
> > +
> > +			if (is_trans_port_sync_mode(new_crtc_state))
> > +				continue;
> > +
> > +			if
> > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > +							entries,
> > +							INTEL_NUM_PIPES
> > (dev_priv), i))
> > +				continue;
> > +
> > +			skl_commit_modeset_enable_pipe(crtc,
> > old_crtc_state,
> > +						       new_crtc_state,
> > &updated,
> > +						       &progress,
> > &entries[i]);
> > +		}
> > +	} while (progress);
> > +
> > +	/* Port sync loop */
> > +	do {
> > +		progress = false;
> > +
> > +		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +						    new_crtc_state, i)
> > {
> >  			enum pipe pipe = crtc->pipe;
> >  			bool vbl_wait = false;
> >  			bool modeset = needs_modeset(new_crtc_state);
> >  
> > -			if (updated & BIT(crtc->pipe) ||
> > !new_crtc_state->hw.active)
> > +			if (updated & BIT(pipe) || !new_crtc_state-
> > >hw.active)
> > +				continue;
> > +
> > +			if (!is_trans_port_sync_master(new_crtc_state))
> > +				continue;
> > +
> > +			if (!modeset)
> >  				continue;
> >  
> >  			if
> > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > @@ -14634,18 +14733,9 @@ static void
> > skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  			    state->wm_results.dirty_pipes != updated)
> >  				vbl_wait = true;
> >  
> > -			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);
> > -			}
> > +			intel_update_trans_port_sync_crtcs(crtc, state,
> > +							   old_crtc_sta
> > te,
> > +							   new_crtc_sta
> > te);
> >  
> >  			if (vbl_wait)
> >  				intel_wait_for_vblank(dev_priv, pipe);
> > -- 
> > 2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx