[PATCH 0/2] Two patches relating to ring initialization failure

[PATCH 0/2] Two patches relating to ring initialization failure

[Sean Paul]

These two patches fix/mitigate an issue I was seeing during suspend/resume. I
would see a "blt ring initialization failed" error in the logs, followed by a
NULL dereference in gen6_render_ring_flush shortly after. The first patch adds
a BUG_ON to catch the NULL, and a bread crumb. The second patch fixes the init
error by waiting for a little bit before failing the read (I found the register
values were correct after waiting a bit).

Sean Paul (2):
  drm/i915: Add BUG_ON when ring->private is NULL
  drm/i915: Add wait_for in init_ring_common

 drivers/gpu/drm/i915/intel_ringbuffer.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

-- 
1.7.7.3

[PATCH 1/2] drm/i915: Add BUG_ON when ring->private is NULL

[Sean Paul]

I have experienced a number of NULL pointer dereferences on
suspend/resume where ring->private is NULL. These come from failed
initialization of the ring buffer. Sincce this doesn't seem to be easily
recoverable, this patch adds a BUG_ON and a bread crumb pointer for
people who might encounter this in the future.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ca3972f..8357822 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -198,9 +198,17 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 {
 	u32 flags = 0;
 	struct pipe_control *pc = ring->private;
-	u32 scratch_addr = pc->gtt_offset + 128;
+	u32 scratch_addr;
 	int ret;
 
+	/* This condition was being hit when the ring buffer wasn't being
+	 * properly initialized and subsequently cleaned (hence setting
+	 * ring->private to NULL).
+	 */
+	BUG_ON(pc == NULL);
+
+	scratch_addr = pc->gtt_offset + 128;
+
 	/* Force SNB workarounds for PIPE_CONTROL flushes */
 	intel_emit_post_sync_nonzero_flush(ring);
 
-- 
1.7.7.3

Re: [PATCH 1/2] drm/i915: Add BUG_ON when ring->private is NULL

[Daniel Vetter]

On Fri, Mar 16, 2012 at 12:43:21PM -0400, Sean Paul wrote:
> I have experienced a number of NULL pointer dereferences on
> suspend/resume where ring->private is NULL. These come from failed
> initialization of the ring buffer. Sincce this doesn't seem to be easily
> recoverable, this patch adds a BUG_ON and a bread crumb pointer for
> people who might encounter this in the future.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Chris shot this down on irc, BUG_ON isn't really more useful than a NULL
pointer oops. I agree, so I've dropped this from -next.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/2] drm/i915: Add BUG_ON when ring->private is NULL

[Sean Paul]

On Sun, Mar 18, 2012 at 2:09 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 16, 2012 at 12:43:21PM -0400, Sean Paul wrote:
>> I have experienced a number of NULL pointer dereferences on
>> suspend/resume where ring->private is NULL. These come from failed
>> initialization of the ring buffer. Sincce this doesn't seem to be easily
>> recoverable, this patch adds a BUG_ON and a bread crumb pointer for
>> people who might encounter this in the future.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Chris shot this down on irc, BUG_ON isn't really more useful than a NULL
> pointer oops. I agree, so I've dropped this from -next.

Yep, makes sense, I agree. Thanks for the explanation.

Sean

> -Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

[PATCH 2/2] drm/i915: Add wait_for in init_ring_common

[Sean Paul]

I have seen a number of "blt ring initialization failed" messages
where the ctl or start registers are not the correct value. Upon further
inspection, if the code just waited a little bit, it would read the
correct value. Adding the wait_for to these reads should eliminate the
issue.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8357822..d95f0f9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -298,9 +298,9 @@ static int init_ring_common(struct intel_ring_buffer *ring)
 			| RING_REPORT_64K | RING_VALID);
 
 	/* If the head is still not zero, the ring is dead */
-	if ((I915_READ_CTL(ring) & RING_VALID) == 0 ||
-	    I915_READ_START(ring) != obj->gtt_offset ||
-	    (I915_READ_HEAD(ring) & HEAD_ADDR) != 0) {
+	if (wait_for((I915_READ_CTL(ring) & RING_VALID) != 0 &&
+		     I915_READ_START(ring) == obj->gtt_offset &&
+		     (I915_READ_HEAD(ring) & HEAD_ADDR) == 0, 50)) {
 		DRM_ERROR("%s initialization failed "
 				"ctl %08x head %08x tail %08x start %08x\n",
 				ring->name,
-- 
1.7.7.3

Re: [PATCH 2/2] drm/i915: Add wait_for in init_ring_common

[Ben Widawsky]

On Fri, Mar 16, 2012 at 12:43:22PM -0400, Sean Paul wrote:
> I have seen a number of "blt ring initialization failed" messages
> where the ctl or start registers are not the correct value. Upon further
> inspection, if the code just waited a little bit, it would read the
> correct value. Adding the wait_for to these reads should eliminate the
> issue.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 8357822..d95f0f9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -298,9 +298,9 @@ static int init_ring_common(struct intel_ring_buffer *ring)
>  			| RING_REPORT_64K | RING_VALID);
>  
>  	/* If the head is still not zero, the ring is dead */
> -	if ((I915_READ_CTL(ring) & RING_VALID) == 0 ||
> -	    I915_READ_START(ring) != obj->gtt_offset ||
> -	    (I915_READ_HEAD(ring) & HEAD_ADDR) != 0) {
> +	if (wait_for((I915_READ_CTL(ring) & RING_VALID) != 0 &&
> +		     I915_READ_START(ring) == obj->gtt_offset &&
> +		     (I915_READ_HEAD(ring) & HEAD_ADDR) == 0, 50)) {
>  		DRM_ERROR("%s initialization failed "
>  				"ctl %08x head %08x tail %08x start %08x\n",
>  				ring->name,
> -- 
> 1.7.7.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add wait_for in init_ring_common

[Daniel Vetter]

On Fri, Mar 16, 2012 at 12:43:22PM -0400, Sean Paul wrote:
> I have seen a number of "blt ring initialization failed" messages
> where the ctl or start registers are not the correct value. Upon further
> inspection, if the code just waited a little bit, it would read the
> correct value. Adding the wait_for to these reads should eliminate the
> issue.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

I've picked up both patches into my -next tree.

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48