On Thu, Mar 05, 2015 at 01:57:31PM +0000, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
