[Intel-gfx] [PATCH 02/55] drm/i915: Reserve ring buffer space for i915_add_request() commands
Tomas Elf
tomas.elf at intel.com
Tue Jun 9 09:00:19 PDT 2015
On 04/06/2015 13:06, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
>
> It is a bad idea for i915_add_request() to fail. The work will already have been
> send to the ring and will be processed, but there will not be any tracking or
> management of that work.
>
> The only way the add request call can fail is if it can't write its epilogue
> commands to the ring (cache flushing, seqno updates, interrupt signalling). The
> reasons for that are mostly down to running out of ring buffer space and the
> problems associated with trying to get some more. This patch prevents that
> situation from happening in the first place.
>
> When a request is created, it marks sufficient space as reserved for the
> epilogue commands. Thus guaranteeing that by the time the epilogue is written,
> there will be plenty of space for it. Note that a ring_begin() call is required
> to actually reserve the space (and do any potential waiting). However, that is
> not currently done at request creation time. This is because the ring_begin()
> code can allocate a request. Hence calling begin() from the request allocation
> code would lead to infinite recursion! Later patches in this series remove the
> need for begin() to do the allocate. At that point, it becomes safe for the
> allocate to call begin() and really reserve the space.
>
> Until then, there is a potential for insufficient space to be available at the
> point of calling i915_add_request(). However, that would only be in the case
> where the request was created and immediately submitted without ever calling
> ring_begin() and adding any work to that request. Which should never happen. And
> even if it does, and if that request happens to fall down the tiny window of
> opportunity for failing due to being out of ring space then does it really
> matter because the request wasn't doing anything in the first place?
>
> v2: Updated the 'reserved space too small' warning to include the offending
> sizes. Added a 'cancel' operation to clean up when a request is abandoned. Added
> re-initialisation of tracking state after a buffer wrap to keep the sanity
> checks accurate.
>
> v3: Incremented the reserved size to accommodate Ironlake (after finally
> managing to run on an ILK system). Also fixed missing wrap code in LRC mode.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 37 +++++++++++++++++
> drivers/gpu/drm/i915/intel_lrc.c | 18 ++++++++
> drivers/gpu/drm/i915/intel_ringbuffer.c | 68 ++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_ringbuffer.h | 25 ++++++++++++
> 5 files changed, 147 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e9d76f3..44dee31 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2187,6 +2187,7 @@ struct drm_i915_gem_request {
>
> int i915_gem_request_alloc(struct intel_engine_cs *ring,
> struct intel_context *ctx);
> +void i915_gem_request_cancel(struct drm_i915_gem_request *req);
> void i915_gem_request_free(struct kref *req_ref);
>
> static inline uint32_t
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 78f6a89..516e9b7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2495,6 +2495,13 @@ int __i915_add_request(struct intel_engine_cs *ring,
> } else
> ringbuf = ring->buffer;
>
> + /*
> + * To ensure that this call will not fail, space for its emissions
> + * should already have been reserved in the ring buffer. Let the ring
> + * know that it is time to use that space up.
> + */
> + intel_ring_reserved_space_use(ringbuf);
> +
> request_start = intel_ring_get_tail(ringbuf);
> /*
> * Emit any outstanding flushes - execbuf can fail to emit the flush
> @@ -2577,6 +2584,9 @@ int __i915_add_request(struct intel_engine_cs *ring,
> round_jiffies_up_relative(HZ));
> intel_mark_busy(dev_priv->dev);
>
> + /* Sanity check that the reserved size was large enough. */
> + intel_ring_reserved_space_end(ringbuf);
> +
> return 0;
> }
>
> @@ -2676,6 +2686,26 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
> if (ret)
> goto err;
>
> + /*
> + * Reserve space in the ring buffer for all the commands required to
> + * eventually emit this request. This is to guarantee that the
> + * i915_add_request() call can't fail. Note that the reserve may need
> + * to be redone if the request is not actually submitted straight
> + * away, e.g. because a GPU scheduler has deferred it.
> + *
> + * Note further that this call merely notes the reserve request. A
> + * subsequent call to *_ring_begin() is required to actually ensure
> + * that the reservation is available. Without the begin, if the
> + * request creator immediately submitted the request without adding
> + * any commands to it then there might not actually be sufficient
> + * room for the submission commands. Unfortunately, the current
> + * *_ring_begin() implementations potentially call back here to
> + * i915_gem_request_alloc(). Thus calling _begin() here would lead to
> + * infinite recursion! Until that back call path is removed, it is
> + * necessary to do a manual _begin() outside.
> + */
> + intel_ring_reserved_space_reserve(req->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> +
> ring->outstanding_lazy_request = req;
> return 0;
>
> @@ -2684,6 +2714,13 @@ err:
> return ret;
> }
>
> +void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> +{
> + intel_ring_reserved_space_cancel(req->ringbuf);
> +
> + i915_gem_request_unreference(req);
> +}
> +
> struct drm_i915_gem_request *
> i915_gem_find_active_request(struct intel_engine_cs *ring)
> {
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6a5ed07..42a756d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -687,6 +687,9 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> unsigned space;
> int ret;
>
> + /* The whole point of reserving space is to not wait! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> if (intel_ring_space(ringbuf) >= bytes)
> return 0;
>
> @@ -747,6 +750,9 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
> uint32_t __iomem *virt;
> int rem = ringbuf->size - ringbuf->tail;
>
> + /* Can't wrap if space has already been reserved! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> if (ringbuf->space < rem) {
> int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
>
> @@ -770,10 +776,22 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
> {
> int ret;
>
> + if (!ringbuf->reserved_in_use)
> + bytes += ringbuf->reserved_size;
This line right here is the main integration point between the buffer
reservation scheme and the existing infrastructure. Please point this
out in a comment down together with the space reservation prototypes
where you describe _where_ this reserved ring space size actually comes
to matter in the end. Or include a comment here. Or both. It's kinda
important to know where the reserved space ends up in the end and where
it integrates.
> +
> if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> + WARN_ON(ringbuf->reserved_in_use);
This WARN_ON is already done in logical_ring_wrap_buffer. Unless there
is a reason for a second warning please remove this one.
> +
> ret = logical_ring_wrap_buffer(ringbuf, ctx);
> if (unlikely(ret))
> return ret;
> +
> + if(ringbuf->reserved_size) {
> + uint32_t size = ringbuf->reserved_size;
> +
> + intel_ring_reserved_space_cancel(ringbuf);
> + intel_ring_reserved_space_reserve(ringbuf, size);
> + }
> }
>
> if (unlikely(ringbuf->space < bytes)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d934f85..74c2222 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2103,6 +2103,9 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
> unsigned space;
> int ret;
>
> + /* The whole point of reserving space is to not wait! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> if (intel_ring_space(ringbuf) >= n)
> return 0;
>
> @@ -2130,6 +2133,9 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
> struct intel_ringbuffer *ringbuf = ring->buffer;
> int rem = ringbuf->size - ringbuf->tail;
>
> + /* Can't wrap if space has already been reserved! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> if (ringbuf->space < rem) {
> int ret = ring_wait_for_space(ring, rem);
> if (ret)
> @@ -2180,16 +2186,74 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
> return 0;
> }
>
> -static int __intel_ring_prepare(struct intel_engine_cs *ring,
> - int bytes)
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> +{
> + /* NB: Until request management is fully tidied up and the OLR is
> + * removed, there are too many ways for get false hits on this
> + * anti-recursion check! */
> + /*WARN_ON(ringbuf->reserved_size);*/
> + WARN_ON(ringbuf->reserved_in_use);
> +
> + ringbuf->reserved_size = size;
> +
> + /*
> + * Really need to call _begin() here but that currently leads to
> + * recursion problems! This will be fixed later but for now just
> + * return and hope for the best. Note that there is only a real
> + * problem if the create of the request never actually calls _begin()
> + * but if they are not submitting any work then why did they create
> + * the request in the first place?
> + */
> +}
> +
> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
> +{
> + WARN_ON(ringbuf->reserved_in_use);
> +
> + ringbuf->reserved_size = 0;
> + ringbuf->reserved_in_use = false;
> +}
> +
> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> +{
> + WARN_ON(ringbuf->reserved_in_use);
> +
> + ringbuf->reserved_in_use = true;
> + ringbuf->reserved_tail = ringbuf->tail;
> +}
> +
> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> +{
> + WARN_ON(!ringbuf->reserved_in_use);
> + WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
> + "request reserved size too small: %d vs %d!\n",
> + ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
> +
> + ringbuf->reserved_size = 0;
> + ringbuf->reserved_in_use = false;
> +}
> +
> +static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes)
> {
> struct intel_ringbuffer *ringbuf = ring->buffer;
> int ret;
>
> + if (!ringbuf->reserved_in_use)
> + bytes += ringbuf->reserved_size;
> +
> if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> + WARN_ON(ringbuf->reserved_in_use);
> +
This WARN_ON is already done in intel_wrap_ring_buffer. Unless there is
a reason for a second warning please remove this one.
Thanks,
Tomas
> ret = intel_wrap_ring_buffer(ring);
> if (unlikely(ret))
> return ret;
> +
> + if(ringbuf->reserved_size) {
> + uint32_t size = ringbuf->reserved_size;
> +
> + intel_ring_reserved_space_cancel(ringbuf);
> + intel_ring_reserved_space_reserve(ringbuf, size);
> + }
> }
>
> if (unlikely(ringbuf->space < bytes)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 39f6dfc..bf2ac28 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -105,6 +105,9 @@ struct intel_ringbuffer {
> int space;
> int size;
> int effective_size;
> + int reserved_size;
> + int reserved_tail;
> + bool reserved_in_use;
>
> /** We track the position of the requests in the ring buffer, and
> * when each is retired we increment last_retired_head as the GPU
> @@ -450,4 +453,26 @@ intel_ring_get_request(struct intel_engine_cs *ring)
> return ring->outstanding_lazy_request;
> }
>
> +/*
> + * Arbitrary size for largest possible 'add request' sequence. The code paths
> + * are complex and variable. Empirical measurement shows that the worst case
> + * is ILK at 136 words. Reserving too much is better than reserving too little
> + * as that allows for corner cases that might have been missed. So the figure
> + * has been rounded up to 160 words.
> + */
> +#define MIN_SPACE_FOR_ADD_REQUEST 160
> +
> +/*
> + * Reserve space in the ring to guarantee that the i915_add_request() call
> + * will always have sufficient room to do its stuff. The request creation
> + * code calls this automatically.
> + */
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
> +/* Cancel the reservation, e.g. because the request is being discarded. */
> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf);
> +/* Use the reserved space - for use by i915_add_request() only. */
> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf);
> +/* Finish with the reserved space - for use by i915_add_request() only. */
> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
> +
> #endif /* _INTEL_RINGBUFFER_H_ */
>
More information about the Intel-gfx
mailing list