Message ID | 1433419594-32559-1-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 04/06/2015 13:06, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@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@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_ */ >
On Thu, Jun 04, 2015 at 01:06:34PM +0100, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@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@Intel.com> From the last review round there's still my question wrt the correctness of the reservation overflow vs. wrapping outstanding: http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/56575 Also when resending patches, especially after such a long delay please leave some indication of what you've decided to do wrt review comments. Either as a reply in the review discussion (preferred) or at least as an update in the cover letter or per-patch changelog. Otherwise reviewers need to reverse-engineer what you have or haven't done by diffing patches, which is just not that efficient. -Daniel > --- > 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; > + > if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) { > + WARN_ON(ringbuf->reserved_in_use); > + > 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); > + > 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_ */ > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 17/06/2015 15:04, Daniel Vetter wrote: > On Thu, Jun 04, 2015 at 01:06:34PM +0100, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@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@Intel.com> > From the last review round there's still my question wrt the correctness > of the reservation overflow vs. wrapping outstanding: > > http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/56575 v2 - 'added re-intialisation of tracking state after a buffer wrap to keep the sanity checks accurate'. Does that not address your issue with wrapping? > Also when resending patches, especially after such a long delay please > leave some indication of what you've decided to do wrt review comments. > Either as a reply in the review discussion (preferred) or at least as an > update in the cover letter or per-patch changelog. Otherwise reviewers > need to reverse-engineer what you have or haven't done by diffing patches, > which is just not that efficient. > -Daniel > >> --- >> 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; >> + >> if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) { >> + WARN_ON(ringbuf->reserved_in_use); >> + >> 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); >> + >> 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_ */ >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; + if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) { + WARN_ON(ringbuf->reserved_in_use); + 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); + 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_ */