Re: [Intel-gfx] [PATCH 5/8] drm/i915: Streamline skl_commit_modeset_enables()

From: "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 5/8] drm/i915: Streamline skl_commit_modeset_enables()
Date: Tue, 3 Dec 2019 13:47:33 +0000	[thread overview]
Message-ID: <af0c92ae36211929f9012da482cc48fe400f1d86.camel@intel.com> (raw)
In-Reply-To: <20191011200949.7839-6-ville.syrjala@linux.intel.com>

On Fri, 2019-10-11 at 23:09 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> skl_commit_modeset_enables() is a bit of mess. Let's streamline
> it by simply tracking which pipes still need to be updated.
> As a bonus we get rid of the state->wm_results.dirty_pipes usage.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 39 +++++++++---------
> --
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index b9b51bd0c956..9eab67bbf61d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13881,17 +13881,19 @@ static void
> skl_commit_modeset_enables(struct intel_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_crtc *crtc;
>  	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> -	unsigned int updated = 0;
> -	bool progress;
> -	int i;
>  	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;


> +	int i;
>  
> -	for_each_oldnew_intel_crtc_in_state(state, crtc,
> old_crtc_state, new_crtc_state, i)

Really "love" this medieval C standard thing for that nice "i" variable
:)

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> old_crtc_state, new_crtc_state, i) {
>  		/* ignore allocations for crtc's that have been turned
> off. */
> -		if (new_crtc_state->base.active)
> +		if (new_crtc_state->base.active) {
>  			entries[i] = old_crtc_state->wm.skl.ddb;
> +			dirty_pipes |= BIT(crtc->pipe);
> +		}
> +	}
>  
>  	/* If 2nd DBuf slice required, enable it here */
>  	if (INTEL_GEN(dev_priv) >= 11 && required_slices >
> hw_enabled_slices)
> @@ -13903,14 +13905,12 @@ static void
> skl_commit_modeset_enables(struct intel_atomic_state *state)
>  	 * never overlap with eachother inbetween CRTC updates.
> Otherwise we'll
>  	 * cause pipe underruns and other bad stuff.
>  	 */
> -	do {
> -		progress = false;
> -
> -		for_each_oldnew_intel_crtc_in_state(state, crtc,
> old_crtc_state, new_crtc_state, i) {
> +	while (dirty_pipes) {
> +		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;
>  
> -			if (updated & BIT(crtc->pipe) ||
> !new_crtc_state->base.active)
> +			if ((dirty_pipes & BIT(pipe)) == 0)
>  				continue;
>  
>  			if
> (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> @@ -13918,8 +13918,11 @@ static void
> skl_commit_modeset_enables(struct intel_atomic_state *state)
>  							INTEL_NUM_PIPES
> (dev_priv), i))
>  				continue;
>  
> -			updated |= BIT(pipe);
>  			entries[i] = new_crtc_state->wm.skl.ddb;
> +			dirty_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,
> @@ -13930,18 +13933,10 @@ 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) &&
>  			    !new_crtc_state->base.active_changed &&
> -			    state->wm_results.dirty_pipes != updated)
> -				vbl_wait = true;
> -
> -			intel_update_crtc(crtc, state, old_crtc_state,
> -					  new_crtc_state);
> -
> -			if (vbl_wait)
> +			    dirty_pipes)
>  				intel_wait_for_vblank(dev_priv, pipe);
> -
> -			progress = true;
>  		}
> -	} while (progress);
> +	}
>  
>  	/* If 2nd DBuf slice is no more required disable it */
>  	if (INTEL_GEN(dev_priv) >= 11 && required_slices <
> hw_enabled_slices)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx