Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

Chris Wilson
2014-11-03T20:59:55Z

On Mon, Nov 03, 2014 at 01:29:04PM +0000, Dave Gordon wrote:
> Fixes to both the LRC and the legacy ringbuffer code to correctly
> calculate and update the available space in a ring.
>
> The logical ring code was updating the software ring 'head' value
> by reading the hardware 'HEAD' register. In LRC mode, this is not
> valid as the hardware is not necessarily executing the same context
> that is being processed by the software. Thus reading the h/w HEAD
> could put an unrelated (undefined, effectively random) value into
> the s/w 'head' -- A Bad Thing for the free space calculations.
>
> In addition, the old code could update a ringbuffer's 'head' value
> from the 'last_retired_head' even when the latter hadn't been recently
> updated and therefore had a value of -1; this would also confuse the
> freespace calculations. Now, we consume 'last_retired_head' in just
> one place, ensuring that this confusion does not arise.

What? Not unless someone had broken it...

This is the code I expect to see here based on requests:

static int ring_wait(struct intel_ringbuffer *ring, int n)
{
int ret;

trace_intel_ringbuffer_wait(ring, n);

do {
struct i915_gem_request *rq;

i915_gem_retire_requests__engine(ring->engine);
if (ring->retired_head != -1) {
ring->head = ring->retired_head;
ring->retired_head = -1;

ring->space = intel_ring_space(ring);
if (ring->space >= n)
return 0;
}

list_for_each_entry(rq, &ring->breadcrumbs, breadcrumb_link)
if (__intel_ring_space(rq->tail, ring->tail,
ring->size, I915_RING_RSVD) >= n)
break;

if (WARN_ON(&rq->breadcrumb_link == &ring->breadcrumbs))
return -EDEADLK;

ret = i915_request_wait(rq);
} while (ret == 0);

return ret;
}

and that works for both execlists and regular...
-Chris

--
Chris Wilson, Intel Open Source Technology Centre