[Intel-gfx] [PATCH v2 7/8] drm/i915: Grab execlist spinlock to avoid post-reset concurrency issues.

[Daniel Vetter]

On Fri, Oct 23, 2015 at 09:42:27AM +0100, Tvrtko Ursulin wrote:
> 
> On 19/10/15 16:32, Tomas Elf wrote:
> >Grab execlist lock when cleaning up execlist queues after GPU reset to avoid
> >concurrency problems between the context event interrupt handler and the reset
> >path immediately following a GPU reset.
> >
> >* v2 (Chris Wilson):
> >Do execlist check and use simpler form of spinlock functions.
> >
> >Signed-off-by: Tomas Elf <tomas.elf at intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index e57061a..2c7a0b7 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2753,18 +2753,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
> >  	 * are the ones that keep the context and ringbuffer backing objects
> >  	 * pinned in place.
> >  	 */
> >-	while (!list_empty(&ring->execlist_queue)) {
> >-		struct drm_i915_gem_request *submit_req;
> >
> >-		submit_req = list_first_entry(&ring->execlist_queue,
> >-				struct drm_i915_gem_request,
> >-				execlist_link);
> >-		list_del(&submit_req->execlist_link);
> >+	if (i915.enable_execlists) {
> >+		spin_lock_irq(&ring->execlist_lock);
> >+		while (!list_empty(&ring->execlist_queue)) {
> >+			struct drm_i915_gem_request *submit_req;
> >
> >-		if (submit_req->ctx != ring->default_context)
> >-			intel_lr_context_unpin(submit_req);
> >+			submit_req = list_first_entry(&ring->execlist_queue,
> >+					struct drm_i915_gem_request,
> >+					execlist_link);
> >+			list_del(&submit_req->execlist_link);
> >
> >-		i915_gem_request_unreference(submit_req);
> >+			if (submit_req->ctx != ring->default_context)
> >+				intel_lr_context_unpin(submit_req);
> >+
> >+			i915_gem_request_unreference(submit_req);
> >+		}
> >+		spin_unlock_irq(&ring->execlist_lock);
> 
> Can't this, in theory at least, end up calling drm_gem_object_unreference in
> a couple of code paths, which can sleep, while holding a spinlock?
> 
> If this cannot happen in practice for some reason it would probably be good
> to put a comment explaining it.
> 
> Or I am missing something?

It can indeed I think. Tvrtko, since I'm off to KS next week and you have
push access, can you pls push the revert if this checks out?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch