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

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

[Chris Wilson]

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@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_sprite.c | 47 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 11f3b38229dd..a6a51f16ae0b 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
@@ -119,26 +120,22 @@ void intel_pipe_update_start(struct intel_crtc *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;
-		}
-
-		local_irq_enable();
-
-		timeout = schedule_timeout(timeout);
+		preempt_enable();
+		if (!schedule_timeout(2))
+			DRM_ERROR("vblank wait timed out\n");
+		preempt_disable();
 
-		local_irq_disable();
-	}
-
-	finish_wait(wq, &wait);
+		finish_wait(wq, &wait);
+	} while (0);
 
 	drm_crtc_vblank_put(&crtc->base);
 
-	crtc->debug.scanline_start = scanline;
+out:
+	local_irq_disable();
+
 	crtc->debug.start_vbl_time = ktime_get();
 	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
+	crtc->debug.scanline_start = intel_get_crtc_scanline(crtc);
 
 	trace_i915_pipe_update_vblank_evaded(crtc);
 }
@@ -192,6 +189,8 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 			  crtc->debug.min_vbl, crtc->debug.max_vbl,
 			  crtc->debug.scanline_start, scanline_end);
 	}
+
+	preempt_enable();
 }
 
 static void
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion

[Chris Wilson]

On Mon, Aug 22, 2016 at 05:56:24PM +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@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 47 ++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 11f3b38229dd..a6a51f16ae0b 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
> @@ -119,26 +120,22 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>  		if (scanline < min || scanline > max)
>  			break;

Hmm, prepare_to_wait() itself has a might_sleep() check, preventing the
preempt fun. The challenge is that we do not want to be interrupted
once we decide to apply the update (reading the scanline) - but that
read has to be after prepare_to_wait to prevent any races before
sleeping.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion

[Chris Wilson]

On Mon, Aug 22, 2016 at 06:15:53PM +0100, Chris Wilson wrote:
> Hmm, prepare_to_wait() itself has a might_sleep() check, preventing the
> preempt fun. The challenge is that we do not want to be interrupted
> once we decide to apply the update (reading the scanline) - but that
> read has to be after prepare_to_wait to prevent any races before
> sleeping.

Scratch that, didn't read the warning carefully enough and was thrown by
an unreliable frame: 

[   32.616197]  [<ffffffff8104583a>] warn_slowpath_fmt+0x4a/0x50
[   32.616208]  [<ffffffff8107ff6b>] ? prepare_to_wait+0x2b/0x90
[   32.616222]  [<ffffffff8107ff6b>] ? prepare_to_wait+0x2b/0x90
[   32.616229]  [<ffffffff810686b7>] __might_sleep+0x77/0x80

That of course is the wrong way around for prepare_to_wait to be doing
the might_sleep. The issue was just not rearranging the control flow
correctly to finish_wait.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@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");
+			preempt_disable();
 		}
 
-		local_irq_enable();
-
-		timeout = schedule_timeout(timeout);
-
-		local_irq_disable();
-	}
-
-	finish_wait(wq, &wait);
+		finish_wait(wq, &wait);
+	} while (0);
 
 	drm_crtc_vblank_put(&crtc->base);
 
-	crtc->debug.scanline_start = scanline;
+out:
+	local_irq_disable();
+
 	crtc->debug.start_vbl_time = ktime_get();
 	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
+	crtc->debug.scanline_start = intel_get_crtc_scanline(crtc);
 
 	trace_i915_pipe_update_vblank_evaded(crtc);
 }
@@ -192,6 +188,8 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 			  crtc->debug.min_vbl, crtc->debug.max_vbl,
 			  crtc->debug.scanline_start, scanline_end);
 	}
+
+	preempt_enable();
 }
 
 static void
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: warning for drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion (rev2)
URL   : https://patchwork.freedesktop.org/series/11427/
State : warning

== Summary ==

Series 11427v2 drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion
http://patchwork.freedesktop.org/api/1.0/series/11427/revisions/2/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                dmesg-warn -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-byt-n2820)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)

fi-hsw-i7-4770k  total:249  pass:219  dwarn:7   dfail:0   fail:1   skip:22 
fi-kbl-qkkr      total:249  pass:189  dwarn:28  dfail:0   fail:4   skip:28 
fi-skl-i7-6700k  total:249  pass:214  dwarn:4   dfail:2   fail:1   skip:28 
fi-snb-i7-2600   total:249  pass:202  dwarn:4   dfail:0   fail:1   skip:42 
ro-bdw-i5-5250u  total:240  pass:220  dwarn:2   dfail:0   fail:0   skip:18 
ro-bdw-i7-5557U  total:240  pass:217  dwarn:4   dfail:0   fail:0   skip:19 
ro-bdw-i7-5600u  total:240  pass:205  dwarn:1   dfail:0   fail:2   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:213  dwarn:0   dfail:0   fail:1   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:2   skip:59 
ro-ivb-i7-3770   total:240  pass:204  dwarn:0   dfail:0   fail:1   skip:35 
ro-ivb2-i7-3770  total:240  pass:208  dwarn:0   dfail:0   fail:1   skip:31 
ro-skl3-i5-6260u total:240  pass:225  dwarn:0   dfail:0   fail:1   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1968/

f1707bb drm-intel-nightly: 2016y-08m-22d-17h-43m-22s UTC integration manifest
ad4f88a drm/i915/sprite: Fix race of vblank irq vs wait in vblank evasion

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx