Re: [Intel-gfx] [PATCH v4 3/6] drm/i915/display: Always enables MST master pipe first

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

On Wed, Dec 18, 2019 at 10:59:07AM -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
> 
> v4:
> - added check for modeset_pipes too to decide if is necessary for a
> wait a vblank
> - added DDB allocation overlap check for pipes that needs a modeset
> 
> 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 | 108 ++++++++++++++-----
>  1 file changed, 83 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 59b3bfe8b721..a4f252d05b37 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14517,15 +14517,21 @@ 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;
> +	const u8 num_pipes = INTEL_NUM_PIPES(dev_priv);
> +	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 */
> @@ -14535,38 +14541,29 @@ 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,
> -							entries,
> -							INTEL_NUM_PIPES(dev_priv), i))
> +							entries, num_pipes, i))
>  				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_crtcs(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,
> @@ -14576,11 +14573,72 @@ 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 | modeset_pipes))
>  				intel_wait_for_vblank(dev_priv, pipe);
>  		}
>  	}
>  
> +	/*
> +	 * 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;
> +
> +		WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> +						    entries, num_pipes, i));
> +
> +		entries[i] = new_crtc_state->wm.skl.ddb;
> +		modeset_pipes &= ~BIT(pipe);
> +
> +		if (is_trans_port_sync_mode(new_crtc_state)) {
> +			struct intel_crtc *slave_crtc;
> +
> +			intel_update_trans_port_sync_crtcs(crtc, state,
> +							   old_crtc_state,
> +							   new_crtc_state);
> +
> +			slave_crtc = intel_get_slave_crtc(new_crtc_state);
> +			/* TODO: update entries[] of slave */
> +			modeset_pipes &= ~BIT(slave_crtc->pipe);

OK. So we clear the bit for both master+slave at the same time. Should
work and not trip un the WARN at the end (assuming both have their
modeset properly flagged).

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +		} 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;
> +
> +		WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> +						    entries, num_pipes, i));
> +
> +		entries[i] = new_crtc_state->wm.skl.ddb;
> +		modeset_pipes &= ~BIT(pipe);
> +
> +		intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
> +	}
> +
> +	WARN_ON(modeset_pipes);
> +
>  	/* 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