[Intel-gfx] [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations
Deepak S
deepak.s at linux.intel.com
Tue Nov 25 08:59:32 CET 2014
On Wednesday 19 November 2014 01:37 AM, Dave Gordon wrote:
> There are numerous places in the code where the driver's idea of
> how much space is left in a ring is updated using the driver's
> latest notions of the positions of 'head' and 'tail' for the ring.
> Among them are some that update one or both of these values before
> (re)doing the calculation. In particular, there are four different
> places in the code where 'last_retired_head' is copied to 'head'
> and then set to -1; and two of these do not have a guard to check
> that it has actually been updated since last time it was consumed,
> leaving the possibility that the dummy -1 can be transferred from
> 'last_retired_head' to 'head', causing the space calculation to
> produce 'impossible' results (previously seen on Android/VLV).
>
> This code therefore consolidates all the calculation and updating of
> these values, such that there is only one place where the ring space
> is updated, and it ALWAYS uses (and consumes) 'last_retired_head' if
> (and ONLY if) it has been updated since the last call.
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 5 ++-
> drivers/gpu/drm/i915/intel_lrc.c | 25 +++++----------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 51 ++++++++++++++++---------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
> 4 files changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5dc37f0..4a98399 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -154,11 +154,10 @@ void i915_kernel_lost_context(struct drm_device *dev)
> if (drm_core_check_feature(dev, DRIVER_MODESET))
> return;
>
> + ringbuf->last_retired_head = -1;
> ringbuf->head = I915_READ_HEAD(ring) & HEAD_ADDR;
> ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
> - ringbuf->space = ringbuf->head - (ringbuf->tail + I915_RING_FREE_SPACE);
> - if (ringbuf->space < 0)
> - ringbuf->space += ringbuf->size;
> + intel_ring_update_space(ringbuf);
>
> if (!dev->primary->master)
> return;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ad31373..c9a5227 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -825,14 +825,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> u32 seqno = 0;
> int ret;
>
> - if (ringbuf->last_retired_head != -1) {
> - ringbuf->head = ringbuf->last_retired_head;
> - ringbuf->last_retired_head = -1;
> -
> - ringbuf->space = intel_ring_space(ringbuf);
> - if (ringbuf->space >= bytes)
> - return 0;
> - }
> + if (intel_ring_space(ringbuf) >= bytes)
> + return 0;
>
> list_for_each_entry(request, &ring->request_list, list) {
> /*
> @@ -860,11 +854,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> return ret;
>
> i915_gem_retire_requests_ring(ring);
> - ringbuf->head = ringbuf->last_retired_head;
> - ringbuf->last_retired_head = -1;
>
> - ringbuf->space = intel_ring_space(ringbuf);
> - return 0;
> + return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
> }
>
> static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> @@ -890,12 +881,10 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> * case by choosing an insanely large timeout. */
> end = jiffies + 60 * HZ;
>
> + ret = 0;
> do {
> - ringbuf->space = intel_ring_space(ringbuf);
> - if (ringbuf->space >= bytes) {
> - ret = 0;
> + if (intel_ring_space(ringbuf) >= bytes)
> break;
> - }
>
> msleep(1);
>
> @@ -936,7 +925,7 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf)
> iowrite32(MI_NOOP, virt++);
>
> ringbuf->tail = 0;
> - ringbuf->space = intel_ring_space(ringbuf);
> + intel_ring_update_space(ringbuf);
>
> return 0;
> }
> @@ -1777,8 +1766,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
> ringbuf->effective_size = ringbuf->size;
> ringbuf->head = 0;
> ringbuf->tail = 0;
> - ringbuf->space = ringbuf->size;
> ringbuf->last_retired_head = -1;
> + intel_ring_update_space(ringbuf);
>
> /* TODO: For now we put this in the mappable region so that we can reuse
> * the existing ringbuffer code which ioremaps it. When we start
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ae09258..a08ae65 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -52,16 +52,27 @@ intel_ring_initialized(struct intel_engine_cs *ring)
>
> int __intel_ring_space(int head, int tail, int size)
> {
> - int space = head - (tail + I915_RING_FREE_SPACE);
> - if (space < 0)
> + int space = head - tail;
> + if (space <= 0)
> space += size;
> - return space;
> + return space - I915_RING_FREE_SPACE;
> +}
> +
> +void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
> +{
> + if (ringbuf->last_retired_head != -1) {
> + ringbuf->head = ringbuf->last_retired_head;
> + ringbuf->last_retired_head = -1;
> + }
> +
> + ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR,
> + ringbuf->tail, ringbuf->size);
> }
>
> int intel_ring_space(struct intel_ringbuffer *ringbuf)
> {
> - return __intel_ring_space(ringbuf->head & HEAD_ADDR,
> - ringbuf->tail, ringbuf->size);
> + intel_ring_update_space(ringbuf);
> + return ringbuf->space;
> }
>
> bool intel_ring_stopped(struct intel_engine_cs *ring)
> @@ -592,10 +603,10 @@ static int init_ring_common(struct intel_engine_cs *ring)
> if (!drm_core_check_feature(ring->dev, DRIVER_MODESET))
> i915_kernel_lost_context(ring->dev);
> else {
> + ringbuf->last_retired_head = -1;
> ringbuf->head = I915_READ_HEAD(ring);
> ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
> - ringbuf->space = intel_ring_space(ringbuf);
> - ringbuf->last_retired_head = -1;
> + intel_ring_update_space(ringbuf);
> }
>
> memset(&ring->hangcheck, 0, sizeof(ring->hangcheck));
> @@ -1880,14 +1891,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
> u32 seqno = 0;
> int ret;
>
> - if (ringbuf->last_retired_head != -1) {
> - ringbuf->head = ringbuf->last_retired_head;
> - ringbuf->last_retired_head = -1;
> -
> - ringbuf->space = intel_ring_space(ringbuf);
> - if (ringbuf->space >= n)
> - return 0;
> - }
> + if (intel_ring_space(ringbuf) >= n)
> + return 0;
>
> list_for_each_entry(request, &ring->request_list, list) {
> if (__intel_ring_space(request->tail, ringbuf->tail,
> @@ -1905,10 +1910,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
> return ret;
>
> i915_gem_retire_requests_ring(ring);
> - ringbuf->head = ringbuf->last_retired_head;
> - ringbuf->last_retired_head = -1;
>
> - ringbuf->space = intel_ring_space(ringbuf);
> return 0;
> }
>
> @@ -1934,14 +1936,14 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
> * case by choosing an insanely large timeout. */
> end = jiffies + 60 * HZ;
>
> + ret = 0;
> trace_i915_ring_wait_begin(ring);
> do {
> + if (intel_ring_space(ringbuf) >= n)
> + break;
> ringbuf->head = I915_READ_HEAD(ring);
> - ringbuf->space = intel_ring_space(ringbuf);
> - if (ringbuf->space >= n) {
> - ret = 0;
> + if (intel_ring_space(ringbuf) >= n)
> break;
> - }
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET) &&
> dev->primary->master) {
> @@ -1989,7 +1991,7 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
> iowrite32(MI_NOOP, virt++);
>
> ringbuf->tail = 0;
> - ringbuf->space = intel_ring_space(ringbuf);
> + intel_ring_update_space(ringbuf);
>
> return 0;
> }
> @@ -2061,6 +2063,7 @@ int intel_ring_begin(struct intel_engine_cs *ring,
> int num_dwords)
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
> + struct intel_ringbuffer *ringbuf = ring->buffer;
> int ret;
>
> ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> @@ -2077,7 +2080,7 @@ int intel_ring_begin(struct intel_engine_cs *ring,
> if (ret)
> return ret;
>
> - ring->buffer->space -= num_dwords * sizeof(uint32_t);
> + ringbuf->space -= num_dwords * sizeof(uint32_t);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index aab2e2f..24a5723 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -404,6 +404,7 @@ static inline void intel_ring_advance(struct intel_engine_cs *ring)
> ringbuf->tail &= ringbuf->size - 1;
> }
> int __intel_ring_space(int head, int tail, int size);
> +void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
> int intel_ring_space(struct intel_ringbuffer *ringbuf);
> bool intel_ring_stopped(struct intel_engine_cs *ring);
> void __intel_ring_advance(struct intel_engine_cs *ring);
Looks fine to me
Reviewed-by: Deepak S<deepak.s at linux.intel.com>
More information about the Intel-gfx
mailing list