[PATCH 0/2] drm/i915: Ring buffer stuff

[PATCH 0/2] drm/i915: Ring buffer stuff

[ville.syrjala]

After getting slightly confused by the values of the ring registers
in the error state dumps, I started to actually read the specs and
compare what I learned with the code.

I spotted one possible issue with the head and tail handling. But I
don't know if my patch might fix some obscure bug, or not.
gem_ringfill seems to work equally well with and without the patch.

[PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

According to BSpec the ring head and tail pointers must not be
on the same cacheline when head > tail. The easiest way to enforce
this is to reduce the reported ring space.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2cba7b4..d184727 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -168,7 +168,7 @@ void i915_kernel_lost_context(struct drm_device * dev)
 
 	ring->head = I915_READ_HEAD(ring) & HEAD_ADDR;
 	ring->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
-	ring->space = ring->head - (ring->tail + 8);
+	ring->space = ring->head - (ring->tail + 64);
 	if (ring->space < 0)
 		ring->space += ring->size;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c828169..70a184e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -46,7 +46,7 @@ struct pipe_control {
 
 static inline int ring_space(struct intel_ring_buffer *ring)
 {
-	int space = (ring->head & HEAD_ADDR) - (ring->tail + 8);
+	int space = (ring->head & HEAD_ADDR) - (ring->tail + 64);
 	if (space < 0)
 		space += ring->size;
 	return space;
@@ -1163,7 +1163,7 @@ static int intel_ring_wait_request(struct intel_ring_buffer *ring, int n)
 		if (request->tail == -1)
 			continue;
 
-		space = request->tail - (ring->tail + 8);
+		space = request->tail - (ring->tail + 64);
 		if (space < 0)
 			space += ring->size;
 		if (space >= n) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 2ea7a31..b15f896 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -187,7 +187,7 @@ void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring);
 int __must_check intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n);
 static inline int intel_wait_ring_idle(struct intel_ring_buffer *ring)
 {
-	return intel_wait_ring_buffer(ring, ring->size - 8);
+	return intel_wait_ring_buffer(ring, ring->size - 64);
 }
 
 int __must_check intel_ring_begin(struct intel_ring_buffer *ring, int n);
-- 
1.7.8.6

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

[PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ringbuffer tail pointer must be qword aligned. Warn if someone
makes a mistake and forgets to pad the ring when the commands
inserted into the ring don't align to qword naturally.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 70a184e..79c8b13 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1275,6 +1275,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
 	ring->tail &= ring->size - 1;
 	if (dev_priv->stop_rings & intel_ring_flag(ring))
 		return;
+	/* tail must be qword aligned */
+	WARN_ON(ring->tail & 7);
 	ring->write_tail(ring, ring->tail);
 }
 
-- 
1.7.8.6

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

Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Daniel Vetter]

On Mon, Nov 26, 2012 at 1:48 PM,  <ville.syrjala@linux.intel.com> wrote:
> +       /* tail must be qword aligned */
> +       WARN_ON(ring->tail & 7);

Minor bikeshed: Can you move the comment into the WARN argument so
that if someone forgets to correctly pad stuff, dmesg will directly
tell him wants wrong, instead of being forced to check the line
numbers? Helps ridiculously lazy people like me ... ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Ringbuffer tail pointer must be qword aligned. Warn if someone
> makes a mistake and forgets to pad the ring when the commands
> inserted into the ring don't align to qword naturally.

The assertion should be that we wrote precisely the number of dwords we
declared in intel_ring_begin(). Which is one of the important factors to
check whenever reviewing such code. This assertion (which should be a
BUG_ON) is no substitute for such review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Mon, 26 Nov 2012 14:48:18 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> According to BSpec the ring head and tail pointers must not be
> on the same cacheline when head > tail. The easiest way to enforce
> this is to reduce the reported ring space.

I'm going to admit blindness because I don't see that warning in the
gen2-gen7 bspecs. Can you please give chapter and verse, and check to
see if there is a rationale?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Ville Syrjälä]

On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote:
> On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Ringbuffer tail pointer must be qword aligned. Warn if someone
> > makes a mistake and forgets to pad the ring when the commands
> > inserted into the ring don't align to qword naturally.
> 
> The assertion should be that we wrote precisely the number of dwords we
> declared in intel_ring_begin(). Which is one of the important factors to
> check whenever reviewing such code. This assertion (which should be a
> BUG_ON) is no substitute for such review.

Yeah. I was considering adding some reserved_space field to the ring,
populate it in ring_begin(), and and make sure it was correctly
consumed at ring_advance(). If you think that sounds good, I can cook
up a patch for it.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote:
> > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Ringbuffer tail pointer must be qword aligned. Warn if someone
> > > makes a mistake and forgets to pad the ring when the commands
> > > inserted into the ring don't align to qword naturally.
> > 
> > The assertion should be that we wrote precisely the number of dwords we
> > declared in intel_ring_begin(). Which is one of the important factors to
> > check whenever reviewing such code. This assertion (which should be a
> > BUG_ON) is no substitute for such review.
> 
> Yeah. I was considering adding some reserved_space field to the ring,
> populate it in ring_begin(), and and make sure it was correctly
> consumed at ring_advance(). If you think that sounds good, I can cook
> up a patch for it.

To be honest, I was thinking of a firing squad for the author and
reviewers of any such patch that gets intel_ring_begin()..end() wrong.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head

[Ville Syrjälä]

On Mon, Nov 26, 2012 at 04:28:33PM +0000, Chris Wilson wrote:
> On Mon, 26 Nov 2012 14:48:18 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > According to BSpec the ring head and tail pointers must not be
> > on the same cacheline when head > tail. The easiest way to enforce
> > this is to reduce the reported ring space.
> 
> I'm going to admit blindness because I don't see that warning in the
> gen2-gen7 bspecs. Can you please give chapter and verse, and check to
> see if there is a rationale?

It's always the last thing in the section titled 'Ring Buffer Use'. I
believe it's present in all the pre-snb internal bspecs, and it's also
in all the public docs. I can't find it in the internal snb+ bspec but
then again those don't seem to include the relevant chapter at all.

Of course I can't be sure if it's a valid issue, or just something that
got faithfully copypasted from one document to the next.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Ville Syrjälä]

On Mon, Nov 26, 2012 at 05:46:48PM +0000, Chris Wilson wrote:
> On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote:
> > > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Ringbuffer tail pointer must be qword aligned. Warn if someone
> > > > makes a mistake and forgets to pad the ring when the commands
> > > > inserted into the ring don't align to qword naturally.
> > > 
> > > The assertion should be that we wrote precisely the number of dwords we
> > > declared in intel_ring_begin(). Which is one of the important factors to
> > > check whenever reviewing such code. This assertion (which should be a
> > > BUG_ON) is no substitute for such review.
> > 
> > Yeah. I was considering adding some reserved_space field to the ring,
> > populate it in ring_begin(), and and make sure it was correctly
> > consumed at ring_advance(). If you think that sounds good, I can cook
> > up a patch for it.
> 
> To be honest, I was thinking of a firing squad for the author and
> reviewers of any such patch that gets intel_ring_begin()..end() wrong.

I was mainly thinking it might helpful during development, to catch
obvious bugs. A compile time check would be even better though.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On Mon, 26 Nov 2012 20:02:00 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Nov 26, 2012 at 04:28:33PM +0000, Chris Wilson wrote:
> > On Mon, 26 Nov 2012 14:48:18 +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > According to BSpec the ring head and tail pointers must not be
> > > on the same cacheline when head > tail. The easiest way to enforce
> > > this is to reduce the reported ring space.
> > 
> > I'm going to admit blindness because I don't see that warning in the
> > gen2-gen7 bspecs. Can you please give chapter and verse, and check to
> > see if there is a rationale?
> 
> It's always the last thing in the section titled 'Ring Buffer Use'. I
> believe it's present in all the pre-snb internal bspecs, and it's also
> in all the public docs. I can't find it in the internal snb+ bspec but
> then again those don't seem to include the relevant chapter at all.

Gotcha. Yes, nice catch.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Ben Widawsky]

On Mon, 26 Nov 2012 20:09:00 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Nov 26, 2012 at 05:46:48PM +0000, Chris Wilson wrote:
> > On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote:
> > > > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Ringbuffer tail pointer must be qword aligned. Warn if someone
> > > > > makes a mistake and forgets to pad the ring when the commands
> > > > > inserted into the ring don't align to qword naturally.
> > > > 
> > > > The assertion should be that we wrote precisely the number of dwords we
> > > > declared in intel_ring_begin(). Which is one of the important factors to
> > > > check whenever reviewing such code. This assertion (which should be a
> > > > BUG_ON) is no substitute for such review.
> > > 
> > > Yeah. I was considering adding some reserved_space field to the ring,
> > > populate it in ring_begin(), and and make sure it was correctly
> > > consumed at ring_advance(). If you think that sounds good, I can cook
> > > up a patch for it.
> > 
> > To be honest, I was thinking of a firing squad for the author and
> > reviewers of any such patch that gets intel_ring_begin()..end() wrong.
> 
> I was mainly thinking it might helpful during development, to catch
> obvious bugs. A compile time check would be even better though.
> 

It is nice for development (the version that Chris was hinting at), if
the perf cost is provably low, or you can wrap it neatly in some #if
blocks, I'd be in favor.

Since you're fairly new to this world though, I'll mention that my
opinion is almost always the minority, and the kiss of death in some
cases... sorry.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx