Re: [PATCH 2/2] drm/i915/skl: Don't mark pipes as dirty unless we've added/removed pipes

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Jul 19, 2016 at 12:30:56PM -0400, Lyude wrote:
> Now that we update the watermark values atomically, we still need to fix
> the case of how we update watermarks when we haven't added or removed
> pipes.
> 
> When we haven't added or removed any pipes, we don't need to worry about
> the ddb allocation changing. As such there's no order of flushing pipes
> we have to guarantee; e.g. each pipe can be flushed at the earliest
> given oppurtunity. This means all we have to do is:
> 
>  - Calculate the new wm values
>  - Update each plane's settings and wm values
>  - Arm the plane's registers to update at the next vblank
> 
> Right now the watermark code takes an additional few steps:
>  - Rewrite the ddb allocations
>  - Arm all of the planes for another update at the start of the next
>    vblank
>  - *Potentially underrun later if we update the wm registers before the
>    start of the next vblank*
> 
> This patch prevents us from marking pipes as dirty unless the number of
> pipes, and with that the ddb allocation, has actually changed. This
> results in us skipping the additional steps, preventing the hardware
> from using any intermediate values during each wm update.
> 
> This also fixes cursors causing monitors to flicker on Skylake. Finally.
> 

This doesn't look right to me.  It's true that we don't need to worry
about *inter*-pipe DDB allocation changing, but *intra*-pipe DDB
allocations can still change.  We may be resizing planes, changing
scalers, enabling/disabling new planes, etc.  Those operations change
the DDB allocations for the planes within the fixed pipe allocation.
The watermark values will also change accordingly in that case.


Matt

> Signed-off-by: Lyude Paul <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3a7709c..92158e3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state)
>  		if (ret)
>  			return ret;
>  
> -		if (changed)
> -			results->dirty_pipes |= drm_crtc_mask(crtc);
> +		/*
> +		 * We don't need to do any additional setup for the pipes if we
> +		 * haven't changed the number of active crtcs
> +		 */
> +		if (intel_state->active_pipe_changes) {
> +			if (changed)
> +				results->dirty_pipes |= drm_crtc_mask(crtc);
>  
> -		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> -			/* This pipe's WM's did not change */
> -			continue;
> +			if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> +				/* This pipe's WM's did not change */
> +				continue;
> +		}
>  
>  		intel_cstate->update_wm_pre = true;
>  		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
> -- 
> 2.7.4
> 

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