[2/4] drm/i915/display: Always enables MST master pipe first
diff mbox series

Message ID 20191207011832.422566-2-jose.souza@intel.com
State New
Headers show
Series
  • [1/4] drm/i915/tgl: Select master transcoder for MST stream
Related show

Commit Message

José Roberto de Souza Dec. 7, 2019, 1:18 a.m. UTC
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(-)

Comments

Ville Syrjälä Dec. 9, 2019, 5:19 p.m. UTC | #1
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.

> +
> +		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_crtcs(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_state,
> +							   new_crtc_state);
>  
>  			if (vbl_wait)
>  				intel_wait_for_vblank(dev_priv, pipe);
> -- 
> 2.24.0
José Roberto de Souza Dec. 9, 2019, 6:47 p.m. UTC | #2
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

Patch
diff mbox series

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;
+
+		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_crtcs(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_state,
+							   new_crtc_state);
 
 			if (vbl_wait)
 				intel_wait_for_vblank(dev_priv, pipe);