Re: [PATCH v6 13/25] drm/i915: Remove the identical implementations of request space reservation

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2016-04-26 at 21:06 +0100, Chris Wilson wrote:
> Now that we share intel_ring_begin(), reserving space for the tail of
> the request is identical between legacy/execlists and so the tautology
> can be removed. In the process, we move the reserved space tracking
> from the ringbuffer on to the request. This is to enable us to reorder
> the reserved space allocation in the next patch.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Some comments below.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  3 +++
>  drivers/gpu/drm/i915/i915_gem.c         | 21 ++++++++++------
>  drivers/gpu/drm/i915/intel_lrc.c        | 15 -----------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 44 +++------------------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 17 -------------
>  5 files changed, 19 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 444a8ea0c5c4..831da9f43324 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2278,6 +2278,9 @@ struct drm_i915_gem_request {
>  	/** Position in the ringbuffer of the end of the whole request */
>  	u32 tail;
>  
> +	/** Preallocate space in the ringbuffer for the emitting the request */
> +	u32 reserved_space;
> +
>  	/**
>  	 * Context and ring buffer related to this request
>  	 * Contexts are refcounted, so when this request is associated with a
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 71135c3ce44e..0e27484bd28a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2573,6 +2573,7 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  	struct drm_i915_private *dev_priv;
>  	struct intel_ringbuffer *ringbuf;
>  	u32 request_start;
> +	u32 reserved_tail;
>  	int ret;
>  
>  	if (WARN_ON(request == NULL))
> @@ -2587,9 +2588,10 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  	 * 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);
> +	reserved_tail = request->reserved_space;
> +	request->reserved_space = 0;
> +
>  	/*
>  	 * Emit any outstanding flushes - execbuf can fail to emit the flush
>  	 * after having emitted the batchbuffer command. Hence we need to fix
> @@ -2653,7 +2655,13 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  	intel_mark_busy(dev_priv->dev);
>  
>  	/* Sanity check that the reserved size was large enough. */
> -	intel_ring_reserved_space_end(ringbuf);
> +	ret = intel_ring_get_tail(ringbuf) - request_start;
> +	if (ret < 0)
> +		ret += ringbuf->size;
> +	WARN_ONCE(ret > reserved_tail,
> +		  "Not enough space reserved (%d bytes) "
> +		  "for adding the request (%d bytes)\n",
> +		  reserved_tail, ret);

Not sure if I'd rather prefer this to stay in an enclosed function and
not spill ringbuf stuff in the function.

>  }
>  
>  static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
> @@ -2774,17 +2782,14 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	 * to be redone if the request is not actually submitted straight
>  	 * away, e.g. because a GPU scheduler has deferred it.
>  	 */
> -	if (i915.enable_execlists)
> -		ret = intel_logical_ring_reserve_space(req);
> -	else
> -		ret = intel_ring_reserve_space(req);
> +	req->reserved_space = MIN_SPACE_FOR_ADD_REQUEST;
> +	ret = intel_ring_begin(req, 0);
>  	if (ret) {
>  		/*
>  		 * At this point, the request is fully allocated even if not
>  		 * fully prepared. Thus it can be cleaned up using the proper
>  		 * free code.
>  		 */
> -		intel_ring_reserved_space_cancel(req->ringbuf);

You could append this to the above comment that the resrved space will
be released along it.

>  		i915_gem_request_unreference(req);
>  		return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ba87c94928e7..910044cf143e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -772,21 +772,6 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>  	return 0;
>  }
>  
> -int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request)
> -{
> -	/*
> -	 * The first call merely notes the reserve request and is common for
> -	 * all back ends. The subsequent localised _begin() call actually
> -	 * ensures 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.
> -	 */
> -	intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> -
> -	return intel_ring_begin(request, 0);
> -}
> -
>  /**
>   * execlists_submission() - submit a batchbuffer for execution, Execlists style
>   * @dev: DRM device.
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1d3d2ea3e9bc..ba5946b9fa06 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2336,44 +2336,6 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  	return 0;
>  }
>  
> -int intel_ring_reserve_space(struct drm_i915_gem_request *request)
> -{
> -	/*
> -	 * The first call merely notes the reserve request and is common for
> -	 * all back ends. The subsequent localised _begin() call actually
> -	 * ensures 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.
> -	 */
> -	intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> -
> -	return intel_ring_begin(request, 0);
> -}
> -
> -void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> -{
> -	GEM_BUG_ON(ringbuf->reserved_size);
> -	ringbuf->reserved_size = size;
> -}
> -
> -void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
> -{
> -	GEM_BUG_ON(!ringbuf->reserved_size);
> -	ringbuf->reserved_size   = 0;
> -}
> -
> -void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> -{
> -	GEM_BUG_ON(!ringbuf->reserved_size);
> -	ringbuf->reserved_size   = 0;
> -}
> -
> -void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> -{
> -	GEM_BUG_ON(ringbuf->reserved_size);
> -}
> -
>  static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>  {
>  	struct intel_ringbuffer *ringbuf = req->ringbuf;
> @@ -2385,7 +2347,7 @@ static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>  		return 0;
>  
>  	/* The whole point of reserving space is to not wait! */
> -	GEM_BUG_ON(!ringbuf->reserved_size);
> +	GEM_BUG_ON(!req->reserved_space);
>  
>  	list_for_each_entry(target, &engine->request_list, list) {
>  		unsigned space;
> @@ -2420,7 +2382,7 @@ int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>  	int total_bytes, wait_bytes;
>  	bool need_wrap = false;
>  
> -	total_bytes = bytes + ringbuf->reserved_size;
> +	total_bytes = bytes + req->reserved_space;
>  
>  	if (unlikely(bytes > remain_usable)) {
>  		/*
> @@ -2436,7 +2398,7 @@ int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>  		 * and only need to effectively wait for the reserved
>  		 * size space from the start of ringbuffer.
>  		 */
> -		wait_bytes = remain_actual + ringbuf->reserved_size;
> +		wait_bytes = remain_actual + req->reserved_space;
>  	} else
>  		/* No wrapping required, just waiting. */
>  		wait_bytes = total_bytes;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 201e7752d765..038914ccc6fd 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -107,7 +107,6 @@ struct intel_ringbuffer {
>  	int space;
>  	int size;
>  	int effective_size;
> -	int reserved_size;
>  
>  	/** We track the position of the requests in the ring buffer, and
>  	 * when each is retired we increment last_retired_head as the GPU
> @@ -491,20 +490,4 @@ static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
>   */
>  #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);
> -
> -/* Legacy ringbuffer specific portion of reservation code: */
> -int intel_ring_reserve_space(struct drm_i915_gem_request *request);
> -
>  #endif /* _INTEL_RINGBUFFER_H_ */
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx