Re: [PATCH v4 3/6] drm/i915/skl: Only flush pipes when we change the ddb allocation

[Matt Roper]

On Tue, Jul 26, 2016 at 01:34:39PM -0400, Lyude wrote:
> Manual pipe flushes are only necessary in order to make sure that we prevent
> pipes with changed ddb allocations from overlapping from one another at
> any point in time. Additionally, forcing us to wait for the next vblank
> every time we have to update the watermark values because the cursor was
> moving between screens will introduce a rather noticable lag for users.
> 
> Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1f6fe8c..d65e897 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1597,6 +1597,7 @@ struct skl_ddb_allocation {
>  
>  struct skl_wm_values {
>  	unsigned dirty_pipes;
> +	bool ddb_changed;
>  	struct skl_ddb_allocation ddb;
>  	uint32_t wm_linetime[I915_MAX_PIPES];
>  	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8ae3f2b..cda4196 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3892,6 +3892,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	new_ddb = &new_values->ddb;
>  	cur_ddb = &dev_priv->wm.skl_hw.ddb;
>  
> +	/* We only ever need to flush when the ddb allocations change */
> +	if (!new_values->ddb_changed)
> +		return;
> +
> +	new_values->ddb_changed = false;
> +
>  	/*
>  	 * First pass: flush the pipes with the new allocation contained into
>  	 * the old space.
> @@ -3996,6 +4002,22 @@ pipes_modified(struct drm_atomic_state *state)
>  	return ret;
>  }
>  
> +static bool
> +skl_pipe_ddb_changed(struct skl_ddb_allocation *old,
> +		     struct skl_ddb_allocation *new,
> +		     enum pipe pipe)
> +{
> +	if (memcmp(&old->pipe[pipe], &new->pipe[pipe],
> +		   sizeof(old->pipe[pipe])) != 0 ||
> +	    memcmp(&old->plane[pipe], &new->plane[pipe],
> +		   sizeof(old->plane[pipe])) != 0 ||
> +	    memcmp(&old->y_plane[pipe], &new->y_plane[pipe],
> +		   sizeof(old->y_plane[pipe])) != 0)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int
>  skl_compute_ddb(struct drm_atomic_state *state)
>  {
> @@ -4003,7 +4025,8 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct intel_crtc *intel_crtc;
> -	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
> +	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> +	struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
>  	uint32_t realloc_pipes = pipes_modified(state);
>  	int ret;
>  
> @@ -4041,9 +4064,13 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  		if (IS_ERR(cstate))
>  			return PTR_ERR(cstate);
>  
> -		ret = skl_allocate_pipe_ddb(cstate, ddb);
> +		ret = skl_allocate_pipe_ddb(cstate, new_ddb);
>  		if (ret)
>  			return ret;
> +
> +		if (!intel_state->wm_results.ddb_changed &&

I think you can simplify and drop the first part of this condition here,
but in general,

Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>


> +		    skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe))
> +			intel_state->wm_results.ddb_changed = true;
>  	}
>  
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795