Re: [PATCH 48/55] drm/i915: Add *_ring_begin() to request allocation

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 29, 2015 at 05:44:09PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Now that the *_ring_begin() functions no longer call the request allocation
> code, it is finally safe for the request allocation code to call *_ring_begin().
> This is important to guarantee that the space reserved for the subsequent
> i915_add_request() call does actually get reserved.
> 
> v2: Renamed functions according to review feedback (Tomas Elf).
> 
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Still has my question open from the previos round:

http://mid.gmane.org/20150323091030.GL1349@phenom.ffwll.local

Note that this isn't all that unlikely with GuC mode since there the
ringbuffer is substantially smaller (due to firmware limitations) than
what we allocate ourselves right now.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   25 +++++++++++++------------
>  drivers/gpu/drm/i915/intel_lrc.c        |   15 +++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.h        |    1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   29 ++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
>  5 files changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9f3e0717..1261792 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2680,19 +2680,20 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>  	 * 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);
> +	if (i915.enable_execlists)
> +		ret = intel_logical_ring_reserve_space(req);
> +	else
> +		ret = intel_ring_reserve_space(req);
> +	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.
> +		 */
> +		i915_gem_request_cancel(req);
> +		return ret;
> +	}
>  
>  	*req_out = ring->outstanding_lazy_request = req;
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 548c53d..e164ac0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -823,6 +823,21 @@ static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
>  	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_logical_ring_begin(request, 0);
> +}
> +
>  /**
>   * execlists_submission() - submit a batchbuffer for execution, Execlists style
>   * @dev: DRM device.
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 044c0e5..f59940a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -37,6 +37,7 @@
>  
>  /* Logical Rings */
>  int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request);
> +int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
>  void intel_logical_ring_stop(struct intel_engine_cs *ring);
>  void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
>  int intel_logical_rings_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index bb10fc2..0ba5787 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2192,24 +2192,27 @@ 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)
>  {
> -	/* 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_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)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 16fd9ba..f4633ca 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -450,6 +450,7 @@ intel_ring_get_request(struct intel_engine_cs *ring)
>  
>  #define MIN_SPACE_FOR_ADD_REQUEST	128
>  
> +int intel_ring_reserve_space(struct drm_i915_gem_request *request);
>  void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
>  void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf);
>  void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx