diff mbox

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

Message ID 1354041298-18898-2-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala Nov. 27, 2012, 6:34 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

From BSpec:
"If the Ring Buffer Head Pointer and the Tail Pointer are on the same
cacheline, the Head Pointer must not be greater than the Tail
Pointer."

The easiest way to enforce this is to reduce the reported ring space.

References:
Gen2 BSpec "1. Programming Environment" / "1.4.4.6 Ring Buffer Use"
Gen3 BSpec "vol1c Memory Interface Functions" / "2.3.4.5 Ring Buffer Use"
Gen4+ BSpec "vol1c Memory Interface and Command Stream" / "5.3.4.5 Ring Buffer Use"

v2: Include the exact BSpec references in the description

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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(-)

Comments

Daniel Vetter Nov. 28, 2012, 8:45 p.m. UTC | #1
On Tue, Nov 27, 2012 at 08:34:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> From BSpec:
> "If the Ring Buffer Head Pointer and the Tail Pointer are on the same
> cacheline, the Head Pointer must not be greater than the Tail
> Pointer."
> 
> The easiest way to enforce this is to reduce the reported ring space.
> 
> References:
> Gen2 BSpec "1. Programming Environment" / "1.4.4.6 Ring Buffer Use"
> Gen3 BSpec "vol1c Memory Interface Functions" / "2.3.4.5 Ring Buffer Use"
> Gen4+ BSpec "vol1c Memory Interface and Command Stream" / "5.3.4.5 Ring Buffer Use"
> 
> v2: Include the exact BSpec references in the description
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Another small bikeshed for this one: Less magic numbers with

#define RING_FREE_SPACE 64

-Daniel
> ---
>  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 86b93b2..79089aa 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -141,7 +141,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 b0a83ce..cf11f8a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -45,7 +45,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;
> @@ -1260,7 +1260,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 5af65b8..2f6d9b1 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -191,7 +191,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
Ville Syrjala Nov. 29, 2012, 11:25 a.m. UTC | #2
On Wed, Nov 28, 2012 at 09:45:46PM +0100, Daniel Vetter wrote:
> On Tue, Nov 27, 2012 at 08:34:55PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > From BSpec:
> > "If the Ring Buffer Head Pointer and the Tail Pointer are on the same
> > cacheline, the Head Pointer must not be greater than the Tail
> > Pointer."
> > 
> > The easiest way to enforce this is to reduce the reported ring space.
> > 
> > References:
> > Gen2 BSpec "1. Programming Environment" / "1.4.4.6 Ring Buffer Use"
> > Gen3 BSpec "vol1c Memory Interface Functions" / "2.3.4.5 Ring Buffer Use"
> > Gen4+ BSpec "vol1c Memory Interface and Command Stream" / "5.3.4.5 Ring Buffer Use"
> > 
> > v2: Include the exact BSpec references in the description
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Another small bikeshed for this one: Less magic numbers with
> 
> #define RING_FREE_SPACE 64

Can do. And since the number will only appear in one place, I think
I'll stick the BSpec references into a comment above it.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 86b93b2..79089aa 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -141,7 +141,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 b0a83ce..cf11f8a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -45,7 +45,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;
@@ -1260,7 +1260,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 5af65b8..2f6d9b1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -191,7 +191,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);