Re: [PATCH] drm/i915: Reserve space improvements

[Tomas Elf <tomas.elf@intel.com>]

On 29/06/2015 17:36, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@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.
>
> v2: Incorporated suggestion by David Gordon to move the wrap code
> inside the prepare function and thus allow a single combined
> wait_for_space() call rather than doing one before the wrap and
> another after. This also makes the prepare code much simpler and
> easier to follow.
>
> For: VIZ-5115
> CC: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c        | 72 +++++++++++++-------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 89 ++++++++++++++++++---------------
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  4 +-
>   3 files changed, 88 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b36e064..a41936b 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
> @@ -718,22 +718,11 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>   	execlists_context_queue(request);
>   }
>
> -static int logical_ring_wrap_buffer(struct drm_i915_gem_request *req)
> +static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
>   {
> -	struct intel_ringbuffer *ringbuf = req->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(req, rem);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
>   	virt = ringbuf->virtual_start + ringbuf->tail;
>   	rem /= 4;
>   	while (rem--)
> @@ -741,40 +730,49 @@ static int logical_ring_wrap_buffer(struct drm_i915_gem_request *req)
>
>   	ringbuf->tail = 0;
>   	intel_ring_update_space(ringbuf);
> -
> -	return 0;
>   }
>
>   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;
> -
> -	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> -		ret = logical_ring_wrap_buffer(req);
> -		if (unlikely(ret))
> -			return ret;
> +	int remain = ringbuf->effective_size - ringbuf->tail;
> +	int ret, total_bytes, wait_bytes = 0;
> +	bool need_wrap = false;
>
> -		if(ringbuf->reserved_size) {
> -			uint32_t size = ringbuf->reserved_size;
> +	if (ringbuf->reserved_in_use)
> +		total_bytes = bytes;
> +	else
> +		total_bytes = bytes + ringbuf->reserved_size;
>
> -			intel_ring_reserved_space_cancel(ringbuf);
> -			intel_ring_reserved_space_reserve(ringbuf, size);
> +	if (unlikely(bytes > remain)) {
> +		/*
> +		 * Not enough space for the basic request. So need to flush
> +		 * out the remainder and then wait for base + reserved.
> +		 */
> +		wait_bytes = remain + total_bytes;
> +		need_wrap = true;
> +	} else {
> +		if (unlikely(total_bytes > remain)) {
> +			/*
> +			 * The base request will fit but the reserved space
> +			 * falls off the end. So only need to to wait for the
> +			 * reserved size after flushing out the remainder.
> +			 */
> +			wait_bytes = remain + ringbuf->reserved_size;
> +			need_wrap = true;
> +		} else if (total_bytes > ringbuf->space) {
> +			/* No wrapping required, just waiting. */
> +			wait_bytes = total_bytes;
>   		}
>   	}
>
> -	if (unlikely(ringbuf->space < bytes)) {
> -		ret = logical_ring_wait_for_space(req, bytes);
> +	if (wait_bytes) {
> +		ret = logical_ring_wait_for_space(req, wait_bytes);

Here we're potentially waiting for an amount of space that is 
optimistically computed (ringbuf->effective_size - ringbuf->tail). If we 
want to stay consistent, that is stay pessimistic, we should use 
ringbuf->size - ringbuf->tail so that we're waiting for the biggest 
possible size here.

>   		if (unlikely(ret))
>   			return ret;
> +
> +		if (need_wrap)
> +			__wrap_ring_buffer(ringbuf);
>   	}
>
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index af7c12e..9b10019 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);
> @@ -2145,21 +2145,11 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>   	return 0;
>   }
>
> -static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
> +static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
>   {
>   	uint32_t __iomem *virt;
> -	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)
> -			return ret;
> -	}
> -
>   	virt = ringbuf->virtual_start + ringbuf->tail;
>   	rem /= 4;
>   	while (rem--)
> @@ -2167,8 +2157,6 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>
>   	ringbuf->tail = 0;
>   	intel_ring_update_space(ringbuf);
> -
> -	return 0;
>   }
>
>   int intel_ring_idle(struct intel_engine_cs *ring)
> @@ -2238,9 +2226,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,33 +2249,44 @@ 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;
> -
> -	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> -		ret = intel_wrap_ring_buffer(ring);
> -		if (unlikely(ret))
> -			return ret;
> +	int remain = ringbuf->effective_size - ringbuf->tail;
> +	int ret, total_bytes, wait_bytes = 0;
> +	bool need_wrap = false;
>
> -		if(ringbuf->reserved_size) {
> -			uint32_t size = ringbuf->reserved_size;
> +	if (ringbuf->reserved_in_use)
> +		total_bytes = bytes;
> +	else
> +		total_bytes = bytes + ringbuf->reserved_size;
>
> -			intel_ring_reserved_space_cancel(ringbuf);
> -			intel_ring_reserved_space_reserve(ringbuf, size);
> +	if (unlikely(bytes > remain)) {
> +		/*
> +		 * Not enough space for the basic request. So need to flush
> +		 * out the remainder and then wait for base + reserved.
> +		 */
> +		wait_bytes = remain + total_bytes;
> +		need_wrap = true;
> +	} else {
> +		if (unlikely(total_bytes > remain)) {
> +			/*
> +			 * The base request will fit but the reserved space
> +			 * falls off the end. So only need to to wait for the
> +			 * reserved size after flushing out the remainder.
> +			 */
> +			wait_bytes = remain + ringbuf->reserved_size;
> +			need_wrap = true;
> +		} else if (total_bytes > ringbuf->space) {
> +			/* No wrapping required, just waiting. */
> +			wait_bytes = total_bytes;
>   		}
>   	}
>
> -	if (unlikely(ringbuf->space < bytes)) {
> -		ret = ring_wait_for_space(ring, bytes);
> +	if (wait_bytes) {
> +		ret = ring_wait_for_space(ring, wait_bytes);

Same issue here as for the LRC implementation.

Thanks,
Tomas

>   		if (unlikely(ret))
>   			return ret;
> +
> +		if (need_wrap)
> +			__wrap_ring_buffer(ringbuf);
>   	}
>
>   	return 0;
> 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_ */
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx