| Message ID | 1377557469-4078-2-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 26, 2013 at 07:50:53PM -0300, Rodrigo Vivi wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > We use the request to ensure we hold a reference to the context for the > duration that it remains in use by the ring. Each request only holds a > reference to the current context, hence we emit a request after > switching contexts with the final reference to the old context. However, > the extra interrupt caused by that request is not useful (no timing > critical function will wait for the context object), instead the overhead > of servicing the IRQ shows up in some (lightweight) benchmarks. In order > to keep the useful property of using the request to manage the context > lifetime, we want to add a dummy request that is associated with the > interrupt from the subsequent real request following the batch. > > The extra interrupt was added as a side-effect of using > i915_add_request() in > > commit 112522f6789581824903f6f72082b5b841a7f0f9 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Thu May 2 16:48:07 2013 +0300 > > drm/i915: put context upon switching > > v2: Daniel convinced me that the request here was solely for context > lifetime tracking and that we have the active ref to keep the object > alive whilst the MI_SET_CONTEXT. So the only concern then is which > context should get the blame for MI_SET_CONTEXT failing. The old scheme > added a request for the old context so that any hang upto and including > the switch away would mark the old context as guilty. Now any hang here > implicates the new context. However since we have already gone through a > complete flush with the last context in its last request, and all that > lies in no-man's-land is an invalidate flush and the MI_SET_CONTEXT, we > should be safe in not unduly placing blame on the new context. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@bwidawsk.net> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> I'm somewhat new to the core GEM code, but I'm convinced by the commit message and the patch does what it says. So: Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 2d1cb10..56c9104 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2046,7 +2046,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, if (request == NULL) return -ENOMEM; - /* Record the position of the start of the request so that * should we detect the updated seqno part-way through the * GPU processing the request, we never over-estimate the diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 403309c..b6da70b 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -451,17 +451,7 @@ static int do_switch(struct i915_hw_context *to) from->obj->dirty = 1; BUG_ON(from->obj->ring != ring); - ret = i915_add_request(ring, NULL); - if (ret) { - /* Too late, we've already scheduled a context switch. - * Try to undo the change so that the hw state is - * consistent with out tracking. In case of emergency, - * scream. - */ - WARN_ON(mi_set_context(ring, from, MI_RESTORE_INHIBIT)); - return ret; - } - + /* obj is kept alive until the next request by its active ref */ i915_gem_object_unpin(from->obj); i915_gem_context_unreference(from); }