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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel