On Wed, Feb 08, 2012 at 01:34:14PM +0000, Chris Wilson wrote:
> By recording the location of every request in the ringbuffer, we know
> that in order to retire the request the GPU must have finished reading
> it and so the GPU head is now beyond the tail of the request. We can
> therefore provide a conservative estimate of where the GPU is reading
> from in order to avoid having to read back the ring buffer registers
> when polling for space upon starting a new write into the ringbuffer.
>
> A secondary effect is that this allows us to convert
> intel_ring_buffer_wait() to use i915_wait_request() and so consolidate
> upon the single function to handle the complicated task of waiting upon
> the GPU. A necessary precaution is that we need to make that wait
> uninterruptible to match the existing conditions as all the callers of
> intel_ring_begin() have not been audited to handle ERESTARTSYS
> correctly.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
I like how this now looks like, two minor quibbles left though:
I think we should mention the 'head going backwards' race which could
happen when we don't clear up all requests due to the mismatch between
ring->tail head updates and actual head updates from the head registers.
Imo it's good enough to explain what's going on in the commit msg.
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 ++
> drivers/gpu/drm/i915/i915_gem.c | 9 ++++-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 70 +++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 15 +++++++
> 4 files changed, 98 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4ee0793..e8e1b85 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -945,6 +945,9 @@ struct drm_i915_gem_request {
> /** GEM sequence number associated with this request. */
> uint32_t seqno;
>
> + /** Postion in the ringbuffer of the end of the request */
> + u32 tail;
> +
> /** Time at which this request was emitted, in jiffies. */
> unsigned long emitted_jiffies;
>
> @@ -1234,6 +1237,8 @@ i915_gem_object_unpin_fence(struct drm_i915_gem_object *obj)
> }
>
> void i915_gem_retire_requests(struct drm_device *dev);
> +void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
> +
> void i915_gem_reset(struct drm_device *dev);
> void i915_gem_clflush_object(struct drm_i915_gem_object *obj);
> int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 51390b0..4de9e4e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1798,6 +1798,7 @@ i915_add_request(struct intel_ring_buffer *ring,
>
> request->seqno = seqno;
> request->ring = ring;
> + request->tail = intel_ring_get_tail(ring);
I'm rather uneasy with using the tail _after_ we emitted the request as
the cookie. With funky hw workarounds like the pipe control flushing dance
for ilk we could receive the seqno write before the gpu has actually
finished processing all the w/a flushes. Similarly (but even smaller race
window) we could receive the seqno write before the gpu has executed the
user irq. So I think the paranoid but right thing to do here is to grab
the tail pointer before emitting the request.
Otherwise I'm ok with grabbing the tail pointer from i915_add_request -
we already have some nice layering violations with the seqno handling
anyway.
Otherwise I'm happy with the patch and the explanatory comments.
Cheers, Daniel
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
