Re: [PATCH 07/51] drm/i915: Early alloc request in execbuff

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 25/02/2015 22:22, Daniel Vetter wrote:
On Fri, Feb 13, 2015 at 11:48:16AM +0000, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>

Start of explicit request management in the execbuffer code path. This patch
adds a call to allocate a request structure before all the actual hardware work
is done. Thus guaranteeing that all that work is tagged by a known request. At
present, nothing further is done with the request, the rest comes later in the
series.

The only noticable change is that failure to get a request (e.g. due to lack of
memory) will be caught earlier in the sequence. It now occurs right at the start
before any un-undoable work has been done.

For: VIZ-5115
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   13 ++++++++++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ca85803..61471e9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1356,7 +1356,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
  	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
  	u32 dispatch_flags;
  	int ret;
-	bool need_relocs;
+	bool need_relocs, batch_pinned = false;
if (!i915_gem_check_execbuffer(args))
  		return -EINVAL;
@@ -1525,10 +1525,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
  		if (ret)
  			goto err;
+ batch_pinned = true;
  		params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj);
  	} else
  		params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
+ /* Allocate a request for this batch buffer nice and early. */
+	ret = dev_priv->gt.alloc_request(ring, ctx);
+	if (ret)
+		goto err;
+
  	/*
  	 * Save assorted stuff away to pass through to *_submission().
  	 * NB: This data should be 'persistent' and not local as it will
@@ -1544,15 +1550,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas); +err:
  	/*
  	 * FIXME: We crucially rely upon the active tracking for the (ppgtt)
  	 * batch vma for correctness. For less ugly and less fragility this
  	 * needs to be adjusted to also track the ggtt batch vma properly as
  	 * active.
  	 */
-	if (dispatch_flags & I915_DISPATCH_SECURE)
+	if (batch_pinned)
  		i915_gem_object_ggtt_unpin(batch_obj);
-err:
+
  	/* the request owns the ref now */
  	i915_gem_context_unreference(ctx);
  	eb_destroy(eb);
This hunk here looks wrong, or maybe the context changed sufficiently
already (but I can't find that in previous patches). Why do we need to
change the pinning for the ggtt batch pin hack when allocating the request
earlier?
-Daniel

It doesn't change the behaviour. It is just coping with the extra error path. If the alloc request call fails, we need to jump past the do_execbuf() call but not past the batch unpin. Hence the 'err:' tag is moved upwards. That means it is now possible to get to the batch unpin test from an error that occurred before the pin call actually happened. Hence it is no longer safe to just test the dispatch flag. Instead an explicit 'did this get pinned yet' boolean is required.

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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux