[Intel-gfx] [PATCH v2] drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion

[Chris Wilson]

On Mon, Aug 22, 2016 at 06:21:47PM +0100, Chris Wilson wrote:
> When we check whether we are in the danger region before a vblank at the
> start of a pipe update, the irq must be enabled. This is so that as
> decide whether or not to sleep there is no race between us and the irq
> delivery - i.e. we want to immediately wake up if the irq arrives
> before we try to sleep. Whilst here also remove the DRM_ERROR() for
> hitting a jiffie count of 0 as that also has a race against the timer
> irq - instead replace it with a simple check if the sleep was for more
> than a jiffie (at high resolution >1ms, at low resolution >10ms) as we
> know we only try to wait for the irq within 100us of the vblank.
> 
> We replace the early irq_disable for the pipe update critical section
> with a preempt disable to hog the cpu, disabling irq later when we
> go to program the pipe and queue the following vblank event.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 50 ++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 11f3b38229dd..b12732149346 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -82,10 +82,9 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>  void intel_pipe_update_start(struct intel_crtc *crtc)
>  {
>  	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
> -	long timeout = msecs_to_jiffies_timeout(1);
>  	int scanline, min, max, vblank_start;
> -	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> -	DEFINE_WAIT(wait);
> +
> +	preempt_disable();
>  
>  	vblank_start = adjusted_mode->crtc_vblank_start;
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> @@ -95,19 +94,21 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>  	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode, 100);
>  	max = vblank_start - 1;
>  
> -	local_irq_disable();
> +	crtc->debug.min_vbl = min;
> +	crtc->debug.max_vbl = max;
> +
> +	trace_i915_pipe_update_start(crtc);
>  
>  	if (min <= 0 || max <= 0)
> -		return;
> +		goto out;
>  
>  	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> -		return;
> +		goto out;
>  
> -	crtc->debug.min_vbl = min;
> -	crtc->debug.max_vbl = max;
> -	trace_i915_pipe_update_start(crtc);
> +	do {
> +		wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> +		DEFINE_WAIT(wait);
>  
> -	for (;;) {
>  		/*
>  		 * prepare_to_wait() has a memory barrier, which guarantees
>  		 * other CPUs can see the task state update by the time we
> @@ -116,29 +117,24 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>  		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>  
>  		scanline = intel_get_crtc_scanline(crtc);
> -		if (scanline < min || scanline > max)
> -			break;
> -
> -		if (timeout <= 0) {
> -			DRM_ERROR("Potential atomic update failure on pipe %c\n",
> -				  pipe_name(crtc->pipe));
> -			break;
> +		if (scanline >= min && scanline <= max) {
> +			preempt_enable();
> +			if (!schedule_timeout(2))
> +				DRM_ERROR("vblank wait timed out\n");

Fwiw, this error is still not as important as the subsequent error about
tearing. A timeout here implies (a) the hardware is broken or (b) the
system is under so much stress that we can't be woken in time.

Certainly it should only be WARN and possibly just a DEBUG_VBL?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre