[Intel-gfx] [PATCH] drm/i915: Reserve space improvements
Tomas Elf
tomas.elf at intel.com
Thu Jun 25 11:38:59 PDT 2015
On 24/06/2015 18:03, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
>
> An earlier patch was added to reserve space in the ring buffer for the
> commands issued during 'add_request()'. The initial version was
> pessimistic in the way it handled buffer wrapping and would cause
> premature wraps and thus waste ring space.
>
> This patch updates the code to better handle the wrap case. It no
> longer enforces that the space being asked for and the reserved space
> are a single contiguous block. Instead, it allows the reserve to be on
> the far end of a wrap operation. It still guarantees that the space is
> available so when the wrap occurs, no wait will happen. Thus the wrap
> cannot fail which is the whole point of the exercise.
>
> Also fixed a merge failure with some comments from the original patch.
>
> For: VIZ-5115
> CC: Daniel Vetter <daniel at ffwll.ch>
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 54 +++++++++++++-----------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 73 +++++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +-
> 3 files changed, 79 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b36e064..e998a54 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -663,12 +663,12 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> 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;
>
> + /* The whole point of reserving space is to not wait! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> list_for_each_entry(target, &ring->request_list, list) {
> /*
> * The request queue is per-engine, so can contain requests
> @@ -724,12 +724,13 @@ static int logical_ring_wrap_buffer(struct drm_i915_gem_request *req)
> 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(req, rem);
> + int ret;
> +
> + /* Can't wait if space has already been reserved! */
> + WARN_ON(ringbuf->reserved_in_use);
You already do this WARN_ON in logical_ring_wait_for_space() .
>
> + ret = logical_ring_wait_for_space(req, rem);
> if (ret)
> return ret;
> }
> @@ -748,31 +749,36 @@ static int logical_ring_wrap_buffer(struct drm_i915_gem_request *req)
> static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
> {
> struct intel_ringbuffer *ringbuf = req->ringbuf;
> - 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;
> + int ret, max_bytes;
>
It would be helpful if we could flesh out the flow through the
ring_prepare functions and be more explicit about what is actually going
on. Largely this is because there is a distinct lack of documentation
for the entire ring buffer management code on top of a quite
counter-intuitive legacy design, so this is not due to your changes.
However, your changes make things even more complex and hard to
understand. So I've suggested a few comments below. Feel free to reword
or do whatever with them. It would be nice if we could be slightly more
clear about what is going on here, though. The same comments apply to
both legacy and execlist function implementations obviously.
/*
* If the request minus the reserved request size will not fit
* before the buffer wrap point we need to wrap the buffer here.
* Past this block we know for sure that the request minus
* the reserved request size will fit in the buffer without
* further wrapping.
*/
> if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> ret = logical_ring_wrap_buffer(req);
> if (unlikely(ret))
> return ret;
> + }
>
> - if(ringbuf->reserved_size) {
> - uint32_t size = ringbuf->reserved_size;
> + /*
> + * 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.
> + */
> + max_bytes = bytes + ringbuf->reserved_size;
>
> - intel_ring_reserved_space_cancel(ringbuf);
> - intel_ring_reserved_space_reserve(ringbuf, size);
> - }
> - }
> + if (unlikely(ringbuf->space < max_bytes)) {
> + /*
> + * Bytes is guaranteed to fit within the tail of the buffer,
> + * but the reserved space may push it off the end. If so then
> + * need to wait for the whole of the tail plus the reserved
> + * size. That should guarantee that the actual request
> + * (bytes) will fit between here and the end and the reserved
> + * usage will fit either in the same or at the start. Either
> + * way, if a wrap occurs it will not involve a wait and thus
> + * cannot fail.
> + */
How about replacing the above comment with:
/*
* At this point we know that the request minus the
* reserved size (= bytes) is guaranteed to fit within
* the space left before the buffer wrap point.
* However, adding the reserved request space on top of
* it will not fit within the remaining space in the
* buffer - we need to wait for space to free up.
*/
> + if (unlikely(ringbuf->tail + max_bytes + I915_RING_FREE_SPACE > ringbuf->effective_size))
> + max_bytes = ringbuf->reserved_size + I915_RING_FREE_SPACE + ringbuf->size - ringbuf->tail;
You could augment the two lines above like the following:
if (unlikely(ringbuf->tail + max_bytes +
I915_RING_FREE_SPACE > ringbuf->effective_size)) {
/*
* We may have to wrap the buffer at some point in
* order to fit the reserved request space. This
* requires us not only to wait for the reserved
* request space but on top of that also wait
* for enough space to cover the buffer space that is
* discarded during a potential buffer wrap. The lost
* buffer space is unknown at this point so we do a
* pessimistic estimation. Since we know that the
* remaining buffer space before the wrap point
* is large enough to contain the request minus the
* reserved buffer size we don't have to think about
* that (= bytes). Instead we just have to add the
* remaining reserved request size to the estimated
* buffer space lost during wrapping.
*/
int lost_wrap_space = I915_RING_FREE_SPACE + ringbuf->size -
ringbuf->tail;
max_bytes = ringbuf->reserved_size + lost_wrap_space;
}
>
> - if (unlikely(ringbuf->space < bytes)) {
> - ret = logical_ring_wait_for_space(req, bytes);
How about adding something like this at this point? :
/*
* Since we are waiting up front at this point there is
* no need to wait again in the case of a wrap. Thus,
* removing the only point of failure.
*/
> + ret = logical_ring_wait_for_space(req, max_bytes);
> if (unlikely(ret))
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index af7c12e..7c5b4c2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2121,12 +2121,12 @@ 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;
>
> + /* The whole point of reserving space is to not wait! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> list_for_each_entry(request, &ring->request_list, list) {
> space = __intel_ring_space(request->postfix, ringbuf->tail,
> ringbuf->size);
> @@ -2151,11 +2151,13 @@ 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);
> + int ret;
> +
> + /* Can't wait if space has already been reserved! */
> + WARN_ON(ringbuf->reserved_in_use);
> +
> + ret = ring_wait_for_space(ring, rem);
> if (ret)
> return ret;
> }
> @@ -2238,9 +2240,21 @@ void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> 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);
> + if (ringbuf->tail > ringbuf->reserved_tail) {
> + 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);
> + } else {
> + /*
> + * The ring was wrapped while the reserved space was in use.
> + * That means that some unknown amount of the ring tail was
> + * no-op filled and skipped. Thus simply adding the ring size
> + * to the tail and doing the above space check will not work.
> + * Rather than attempt to track how much tail was skipped,
> + * it is much simpler to say that also skipping the sanity
> + * check every once in a while is not a big issue.
> + */
> + }
>
> ringbuf->reserved_size = 0;
> ringbuf->reserved_in_use = false;
> @@ -2249,31 +2263,36 @@ void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> 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;
> + int ret, max_bytes;
>
> 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;
> + /*
> + * 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.
> + */
> + max_bytes = bytes + ringbuf->reserved_size;
>
> - intel_ring_reserved_space_cancel(ringbuf);
> - intel_ring_reserved_space_reserve(ringbuf, size);
> - }
> - }
> + if (unlikely(ringbuf->space < max_bytes)) {
> + /*
> + * Bytes is guaranteed to fit within the tail of the buffer,
> + * but the reserved space may push it off the end. If so then
> + * need to wait for the whole of the tail plus the reserved
> + * size. That should guarantee that the actual request
> + * (bytes) will fit between here and the end and the reserved
> + * usage will fit either in the same or at the start. Either
> + * way, if a wrap occurs it will not involve a wait and thus
> + * cannot fail.
> + */
> + if (unlikely(ringbuf->tail + max_bytes > ringbuf->effective_size))
> + max_bytes = ringbuf->reserved_size + I915_RING_FREE_SPACE + ringbuf->size - ringbuf->tail;
>
> - if (unlikely(ringbuf->space < bytes)) {
> - ret = ring_wait_for_space(ring, bytes);
> + ret = ring_wait_for_space(ring, max_bytes);
> if (unlikely(ret))
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 0e2bbc6..304cac4 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -473,7 +473,6 @@ static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
> * will always have sufficient room to do its stuff. The request creation
> * code calls this automatically.
> */
> -int intel_ring_reserve_space(struct drm_i915_gem_request *request);
> 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);
> @@ -482,4 +481,7 @@ 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);
>
> +/* Legacy ringbuffer specific portion of reservation code: */
> +int intel_ring_reserve_space(struct drm_i915_gem_request *request);
> +
> #endif /* _INTEL_RINGBUFFER_H_ */
>
Thanks,
Tomas
More information about the Intel-gfx
mailing list