drm/i915: Reserve ring buffer space for i915_add_request() commands
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.
v4: Added extra comment and removed duplicate WARN (feedback from Tomas).
For: VIZ-5115
CC: Tomas Elf <tomas.elf@intel.com>
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b70d25b..0c2bf0e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2113,6 +2113,9 @@
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;
@@ -2140,6 +2143,9 @@
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)
@@ -2190,16 +2196,77 @@
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;
+ /*
+ * Add on the reserved size to the request to make sure that after
+ * the intended commands have been emitted, there is guaranteed to
+ * still be enough free space to send them to the hardware.
+ */
+ if (!ringbuf->reserved_in_use)
+ bytes += ringbuf->reserved_size;
+
if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
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)) {