[Intel-gfx] [PATCH 55/56] drm/i915: Remove 'faked' request from LRC submission

[Daniel Vetter]

On Thu, Mar 05, 2015 at 01:57:31PM +0000, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
> 
> The LRC submission code requires a request for tracking purposes. It does not
> actually require that request to 'complete' it simply uses it for keeping hold
> of reference counts on contexts and such like.
> 
> In the case where the ring buffer is completely full, the LRC code looks for a
> pending request that would free up sufficient space upon completion and waits
> for it. If no such request can be found it resorts to simply polling the free
> space count until it is big enough. This situation should only occur when the
> entire buffer is filled with the request currently being generated. I.e., the
> user is trying to submit a single piece of work that is large than the ring
> buffer itself (which should be impossible because very large batch buffers don't
> consume any more ring buffer space). Before starting to poll, a submit call is
> made to make sure that the currently queued up work in the buffer will actually
> be submtted and thus the poll will eventually succeed.
> 
> The problem here is that the 'official' request cannot be used as that could
> lead to multiple LRC submissions being tagged to a single request structure.
> Instead, the code fakes up a private request structure and uses that.
> 
> This patch moves the faked request allocation higher up in the call stack to the
> wait code itself (rather than being at the very lowest submission level). Thus
> it is now obvious where the faked request is coming from and why it is
> necessary. The patch also replaces it with a call to the official request
> allocation code rather than attempting to duplicate that code. This becomes
> especially important in the future when the request allocation changes to
> accommodate a conversion to struct fence.
> 
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>

This is only possible if you pile up tons of olr. Since your patch series
fixes this design issue by removing olr I think we can just put a WARN_ON
in here if this ever happens and bail out with -ELOSTMYMARBLES or
something. And then rip out all this complexity.

Or do I miss something important?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_lrc.c |   45 ++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 65eea51..1fa36de 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -507,23 +507,11 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
>  	if (to != ring->default_context)
>  		intel_lr_context_pin(ring, to);
>  
> -	if (!request) {
> -		/*
> -		 * If there isn't a request associated with this submission,
> -		 * create one as a temporary holder.
> -		 */
> -		request = kzalloc(sizeof(*request), GFP_KERNEL);
> -		if (request == NULL)
> -			return -ENOMEM;
> -		request->ring = ring;
> -		request->ctx = to;
> -		kref_init(&request->ref);
> -		request->uniq = dev_priv->request_uniq++;
> -		i915_gem_context_reference(request->ctx);
> -	} else {
> -		i915_gem_request_reference(request);
> -		WARN_ON(to != request->ctx);
> -	}
> +	WARN_ON(!request);
> +	WARN_ON(to != request->ctx);
> +
> +	i915_gem_request_reference(request);
> +
>  	request->tail = tail;
>  
>  	intel_runtime_pm_get(dev_priv);
> @@ -677,6 +665,7 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>  	struct intel_engine_cs *ring = ringbuf->ring;
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_request *local_req;
>  	unsigned long end;
>  	int ret;
>  
> @@ -684,8 +673,23 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>  	if (ret != -ENOSPC)
>  		return ret;
>  
> -	/* Force the context submission in case we have been skipping it */
> -	intel_logical_ring_advance_and_submit(ringbuf, ctx, NULL);
> +	/*
> +	 * Force the context submission in case we have been skipping it.
> +	 * This requires creating a place holder request so that the LRC
> +	 * submission can be tracked. Note that if this point has been
> +	 * reached then it is the current submission that is blocking the
> +	 * driver and the only course of action is to do a partial send and
> +	 * wait for it to complete.
> +	 * Note also that because there is no space left in the ring, it is
> +	 * not possible to write the request submission prologue (which does
> +	 * things like update seqno values and trigger completion interrupts).
> +	 * Thus the request cannot be submitted via i915_add_request() and
> +	 * can not be waiting on by i915_gem_wait_request().
> +	 */
> +	ret = dev_priv->gt.alloc_request(ring, ctx, &local_req);
> +	if (ret)
> +		return ret;
> +	intel_logical_ring_advance_and_submit(ringbuf, ctx, local_req);
>  
>  	/* With GEM the hangcheck timer should kick us out of the loop,
>  	 * leaving it early runs the risk of corrupting GEM state (due
> @@ -717,6 +721,9 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>  		}
>  	} while (1);
>  
> +	/* This request is now done with and can be disposed of. */
> +	i915_gem_request_unreference(local_req);
> +
>  	return ret;
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch