[Intel-gfx] [PATCH 1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

[Intel-gfx] [PATCH 1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

[Chris Wilson]

As the last user was eliminated in commit e21fecdcde40 ("drm/i915/gt:
Distinguish the virtual breadcrumbs from the irq breadcrumbs"), we can
remove the function. One less implementation detail creeping beyond its
scope.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 12 ------------
 drivers/gpu/drm/i915/gt/intel_lrc.h |  4 ----
 2 files changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 417f6b0c6c61..0c632f15f677 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -5882,18 +5882,6 @@ int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine,
 	return 0;
 }
 
-struct intel_engine_cs *
-intel_virtual_engine_get_sibling(struct intel_engine_cs *engine,
-				 unsigned int sibling)
-{
-	struct virtual_engine *ve = to_virtual_engine(engine);
-
-	if (sibling >= ve->num_siblings)
-		return NULL;
-
-	return ve->siblings[sibling];
-}
-
 void intel_execlists_show_requests(struct intel_engine_cs *engine,
 				   struct drm_printer *m,
 				   void (*show_request)(struct drm_printer *m,
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h b/drivers/gpu/drm/i915/gt/intel_lrc.h
index 91fd8e452d9b..c2d287f25497 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
@@ -121,10 +121,6 @@ int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine,
 				     const struct intel_engine_cs *master,
 				     const struct intel_engine_cs *sibling);
 
-struct intel_engine_cs *
-intel_virtual_engine_get_sibling(struct intel_engine_cs *engine,
-				 unsigned int sibling);
-
 bool
 intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine);
 
-- 
2.20.1

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

[Intel-gfx] [PATCH 2/7] drm/i915/gt: Protect context lifetime with RCU

[Chris Wilson]

Allow a brief period for continued access to a dead intel_context by
deferring the release of the struct until after an RCU grace period.
As we are using a dedicated slab cache for the contexts, we can defer
the release of the slab pages via RCU, with the caveat that individual
structs may be reused from the freelist within an RCU grace period. To
handle that, we have to avoid clearing members of the zombie struct.

This is required for a later patch to handle locking around virtual
requests in the signaler, as those requests may want to move between
engines and be destroyed while we are holding b->irq_lock on a physical
engine.

v2: Drop mutex_reinit(), if we never mark the mutex as destroyed we
don't need to reset the debug code, at the loss of having the mutex
debug code spot us attempting to destroy a locked mutex.
v3: As the intended use will remain strongly referenced counted, with
very little inflight access across reuse, drop the ctor.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_context.c       | 27 ++++++++++++++-----
 drivers/gpu/drm/i915/gt/intel_context_types.h |  6 +++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 52db2bde44a3..8eebb31c6c28 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -22,7 +22,7 @@ static struct i915_global_context {
 
 static struct intel_context *intel_context_alloc(void)
 {
-	return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
+	return kmem_cache_alloc(global.slab_ce, GFP_KERNEL);
 }
 
 void intel_context_free(struct intel_context *ce)
@@ -158,12 +158,12 @@ void intel_context_unpin(struct intel_context *ce)
 	/*
 	 * Once released, we may asynchronously drop the active reference.
 	 * As that may be the only reference keeping the context alive,
-	 * take an extra now so that it is not freed before we finish
+	 * hold onto RCU so that it is not freed before we finish
 	 * dereferencing it.
 	 */
-	intel_context_get(ce);
+	rcu_read_lock();
 	intel_context_active_release(ce);
-	intel_context_put(ce);
+	rcu_read_unlock();
 }
 
 static int __context_pin_state(struct i915_vma *vma)
@@ -280,8 +280,7 @@ static int __intel_context_active(struct i915_active *active)
 }
 
 void
-intel_context_init(struct intel_context *ce,
-		   struct intel_engine_cs *engine)
+intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
 {
 	GEM_BUG_ON(!engine->cops);
 	GEM_BUG_ON(!engine->gt->vm);
@@ -293,6 +292,12 @@ intel_context_init(struct intel_context *ce,
 	ce->sseu = engine->sseu;
 	ce->ring = __intel_context_ring_size(SZ_4K);
 
+	ce->wa_bb_page = 0;
+	ce->flags = 0;
+	ce->tag = 0;
+
+	memset(&ce->runtime, 0, sizeof(ce->runtime));
+
 	ewma_runtime_init(&ce->runtime.avg);
 
 	ce->vm = i915_vm_get(engine->gt->vm);
@@ -300,10 +305,16 @@ intel_context_init(struct intel_context *ce,
 	INIT_LIST_HEAD(&ce->signal_link);
 	INIT_LIST_HEAD(&ce->signals);
 
+	atomic_set(&ce->pin_count, 0);
 	mutex_init(&ce->pin_mutex);
 
+	ce->active_count = 0;
 	i915_active_init(&ce->active,
 			 __intel_context_active, __intel_context_retire);
+
+	ce->inflight = NULL;
+	ce->lrc_reg_state = NULL;
+	ce->lrc.desc = 0;
 }
 
 void intel_context_fini(struct intel_context *ce)
@@ -333,7 +344,9 @@ static struct i915_global_context global = { {
 
 int __init i915_global_context_init(void)
 {
-	global.slab_ce = KMEM_CACHE(intel_context, SLAB_HWCACHE_ALIGN);
+	global.slab_ce = KMEM_CACHE(intel_context,
+				    SLAB_HWCACHE_ALIGN |
+				    SLAB_TYPESAFE_BY_RCU);
 	if (!global.slab_ce)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 4954b0df4864..18622f1a0249 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -41,6 +41,12 @@ struct intel_context_ops {
 };
 
 struct intel_context {
+	/*
+	 * Note: Some fields may be accessed under RCU.
+	 *
+	 * Unless otherwise noted a field can safely be assumed to be protected
+	 * by strong reference counting.
+	 */
 	struct kref ref;
 
 	struct intel_engine_cs *engine;
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH 2/7] drm/i915/gt: Protect context lifetime with RCU

[Tvrtko Ursulin]

On 07/08/2020 13:54, Chris Wilson wrote:
> Allow a brief period for continued access to a dead intel_context by
> deferring the release of the struct until after an RCU grace period.
> As we are using a dedicated slab cache for the contexts, we can defer
> the release of the slab pages via RCU, with the caveat that individual
> structs may be reused from the freelist within an RCU grace period. To
> handle that, we have to avoid clearing members of the zombie struct.
> 
> This is required for a later patch to handle locking around virtual
> requests in the signaler, as those requests may want to move between
> engines and be destroyed while we are holding b->irq_lock on a physical
> engine.
> 
> v2: Drop mutex_reinit(), if we never mark the mutex as destroyed we
> don't need to reset the debug code, at the loss of having the mutex
> debug code spot us attempting to destroy a locked mutex.
> v3: As the intended use will remain strongly referenced counted, with
> very little inflight access across reuse, drop the ctor.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_context.c       | 27 ++++++++++++++-----
>   drivers/gpu/drm/i915/gt/intel_context_types.h |  6 +++++
>   2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 52db2bde44a3..8eebb31c6c28 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -22,7 +22,7 @@ static struct i915_global_context {
>   
>   static struct intel_context *intel_context_alloc(void)
>   {
> -	return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
> +	return kmem_cache_alloc(global.slab_ce, GFP_KERNEL);
>   }
>   
>   void intel_context_free(struct intel_context *ce)
> @@ -158,12 +158,12 @@ void intel_context_unpin(struct intel_context *ce)
>   	/*
>   	 * Once released, we may asynchronously drop the active reference.
>   	 * As that may be the only reference keeping the context alive,
> -	 * take an extra now so that it is not freed before we finish
> +	 * hold onto RCU so that it is not freed before we finish
>   	 * dereferencing it.
>   	 */
> -	intel_context_get(ce);
> +	rcu_read_lock();
>   	intel_context_active_release(ce);
> -	intel_context_put(ce);
> +	rcu_read_unlock();

Strong reference here couldn't stay?

Regards,

Tvrtko

>   }
>   
>   static int __context_pin_state(struct i915_vma *vma)
> @@ -280,8 +280,7 @@ static int __intel_context_active(struct i915_active *active)
>   }
>   
>   void
> -intel_context_init(struct intel_context *ce,
> -		   struct intel_engine_cs *engine)
> +intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
>   {
>   	GEM_BUG_ON(!engine->cops);
>   	GEM_BUG_ON(!engine->gt->vm);
> @@ -293,6 +292,12 @@ intel_context_init(struct intel_context *ce,
>   	ce->sseu = engine->sseu;
>   	ce->ring = __intel_context_ring_size(SZ_4K);
>   
> +	ce->wa_bb_page = 0;
> +	ce->flags = 0;
> +	ce->tag = 0;
> +
> +	memset(&ce->runtime, 0, sizeof(ce->runtime));
> +
>   	ewma_runtime_init(&ce->runtime.avg);
>   
>   	ce->vm = i915_vm_get(engine->gt->vm);
> @@ -300,10 +305,16 @@ intel_context_init(struct intel_context *ce,
>   	INIT_LIST_HEAD(&ce->signal_link);
>   	INIT_LIST_HEAD(&ce->signals);
>   
> +	atomic_set(&ce->pin_count, 0);
>   	mutex_init(&ce->pin_mutex);
>   
> +	ce->active_count = 0;
>   	i915_active_init(&ce->active,
>   			 __intel_context_active, __intel_context_retire);
> +
> +	ce->inflight = NULL;
> +	ce->lrc_reg_state = NULL;
> +	ce->lrc.desc = 0;
>   }
>   
>   void intel_context_fini(struct intel_context *ce)
> @@ -333,7 +344,9 @@ static struct i915_global_context global = { {
>   
>   int __init i915_global_context_init(void)
>   {
> -	global.slab_ce = KMEM_CACHE(intel_context, SLAB_HWCACHE_ALIGN);
> +	global.slab_ce = KMEM_CACHE(intel_context,
> +				    SLAB_HWCACHE_ALIGN |
> +				    SLAB_TYPESAFE_BY_RCU);
>   	if (!global.slab_ce)
>   		return -ENOMEM;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 4954b0df4864..18622f1a0249 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -41,6 +41,12 @@ struct intel_context_ops {
>   };
>   
>   struct intel_context {
> +	/*
> +	 * Note: Some fields may be accessed under RCU.
> +	 *
> +	 * Unless otherwise noted a field can safely be assumed to be protected
> +	 * by strong reference counting.
> +	 */
>   	struct kref ref;
>   
>   	struct intel_engine_cs *engine;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915/gt: Protect context lifetime with RCU

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-08-07 15:10:32)
> 
> On 07/08/2020 13:54, Chris Wilson wrote:
> > Allow a brief period for continued access to a dead intel_context by
> > deferring the release of the struct until after an RCU grace period.
> > As we are using a dedicated slab cache for the contexts, we can defer
> > the release of the slab pages via RCU, with the caveat that individual
> > structs may be reused from the freelist within an RCU grace period. To
> > handle that, we have to avoid clearing members of the zombie struct.
> > 
> > This is required for a later patch to handle locking around virtual
> > requests in the signaler, as those requests may want to move between
> > engines and be destroyed while we are holding b->irq_lock on a physical
> > engine.
> > 
> > v2: Drop mutex_reinit(), if we never mark the mutex as destroyed we
> > don't need to reset the debug code, at the loss of having the mutex
> > debug code spot us attempting to destroy a locked mutex.
> > v3: As the intended use will remain strongly referenced counted, with
> > very little inflight access across reuse, drop the ctor.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_context.c       | 27 ++++++++++++++-----
> >   drivers/gpu/drm/i915/gt/intel_context_types.h |  6 +++++
> >   2 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 52db2bde44a3..8eebb31c6c28 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -22,7 +22,7 @@ static struct i915_global_context {
> >   
> >   static struct intel_context *intel_context_alloc(void)
> >   {
> > -     return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
> > +     return kmem_cache_alloc(global.slab_ce, GFP_KERNEL);
> >   }
> >   
> >   void intel_context_free(struct intel_context *ce)
> > @@ -158,12 +158,12 @@ void intel_context_unpin(struct intel_context *ce)
> >       /*
> >        * Once released, we may asynchronously drop the active reference.
> >        * As that may be the only reference keeping the context alive,
> > -      * take an extra now so that it is not freed before we finish
> > +      * hold onto RCU so that it is not freed before we finish
> >        * dereferencing it.
> >        */
> > -     intel_context_get(ce);
> > +     rcu_read_lock();
> >       intel_context_active_release(ce);
> > -     intel_context_put(ce);
> > +     rcu_read_unlock();
> 
> Strong reference here couldn't stay?

It really shouldn't be here in the first place...

But no, it doesn't have to change in this patch.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/gt: Protect context lifetime with RCU

[Chris Wilson]

Allow a brief period for continued access to a dead intel_context by
deferring the release of the struct until after an RCU grace period.
As we are using a dedicated slab cache for the contexts, we can defer
the release of the slab pages via RCU, with the caveat that individual
structs may be reused from the freelist within an RCU grace period. To
handle that, we have to avoid clearing members of the zombie struct.

This is required for a later patch to handle locking around virtual
requests in the signaler, as those requests may want to move between
engines and be destroyed while we are holding b->irq_lock on a physical
engine.

v2: Drop mutex_reinit(), if we never mark the mutex as destroyed we
don't need to reset the debug code, at the loss of having the mutex
debug code spot us attempting to destroy a locked mutex.
v3: As the intended use will remain strongly referenced counted, with
very little inflight access across reuse, drop the ctor.
v4: Drop the unrequired change to remove the temporary reference around
dropping the active context, and add back some more missing ctor
operations.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_context.c       | 27 +++++++++++++++----
 drivers/gpu/drm/i915/gt/intel_context_types.h |  6 +++++
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 52db2bde44a3..25690b7fe500 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -22,7 +22,7 @@ static struct i915_global_context {
 
 static struct intel_context *intel_context_alloc(void)
 {
-	return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
+	return kmem_cache_alloc(global.slab_ce, GFP_KERNEL);
 }
 
 void intel_context_free(struct intel_context *ce)
@@ -280,8 +280,7 @@ static int __intel_context_active(struct i915_active *active)
 }
 
 void
-intel_context_init(struct intel_context *ce,
-		   struct intel_engine_cs *engine)
+intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
 {
 	GEM_BUG_ON(!engine->cops);
 	GEM_BUG_ON(!engine->gt->vm);
@@ -291,19 +290,35 @@ intel_context_init(struct intel_context *ce,
 	ce->engine = engine;
 	ce->ops = engine->cops;
 	ce->sseu = engine->sseu;
-	ce->ring = __intel_context_ring_size(SZ_4K);
+
+	ce->wa_bb_page = 0;
+	ce->flags = 0;
+	ce->tag = 0;
+
+	memset(&ce->runtime, 0, sizeof(ce->runtime));
 
 	ewma_runtime_init(&ce->runtime.avg);
 
 	ce->vm = i915_vm_get(engine->gt->vm);
+	ce->gem_context = NULL;
+
+	ce->ring = __intel_context_ring_size(SZ_4K);
+	ce->timeline = NULL;
+	ce->state = NULL;
 
 	INIT_LIST_HEAD(&ce->signal_link);
 	INIT_LIST_HEAD(&ce->signals);
 
+	atomic_set(&ce->pin_count, 0);
 	mutex_init(&ce->pin_mutex);
 
+	ce->active_count = 0;
 	i915_active_init(&ce->active,
 			 __intel_context_active, __intel_context_retire);
+
+	ce->inflight = NULL;
+	ce->lrc_reg_state = NULL;
+	ce->lrc.desc = 0;
 }
 
 void intel_context_fini(struct intel_context *ce)
@@ -333,7 +348,9 @@ static struct i915_global_context global = { {
 
 int __init i915_global_context_init(void)
 {
-	global.slab_ce = KMEM_CACHE(intel_context, SLAB_HWCACHE_ALIGN);
+	global.slab_ce = KMEM_CACHE(intel_context,
+				    SLAB_HWCACHE_ALIGN |
+				    SLAB_TYPESAFE_BY_RCU);
 	if (!global.slab_ce)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 4954b0df4864..18622f1a0249 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -41,6 +41,12 @@ struct intel_context_ops {
 };
 
 struct intel_context {
+	/*
+	 * Note: Some fields may be accessed under RCU.
+	 *
+	 * Unless otherwise noted a field can safely be assumed to be protected
+	 * by strong reference counting.
+	 */
 	struct kref ref;
 
 	struct intel_engine_cs *engine;
-- 
2.20.1

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

[Intel-gfx] [PATCH 3/7] drm/i915/gt: Defer enabling the breadcrumb interrupt to after submission

[Chris Wilson]

Move the register slow register write and readback from out of the
critical path for execlists submission and delay it until the following
worker, shaving off around 200us. Note that the same signal_irq_work() is
allowed to run concurrently on each CPU (but it will only be queued once,
once running though it can be requeued and reexecuted) so we have to
remember to lock the global interactions as we cannot rely on the
signal_irq_work() itself providing the serialisation (in constrast to a
tasklet).

By pushing the arm/disarm into the central signaling worker we can close
the race for disarming the interrupt (and dropping its associated
GT wakeref) on parking the engine. If we loose the race, that GT wakeref
may be held indefinitely, preventing the machine from sleeping while
the GPU is ostensibly idle.

v2: Move the self-arming parking of the signal_irq_work to a flush of
the irq-work from intel_breadcrumbs_park().

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2271
Fixes: dfeba1ae34c8 ("drm/i915/gt: Hold context/request reference while breadcrumbs are active")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 106 +++++++++++++-------
 1 file changed, 67 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index d8b206e53660..6c321419441f 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -30,18 +30,21 @@
 #include "i915_trace.h"
 #include "intel_breadcrumbs.h"
 #include "intel_context.h"
+#include "intel_engine_pm.h"
 #include "intel_gt_pm.h"
 #include "intel_gt_requests.h"
 
-static void irq_enable(struct intel_engine_cs *engine)
+static bool irq_enable(struct intel_engine_cs *engine)
 {
 	if (!engine->irq_enable)
-		return;
+		return false;
 
 	/* Caller disables interrupts */
 	spin_lock(&engine->gt->irq_lock);
 	engine->irq_enable(engine);
 	spin_unlock(&engine->gt->irq_lock);
+
+	return true;
 }
 
 static void irq_disable(struct intel_engine_cs *engine)
@@ -57,12 +60,11 @@ static void irq_disable(struct intel_engine_cs *engine)
 
 static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
 {
-	lockdep_assert_held(&b->irq_lock);
-
-	if (!b->irq_engine || b->irq_armed)
-		return;
-
-	if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
+	/*
+	 * Since we are waiting on a request, the GPU should be busy
+	 * and should have its own rpm reference.
+	 */
+	if (GEM_WARN_ON(!intel_gt_pm_get_if_awake(b->irq_engine->gt)))
 		return;
 
 	/*
@@ -73,25 +75,24 @@ static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
 	 */
 	WRITE_ONCE(b->irq_armed, true);
 
-	/*
-	 * Since we are waiting on a request, the GPU should be busy
-	 * and should have its own rpm reference. This is tracked
-	 * by i915->gt.awake, we can forgo holding our own wakref
-	 * for the interrupt as before i915->gt.awake is released (when
-	 * the driver is idle) we disarm the breadcrumbs.
-	 */
-
-	if (!b->irq_enabled++)
-		irq_enable(b->irq_engine);
+	/* Requests may have completed before we could enable the interrupt. */
+	if (!b->irq_enabled++ && irq_enable(b->irq_engine))
+		irq_work_queue(&b->irq_work);
 }
 
-static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
+static void intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
 {
-	lockdep_assert_held(&b->irq_lock);
-
-	if (!b->irq_engine || !b->irq_armed)
+	if (!b->irq_engine)
 		return;
 
+	spin_lock(&b->irq_lock);
+	if (!b->irq_armed)
+		__intel_breadcrumbs_arm_irq(b);
+	spin_unlock(&b->irq_lock);
+}
+
+static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
+{
 	GEM_BUG_ON(!b->irq_enabled);
 	if (!--b->irq_enabled)
 		irq_disable(b->irq_engine);
@@ -105,8 +106,6 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
 {
 	intel_context_get(ce);
 	list_add_tail(&ce->signal_link, &b->signalers);
-	if (list_is_first(&ce->signal_link, &b->signalers))
-		__intel_breadcrumbs_arm_irq(b);
 }
 
 static void remove_signaling_context(struct intel_breadcrumbs *b,
@@ -197,7 +196,32 @@ static void signal_irq_work(struct irq_work *work)
 
 	spin_lock(&b->irq_lock);
 
-	if (list_empty(&b->signalers))
+	/*
+	 * Keep the irq armed until the interrupt after all listeners are gone.
+	 *
+	 * Enabling/disabling the interrupt is rather costly, roughly a couple
+	 * of hundred microseconds. If we are proactive and enable/disable
+	 * the interrupt around every request that wants a breadcrumb, we
+	 * quickly drown in the extra orders of magnitude of latency imposed
+	 * on request submission.
+	 *
+	 * So we try to be lazy, and keep the interrupts enabled until no
+	 * more listeners appear within a breadcrumb interrupt interval (that
+	 * is until a request completes that no one cares about). The
+	 * observation is that listeners come in batches, and will often
+	 * listen to a bunch of requests in succession. Though note on icl+,
+	 * interrupts are always enabled due to concerns with rc6 being
+	 * dysfunctional with per-engine interrupt masking.
+	 *
+	 * We also try to avoid raising too many interrupts, as they may
+	 * be generated by userspace batches and it is unfortunately rather
+	 * too easy to drown the CPU under a flood of GPU interrupts. Thus
+	 * whenever no one appears to be listening, we turn off the interrupts.
+	 * Fewer interrupts should conserve power -- at the very least, fewer
+	 * interrupt draw less ire from other users of the system and tools
+	 * like powertop.
+	 */
+	if (b->irq_armed && list_empty(&b->signalers))
 		__intel_breadcrumbs_disarm_irq(b);
 
 	list_splice_init(&b->signaled_requests, &signal);
@@ -251,6 +275,9 @@ static void signal_irq_work(struct irq_work *work)
 
 		i915_request_put(rq);
 	}
+
+	if (!b->irq_armed && !list_empty(&b->signalers))
+		intel_breadcrumbs_arm_irq(b);
 }
 
 struct intel_breadcrumbs *
@@ -292,21 +319,19 @@ void intel_breadcrumbs_reset(struct intel_breadcrumbs *b)
 
 void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
 {
-	unsigned long flags;
-
-	if (!READ_ONCE(b->irq_armed))
-		return;
-
-	spin_lock_irqsave(&b->irq_lock, flags);
-	__intel_breadcrumbs_disarm_irq(b);
-	spin_unlock_irqrestore(&b->irq_lock, flags);
-
-	if (!list_empty(&b->signalers))
-		irq_work_queue(&b->irq_work);
+	/* Kick the work once more to drain the signalers */
+	irq_work_sync(&b->irq_work);
+	while (unlikely(READ_ONCE(b->irq_armed))) {
+		local_irq_disable();
+		signal_irq_work(&b->irq_work);
+		local_irq_enable();
+		cond_resched();
+	}
 }
 
 void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
 {
+	irq_work_sync(&b->irq_work);
 	kfree(b);
 }
 
@@ -362,9 +387,12 @@ static void insert_breadcrumb(struct i915_request *rq,
 	GEM_BUG_ON(!check_signal_order(ce, rq));
 	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 
-	/* Check after attaching to irq, interrupt may have already fired. */
-	if (__request_completed(rq))
-		irq_work_queue(&b->irq_work);
+	/*
+	 * Defer enabling the interrupt to after HW submission and recheck
+	 * the request as it may have completed and raised the interrupt as
+	 * we were attaching it into the lists.
+	 */
+	irq_work_queue(&b->irq_work);
 }
 
 bool i915_request_enable_breadcrumb(struct i915_request *rq)
-- 
2.20.1

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

[Intel-gfx] [PATCH 4/7] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock

[Chris Wilson]

Make b->signaled_requests a lockless-list so that we can manipulate it
outside of the b->irq_lock.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 34 ++++++++++++-------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |  2 +-
 drivers/gpu/drm/i915/i915_request.h           |  6 +++-
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 6c321419441f..4ff2cf493cca 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -173,26 +173,34 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
 		intel_engine_add_retire(b->irq_engine, tl);
 }
 
-static bool __signal_request(struct i915_request *rq, struct list_head *signals)
+static bool __signal_request(struct i915_request *rq)
 {
-	clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
-
 	if (!__dma_fence_signal(&rq->fence)) {
 		i915_request_put(rq);
 		return false;
 	}
 
-	list_add_tail(&rq->signal_link, signals);
 	return true;
 }
 
+static struct llist_node *
+slist_add(struct llist_node *node, struct llist_node *head)
+{
+	node->next = head;
+	return node;
+}
+
 static void signal_irq_work(struct irq_work *work)
 {
 	struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
 	const ktime_t timestamp = ktime_get();
+	struct llist_node *signal, *sn;
 	struct intel_context *ce, *cn;
 	struct list_head *pos, *next;
-	LIST_HEAD(signal);
+
+	signal = NULL;
+	if (unlikely(!llist_empty(&b->signaled_requests)))
+		signal = llist_del_all(&b->signaled_requests);
 
 	spin_lock(&b->irq_lock);
 
@@ -224,8 +232,6 @@ static void signal_irq_work(struct irq_work *work)
 	if (b->irq_armed && list_empty(&b->signalers))
 		__intel_breadcrumbs_disarm_irq(b);
 
-	list_splice_init(&b->signaled_requests, &signal);
-
 	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
 		GEM_BUG_ON(list_empty(&ce->signals));
 
@@ -242,7 +248,10 @@ static void signal_irq_work(struct irq_work *work)
 			 * spinlock as the callback chain may end up adding
 			 * more signalers to the same context or engine.
 			 */
-			__signal_request(rq, &signal);
+			clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+			if (__signal_request(rq))
+				/* We own signal_node now, xfer to local list */
+				signal = slist_add(&rq->signal_node, signal);
 		}
 
 		/*
@@ -262,9 +271,9 @@ static void signal_irq_work(struct irq_work *work)
 
 	spin_unlock(&b->irq_lock);
 
-	list_for_each_safe(pos, next, &signal) {
+	llist_for_each_safe(signal, sn, signal) {
 		struct i915_request *rq =
-			list_entry(pos, typeof(*rq), signal_link);
+			llist_entry(signal, typeof(*rq), signal_node);
 		struct list_head cb_list;
 
 		spin_lock(&rq->lock);
@@ -291,7 +300,7 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
 
 	spin_lock_init(&b->irq_lock);
 	INIT_LIST_HEAD(&b->signalers);
-	INIT_LIST_HEAD(&b->signaled_requests);
+	init_llist_head(&b->signaled_requests);
 
 	init_irq_work(&b->irq_work, signal_irq_work);
 
@@ -352,7 +361,8 @@ static void insert_breadcrumb(struct i915_request *rq,
 	 * its signal completion.
 	 */
 	if (__request_completed(rq)) {
-		if (__signal_request(rq, &b->signaled_requests))
+		if (__signal_request(rq) &&
+		    llist_add(&rq->signal_node, &b->signaled_requests))
 			irq_work_queue(&b->irq_work);
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
index 8e53b9942695..3fa19820b37a 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -35,7 +35,7 @@ struct intel_breadcrumbs {
 	struct intel_engine_cs *irq_engine;
 
 	struct list_head signalers;
-	struct list_head signaled_requests;
+	struct llist_head signaled_requests;
 
 	struct irq_work irq_work; /* for use from inside irq_lock */
 
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 16b721080195..874af6db6103 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -176,7 +176,11 @@ struct i915_request {
 	struct intel_context *context;
 	struct intel_ring *ring;
 	struct intel_timeline __rcu *timeline;
-	struct list_head signal_link;
+
+	union {
+		struct list_head signal_link;
+		struct llist_node signal_node;
+	};
 
 	/*
 	 * The rcu epoch of when this request was allocated. Used to judiciously
-- 
2.20.1

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

[Intel-gfx] [PATCH 5/7] drm/i915/gt: Don't cancel the interrupt shadow too early

[Chris Wilson]

We currently want to keep the interrupt enabled until the interrupt after
which we have no more work to do. This heuristic was broken by us
kicking the irq-work on adding a completed request without attaching a
signaler -- hence it appearing to the irq-worker that an interrupt had
fired when we were idle.

Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 4ff2cf493cca..a077ef3d02b4 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -229,7 +229,7 @@ static void signal_irq_work(struct irq_work *work)
 	 * interrupt draw less ire from other users of the system and tools
 	 * like powertop.
 	 */
-	if (b->irq_armed && list_empty(&b->signalers))
+	if (!signal && b->irq_armed && list_empty(&b->signalers))
 		__intel_breadcrumbs_disarm_irq(b);
 
 	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
-- 
2.20.1

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

[Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Chris Wilson]

As we funnel more and more contexts into the breadcrumbs on an engine,
the hold time of b->irq_lock grows. As we may then contend with the
b->irq_lock during request submission, this increases the burden upon
the engine->active.lock and so directly impacts both our execution
latency and client latency. If we split the b->irq_lock by introducing a
per-context spinlock to manage the signalers within a context, we then
only need the b->irq_lock for enabling/disabling the interrupt and can
avoid taking the lock for walking the list of contexts within the signal
worker. Even with the current setup, this greatly reduces the number of
times we have to take and fight for b->irq_lock.

Furthermore, this closes the race between enabling the signaling context
while it is in the process of being signaled and removed:

<4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
<4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
<4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
<4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
<4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
<4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
<4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
<4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
<4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
<4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
<4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
<4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
<4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
<4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
<4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
<4>[  416.208675] PKRU: 55555554
<4>[  416.208677] Call Trace:
<4>[  416.208679]  <IRQ>
<4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
<4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
<4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
<4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
<4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
<4>[  416.208954]  __do_softirq+0xdf/0x498
<4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
<4>[  416.208964]  asm_call_on_stack+0xf/0x20
<4>[  416.208966]  </IRQ>
<4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
<4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
<4>[  416.208976]  common_interrupt+0xf7/0x260
<4>[  416.208980]  asm_common_interrupt+0x1e/0x40
<4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
<4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
<4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
<4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
<4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
<4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
<4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
<4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
<4>[  416.209012]  cpuidle_enter+0x24/0x40
<4>[  416.209016]  do_idle+0x22f/0x2d0
<4>[  416.209022]  cpu_startup_entry+0x14/0x20
<4>[  416.209025]  start_secondary+0x158/0x1a0
<4>[  416.209030]  secondary_startup_64+0xa4/0xb0
<4>[  416.209039] irq event stamp: 10186977
<4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
<4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
<4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
<4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20

<4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
<4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
<4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
<4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
<4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
<4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
<4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
<4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
<4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
<4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
<4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
<4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
<4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
<4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
<4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
<4>[  416.209317] PKRU: 55555554
<4>[  416.209317] Call Trace:
<4>[  416.209317]  <IRQ>
<4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
<4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]

This is caused by virtual engines where although we take the breadcrumb
lock on each of the active engines, they may be different engines on
different requests, It turns out that the b->irq_lock was not a
sufficient proxy for the engine->active.lock in the case of more than
one request, so introduce an explicit lock around ce->signals.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2276
Fixes: f94343d0a622 ("drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs")
Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 173 +++++++++++-------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
 drivers/gpu/drm/i915/gt/intel_context.c       |   3 +-
 drivers/gpu/drm/i915/gt/intel_context_types.h |   9 +-
 4 files changed, 114 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index a077ef3d02b4..e28efc1bb41d 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
 	intel_gt_pm_put_async(b->irq_engine->gt);
 }
 
+static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
+{
+	spin_lock(&b->irq_lock);
+	if (b->irq_armed)
+		__intel_breadcrumbs_disarm_irq(b);
+	spin_unlock(&b->irq_lock);
+}
+
 static void add_signaling_context(struct intel_breadcrumbs *b,
 				  struct intel_context *ce)
 {
-	intel_context_get(ce);
-	list_add_tail(&ce->signal_link, &b->signalers);
+	lockdep_assert_held(&b->signalers_lock);
+	list_add_rcu(&ce->signal_link, &b->signalers);
 }
 
 static void remove_signaling_context(struct intel_breadcrumbs *b,
 				     struct intel_context *ce)
 {
-	list_del(&ce->signal_link);
-	intel_context_put(ce);
+	spin_lock(&b->signalers_lock);
+	list_del_rcu(&ce->signal_link);
+	spin_unlock(&b->signalers_lock);
 }
 
 static inline bool __request_completed(const struct i915_request *rq)
@@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
 	struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
 	const ktime_t timestamp = ktime_get();
 	struct llist_node *signal, *sn;
-	struct intel_context *ce, *cn;
-	struct list_head *pos, *next;
+	struct intel_context *ce;
 
 	signal = NULL;
 	if (unlikely(!llist_empty(&b->signaled_requests)))
 		signal = llist_del_all(&b->signaled_requests);
 
-	spin_lock(&b->irq_lock);
-
 	/*
 	 * Keep the irq armed until the interrupt after all listeners are gone.
 	 *
@@ -230,10 +236,32 @@ static void signal_irq_work(struct irq_work *work)
 	 * like powertop.
 	 */
 	if (!signal && b->irq_armed && list_empty(&b->signalers))
-		__intel_breadcrumbs_disarm_irq(b);
+		intel_breadcrumbs_disarm_irq(b);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+		struct list_head *pos, *next;
+		bool release = false;
+
+		/*
+		 * Since the signal_irq_worker may in fact be running on
+		 * different cores simultaneously, just skip over the other
+		 * thread if it is in the middle of processing the signals.
+		 * We will then advance to the next context, and the other
+		 * core will then skip over us, splitting the work between
+		 * the multiple cores that just happen to processing the
+		 * worker simultaneously.
+		 *
+		 * The other use of ce->signal_lock to add new requests
+		 * to be signaled will always kick the worker afterwards
+		 * ensuring that ce->signals will always be processed at
+		 * least once (even after being skipped).
+		 */
+		if (!spin_trylock(&ce->signal_lock))
+			continue;
 
-	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
-		GEM_BUG_ON(list_empty(&ce->signals));
+		if (list_empty(&ce->signals))
+			goto unlock;
 
 		list_for_each_safe(pos, next, &ce->signals) {
 			struct i915_request *rq =
@@ -265,11 +293,16 @@ static void signal_irq_work(struct irq_work *work)
 			if (&ce->signals == pos) { /* now empty */
 				add_retire(b, ce->timeline);
 				remove_signaling_context(b, ce);
+				release = true;
 			}
 		}
-	}
 
-	spin_unlock(&b->irq_lock);
+unlock:
+		spin_unlock(&ce->signal_lock);
+		if (release)
+			intel_context_put(ce);
+	}
+	rcu_read_unlock();
 
 	llist_for_each_safe(signal, sn, signal) {
 		struct i915_request *rq =
@@ -298,14 +331,15 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
 	if (!b)
 		return NULL;
 
-	spin_lock_init(&b->irq_lock);
+	b->irq_engine = irq_engine;
+
+	spin_lock_init(&b->signalers_lock);
 	INIT_LIST_HEAD(&b->signalers);
 	init_llist_head(&b->signaled_requests);
 
+	spin_lock_init(&b->irq_lock);
 	init_irq_work(&b->irq_work, signal_irq_work);
 
-	b->irq_engine = irq_engine;
-
 	return b;
 }
 
@@ -344,9 +378,9 @@ void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
 	kfree(b);
 }
 
-static void insert_breadcrumb(struct i915_request *rq,
-			      struct intel_breadcrumbs *b)
+static void insert_breadcrumb(struct i915_request *rq)
 {
+	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
 	struct intel_context *ce = rq->context;
 	struct list_head *pos;
 
@@ -368,7 +402,33 @@ static void insert_breadcrumb(struct i915_request *rq,
 	}
 
 	if (list_empty(&ce->signals)) {
+		/*
+		 * rq->engine is locked by rq->engine->active.lock. That
+		 * however is not known until after rq->engine has been
+		 * dereferenced and the lock acquired. Hence we acquire the
+		 * lock and then validate that rq->engine still matches the
+		 * lock we hold for it.
+		 *
+		 * Here, we are using the breadcrumb lock as a proxy for the
+		 * rq->engine->active.lock, and we know that since the
+		 * breadcrumb will be serialised within i915_request_submit
+		 * the engine cannot change while active as long as we hold
+		 * the breadcrumb lock on that engine.
+		 *
+		 * From the dma_fence_enable_signaling() path, we are outside
+		 * of the request submit/unsubmit path, and so we must be more
+		 * careful to acquire the right lock.
+		 */
+		intel_context_get(ce);
+		spin_lock(&b->signalers_lock);
+		while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
+			spin_unlock(&b->signalers_lock);
+			b = READ_ONCE(rq->engine)->breadcrumbs;
+			spin_lock(&b->signalers_lock);
+		}
 		add_signaling_context(b, ce);
+		spin_unlock(&b->signalers_lock);
+
 		pos = &ce->signals;
 	} else {
 		/*
@@ -407,7 +467,7 @@ static void insert_breadcrumb(struct i915_request *rq,
 
 bool i915_request_enable_breadcrumb(struct i915_request *rq)
 {
-	struct intel_breadcrumbs *b;
+	struct intel_context *ce = rq->context;
 
 	/* Serialises with i915_request_retire() using rq->lock */
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
@@ -422,67 +482,37 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
 	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
 		return true;
 
-	/*
-	 * rq->engine is locked by rq->engine->active.lock. That however
-	 * is not known until after rq->engine has been dereferenced and
-	 * the lock acquired. Hence we acquire the lock and then validate
-	 * that rq->engine still matches the lock we hold for it.
-	 *
-	 * Here, we are using the breadcrumb lock as a proxy for the
-	 * rq->engine->active.lock, and we know that since the breadcrumb
-	 * will be serialised within i915_request_submit/i915_request_unsubmit,
-	 * the engine cannot change while active as long as we hold the
-	 * breadcrumb lock on that engine.
-	 *
-	 * From the dma_fence_enable_signaling() path, we are outside of the
-	 * request submit/unsubmit path, and so we must be more careful to
-	 * acquire the right lock.
-	 */
-	b = READ_ONCE(rq->engine)->breadcrumbs;
-	spin_lock(&b->irq_lock);
-	while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
-		spin_unlock(&b->irq_lock);
-		b = READ_ONCE(rq->engine)->breadcrumbs;
-		spin_lock(&b->irq_lock);
-	}
-
-	/*
-	 * Now that we are finally serialised with request submit/unsubmit,
-	 * [with b->irq_lock] and with i915_request_retire() [via checking
-	 * SIGNALED with rq->lock] confirm the request is indeed active. If
-	 * it is no longer active, the breadcrumb will be attached upon
-	 * i915_request_submit().
-	 */
+	spin_lock(&ce->signal_lock);
 	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
-		insert_breadcrumb(rq, b);
-
-	spin_unlock(&b->irq_lock);
+		insert_breadcrumb(rq);
+	spin_unlock(&ce->signal_lock);
 
 	return true;
 }
 
 void i915_request_cancel_breadcrumb(struct i915_request *rq)
 {
-	struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
+	struct intel_context *ce = rq->context;
+	bool release = false;
 
-	/*
-	 * We must wait for b->irq_lock so that we know the interrupt handler
-	 * has released its reference to the intel_context and has completed
-	 * the DMA_FENCE_FLAG_SIGNALED_BIT/I915_FENCE_FLAG_SIGNAL dance (if
-	 * required).
-	 */
-	spin_lock(&b->irq_lock);
+	if (!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
+		return;
+
+	spin_lock(&ce->signal_lock);
 	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
-		struct intel_context *ce = rq->context;
+		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 
 		list_del(&rq->signal_link);
-		if (list_empty(&ce->signals))
-			remove_signaling_context(b, ce);
+		if (list_empty(&ce->signals)) {
+			remove_signaling_context(rq->engine->breadcrumbs, ce);
+			release = true;
+		}
 
-		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 		i915_request_put(rq);
 	}
-	spin_unlock(&b->irq_lock);
+	spin_unlock(&ce->signal_lock);
+	if (release)
+		intel_context_put(ce);
 }
 
 static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
@@ -492,18 +522,19 @@ static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
 
 	drm_printf(p, "Signals:\n");
 
-	spin_lock_irq(&b->irq_lock);
-	list_for_each_entry(ce, &b->signalers, signal_link) {
-		list_for_each_entry(rq, &ce->signals, signal_link) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+		spin_lock_irq(&ce->signal_lock);
+		list_for_each_entry(rq, &ce->signals, signal_link)
 			drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
 				   rq->fence.context, rq->fence.seqno,
 				   i915_request_completed(rq) ? "!" :
 				   i915_request_started(rq) ? "*" :
 				   "",
 				   jiffies_to_msecs(jiffies - rq->emitted_jiffies));
-		}
+		spin_unlock_irq(&ce->signal_lock);
 	}
-	spin_unlock_irq(&b->irq_lock);
+	rcu_read_unlock();
 }
 
 void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
index 3fa19820b37a..a74bb3062bd8 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -29,18 +29,16 @@
  * the overhead of waking that client is much preferred.
  */
 struct intel_breadcrumbs {
-	spinlock_t irq_lock; /* protects the lists used in hardirq context */
-
 	/* Not all breadcrumbs are attached to physical HW */
 	struct intel_engine_cs *irq_engine;
 
+	spinlock_t signalers_lock; /* protects the list of signalers */
 	struct list_head signalers;
 	struct llist_head signaled_requests;
 
+	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
 	struct irq_work irq_work; /* for use from inside irq_lock */
-
 	unsigned int irq_enabled;
-
 	bool irq_armed;
 };
 
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 8eebb31c6c28..b202dceeb4ec 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -302,7 +302,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
 
 	ce->vm = i915_vm_get(engine->gt->vm);
 
-	INIT_LIST_HEAD(&ce->signal_link);
+	/* NB ce->signal_link is used under RCU */
+	spin_lock_init(&ce->signal_lock);
 	INIT_LIST_HEAD(&ce->signals);
 
 	atomic_set(&ce->pin_count, 0);
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 18622f1a0249..edb50cbc0eb3 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -57,7 +57,14 @@ struct intel_context {
 	struct i915_address_space *vm;
 	struct i915_gem_context __rcu *gem_context;
 
-	struct list_head signal_link;
+	/*
+	 * @signal_lock protects the list of requests that need signaling,
+	 * @signals. While there are any requests that need signaling,
+	 * we add the context to the breadcrumbs worker, and remove it
+	 * upon completion/cancellation of the last request.
+	 */
+	spinlock_t signal_lock;
+	struct list_head signal_link; /* Accessed under RCU */
 	struct list_head signals;
 
 	struct i915_vma *state;
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Tvrtko Ursulin]

On 07/08/2020 13:54, Chris Wilson wrote:
> As we funnel more and more contexts into the breadcrumbs on an engine,
> the hold time of b->irq_lock grows. As we may then contend with the
> b->irq_lock during request submission, this increases the burden upon
> the engine->active.lock and so directly impacts both our execution
> latency and client latency. If we split the b->irq_lock by introducing a
> per-context spinlock to manage the signalers within a context, we then
> only need the b->irq_lock for enabling/disabling the interrupt and can
> avoid taking the lock for walking the list of contexts within the signal
> worker. Even with the current setup, this greatly reduces the number of
> times we have to take and fight for b->irq_lock.
> 
> Furthermore, this closes the race between enabling the signaling context
> while it is in the process of being signaled and removed:
> 
> <4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
> <4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
> <4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> <4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
> <4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> <4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
> <4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
> <4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
> <4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
> <4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
> <4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
> <4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
> <4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
> <4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> <4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> <4>[  416.208675] PKRU: 55555554
> <4>[  416.208677] Call Trace:
> <4>[  416.208679]  <IRQ>
> <4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
> <4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
> <4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
> <4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
> <4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
> <4>[  416.208954]  __do_softirq+0xdf/0x498
> <4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
> <4>[  416.208964]  asm_call_on_stack+0xf/0x20
> <4>[  416.208966]  </IRQ>
> <4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
> <4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
> <4>[  416.208976]  common_interrupt+0xf7/0x260
> <4>[  416.208980]  asm_common_interrupt+0x1e/0x40
> <4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
> <4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
> <4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
> <4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
> <4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
> <4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
> <4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
> <4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
> <4>[  416.209012]  cpuidle_enter+0x24/0x40
> <4>[  416.209016]  do_idle+0x22f/0x2d0
> <4>[  416.209022]  cpu_startup_entry+0x14/0x20
> <4>[  416.209025]  start_secondary+0x158/0x1a0
> <4>[  416.209030]  secondary_startup_64+0xa4/0xb0
> <4>[  416.209039] irq event stamp: 10186977
> <4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
> <4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
> <4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
> <4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20
> 
> <4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
> <4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
> <4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> <4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
> <4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> <4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
> <4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
> <4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
> <4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
> <4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
> <4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
> <4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
> <4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
> <4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> <4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> <4>[  416.209317] PKRU: 55555554
> <4>[  416.209317] Call Trace:
> <4>[  416.209317]  <IRQ>
> <4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
> <4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]
> 
> This is caused by virtual engines where although we take the breadcrumb
> lock on each of the active engines, they may be different engines on
> different requests, It turns out that the b->irq_lock was not a
> sufficient proxy for the engine->active.lock in the case of more than
> one request, so introduce an explicit lock around ce->signals.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2276
> Fixes: f94343d0a622 ("drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs")
> Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 173 +++++++++++-------
>   .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
>   drivers/gpu/drm/i915/gt/intel_context.c       |   3 +-
>   drivers/gpu/drm/i915/gt/intel_context_types.h |   9 +-
>   4 files changed, 114 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index a077ef3d02b4..e28efc1bb41d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
>   	intel_gt_pm_put_async(b->irq_engine->gt);
>   }
>   
> +static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> +{
> +	spin_lock(&b->irq_lock);
> +	if (b->irq_armed)
> +		__intel_breadcrumbs_disarm_irq(b);
> +	spin_unlock(&b->irq_lock);
> +}
> +
>   static void add_signaling_context(struct intel_breadcrumbs *b,
>   				  struct intel_context *ce)
>   {
> -	intel_context_get(ce);
> -	list_add_tail(&ce->signal_link, &b->signalers);
> +	lockdep_assert_held(&b->signalers_lock);
> +	list_add_rcu(&ce->signal_link, &b->signalers);
>   }
>   
>   static void remove_signaling_context(struct intel_breadcrumbs *b,
>   				     struct intel_context *ce)
>   {
> -	list_del(&ce->signal_link);
> -	intel_context_put(ce);
> +	spin_lock(&b->signalers_lock);
> +	list_del_rcu(&ce->signal_link);
> +	spin_unlock(&b->signalers_lock);
>   }
>   
>   static inline bool __request_completed(const struct i915_request *rq)
> @@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
>   	struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
>   	const ktime_t timestamp = ktime_get();
>   	struct llist_node *signal, *sn;
> -	struct intel_context *ce, *cn;
> -	struct list_head *pos, *next;
> +	struct intel_context *ce;
>   
>   	signal = NULL;
>   	if (unlikely(!llist_empty(&b->signaled_requests)))
>   		signal = llist_del_all(&b->signaled_requests);
>   
> -	spin_lock(&b->irq_lock);
> -
>   	/*
>   	 * Keep the irq armed until the interrupt after all listeners are gone.
>   	 *
> @@ -230,10 +236,32 @@ static void signal_irq_work(struct irq_work *work)
>   	 * like powertop.
>   	 */
>   	if (!signal && b->irq_armed && list_empty(&b->signalers))
> -		__intel_breadcrumbs_disarm_irq(b);
> +		intel_breadcrumbs_disarm_irq(b);

Disarming checks are now all unlocked so are you confident code won't 
miss to re-arm if here it races with someone else adding signalers?

> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> +		struct list_head *pos, *next;
> +		bool release = false;
> +
> +		/*
> +		 * Since the signal_irq_worker may in fact be running on
> +		 * different cores simultaneously, just skip over the other
> +		 * thread if it is in the middle of processing the signals.
> +		 * We will then advance to the next context, and the other
> +		 * core will then skip over us, splitting the work between
> +		 * the multiple cores that just happen to processing the
> +		 * worker simultaneously.
> +		 *
> +		 * The other use of ce->signal_lock to add new requests
> +		 * to be signaled will always kick the worker afterwards
> +		 * ensuring that ce->signals will always be processed at
> +		 * least once (even after being skipped).
> +		 */
> +		if (!spin_trylock(&ce->signal_lock))
> +			continue;
>   
> -	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
> -		GEM_BUG_ON(list_empty(&ce->signals));
> +		if (list_empty(&ce->signals))
> +			goto unlock;
>   
>   		list_for_each_safe(pos, next, &ce->signals) {
>   			struct i915_request *rq =
> @@ -265,11 +293,16 @@ static void signal_irq_work(struct irq_work *work)
>   			if (&ce->signals == pos) { /* now empty */
>   				add_retire(b, ce->timeline);
>   				remove_signaling_context(b, ce);
> +				release = true;
>   			}
>   		}
> -	}
>   
> -	spin_unlock(&b->irq_lock);
> +unlock:
> +		spin_unlock(&ce->signal_lock);
> +		if (release)
> +			intel_context_put(ce);
> +	}
> +	rcu_read_unlock();
>   
>   	llist_for_each_safe(signal, sn, signal) {
>   		struct i915_request *rq =
> @@ -298,14 +331,15 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
>   	if (!b)
>   		return NULL;
>   
> -	spin_lock_init(&b->irq_lock);
> +	b->irq_engine = irq_engine;
> +
> +	spin_lock_init(&b->signalers_lock);
>   	INIT_LIST_HEAD(&b->signalers);
>   	init_llist_head(&b->signaled_requests);
>   
> +	spin_lock_init(&b->irq_lock);
>   	init_irq_work(&b->irq_work, signal_irq_work);
>   
> -	b->irq_engine = irq_engine;
> -

Why these changes?

>   	return b;
>   }
>   
> @@ -344,9 +378,9 @@ void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
>   	kfree(b);
>   }
>   
> -static void insert_breadcrumb(struct i915_request *rq,
> -			      struct intel_breadcrumbs *b)
> +static void insert_breadcrumb(struct i915_request *rq)
>   {
> +	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
>   	struct intel_context *ce = rq->context;
>   	struct list_head *pos;
>   
> @@ -368,7 +402,33 @@ static void insert_breadcrumb(struct i915_request *rq,
>   	}
>   
>   	if (list_empty(&ce->signals)) {
> +		/*
> +		 * rq->engine is locked by rq->engine->active.lock. That
> +		 * however is not known until after rq->engine has been
> +		 * dereferenced and the lock acquired. Hence we acquire the
> +		 * lock and then validate that rq->engine still matches the
> +		 * lock we hold for it.
> +		 *
> +		 * Here, we are using the breadcrumb lock as a proxy for the
> +		 * rq->engine->active.lock, and we know that since the
> +		 * breadcrumb will be serialised within i915_request_submit
> +		 * the engine cannot change while active as long as we hold
> +		 * the breadcrumb lock on that engine.
> +		 *
> +		 * From the dma_fence_enable_signaling() path, we are outside
> +		 * of the request submit/unsubmit path, and so we must be more
> +		 * careful to acquire the right lock.
> +		 */
> +		intel_context_get(ce);
> +		spin_lock(&b->signalers_lock);
> +		while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
> +			spin_unlock(&b->signalers_lock);
> +			b = READ_ONCE(rq->engine)->breadcrumbs;
> +			spin_lock(&b->signalers_lock);
> +		}
>   		add_signaling_context(b, ce);
> +		spin_unlock(&b->signalers_lock);
> +
>   		pos = &ce->signals;
>   	} else {
>   		/*
> @@ -407,7 +467,7 @@ static void insert_breadcrumb(struct i915_request *rq,
>   
>   bool i915_request_enable_breadcrumb(struct i915_request *rq)
>   {
> -	struct intel_breadcrumbs *b;
> +	struct intel_context *ce = rq->context;
>   
>   	/* Serialises with i915_request_retire() using rq->lock */
>   	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
> @@ -422,67 +482,37 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>   	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
>   		return true;
>   
> -	/*
> -	 * rq->engine is locked by rq->engine->active.lock. That however
> -	 * is not known until after rq->engine has been dereferenced and
> -	 * the lock acquired. Hence we acquire the lock and then validate
> -	 * that rq->engine still matches the lock we hold for it.
> -	 *
> -	 * Here, we are using the breadcrumb lock as a proxy for the
> -	 * rq->engine->active.lock, and we know that since the breadcrumb
> -	 * will be serialised within i915_request_submit/i915_request_unsubmit,
> -	 * the engine cannot change while active as long as we hold the
> -	 * breadcrumb lock on that engine.
> -	 *
> -	 * From the dma_fence_enable_signaling() path, we are outside of the
> -	 * request submit/unsubmit path, and so we must be more careful to
> -	 * acquire the right lock.
> -	 */
> -	b = READ_ONCE(rq->engine)->breadcrumbs;
> -	spin_lock(&b->irq_lock);
> -	while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
> -		spin_unlock(&b->irq_lock);
> -		b = READ_ONCE(rq->engine)->breadcrumbs;
> -		spin_lock(&b->irq_lock);
> -	}
> -
> -	/*
> -	 * Now that we are finally serialised with request submit/unsubmit,
> -	 * [with b->irq_lock] and with i915_request_retire() [via checking
> -	 * SIGNALED with rq->lock] confirm the request is indeed active. If
> -	 * it is no longer active, the breadcrumb will be attached upon
> -	 * i915_request_submit().
> -	 */
> +	spin_lock(&ce->signal_lock);
>   	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> -		insert_breadcrumb(rq, b);
> -
> -	spin_unlock(&b->irq_lock);
> +		insert_breadcrumb(rq);
> +	spin_unlock(&ce->signal_lock);
>   
>   	return true;
>   }
>   
>   void i915_request_cancel_breadcrumb(struct i915_request *rq)
>   {
> -	struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
> +	struct intel_context *ce = rq->context;
> +	bool release = false;
>   
> -	/*
> -	 * We must wait for b->irq_lock so that we know the interrupt handler
> -	 * has released its reference to the intel_context and has completed
> -	 * the DMA_FENCE_FLAG_SIGNALED_BIT/I915_FENCE_FLAG_SIGNAL dance (if
> -	 * required).
> -	 */
> -	spin_lock(&b->irq_lock);
> +	if (!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
> +		return;
> +
> +	spin_lock(&ce->signal_lock);
>   	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
> -		struct intel_context *ce = rq->context;
> +		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>   
>   		list_del(&rq->signal_link);
> -		if (list_empty(&ce->signals))
> -			remove_signaling_context(b, ce);
> +		if (list_empty(&ce->signals)) {
> +			remove_signaling_context(rq->engine->breadcrumbs, ce);
> +			release = true;
> +		}
>   
> -		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>   		i915_request_put(rq);
>   	}
> -	spin_unlock(&b->irq_lock);
> +	spin_unlock(&ce->signal_lock);
> +	if (release)
> +		intel_context_put(ce);
>   }
>   
>   static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
> @@ -492,18 +522,19 @@ static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
>   
>   	drm_printf(p, "Signals:\n");
>   
> -	spin_lock_irq(&b->irq_lock);
> -	list_for_each_entry(ce, &b->signalers, signal_link) {
> -		list_for_each_entry(rq, &ce->signals, signal_link) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> +		spin_lock_irq(&ce->signal_lock);
> +		list_for_each_entry(rq, &ce->signals, signal_link)
>   			drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
>   				   rq->fence.context, rq->fence.seqno,
>   				   i915_request_completed(rq) ? "!" :
>   				   i915_request_started(rq) ? "*" :
>   				   "",
>   				   jiffies_to_msecs(jiffies - rq->emitted_jiffies));
> -		}
> +		spin_unlock_irq(&ce->signal_lock);
>   	}
> -	spin_unlock_irq(&b->irq_lock);
> +	rcu_read_unlock();
>   }
>   
>   void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> index 3fa19820b37a..a74bb3062bd8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> @@ -29,18 +29,16 @@
>    * the overhead of waking that client is much preferred.
>    */
>   struct intel_breadcrumbs {
> -	spinlock_t irq_lock; /* protects the lists used in hardirq context */
> -
>   	/* Not all breadcrumbs are attached to physical HW */
>   	struct intel_engine_cs *irq_engine;
>   
> +	spinlock_t signalers_lock; /* protects the list of signalers */
>   	struct list_head signalers;
>   	struct llist_head signaled_requests;
>   
> +	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
>   	struct irq_work irq_work; /* for use from inside irq_lock */
> -
>   	unsigned int irq_enabled;
> -
>   	bool irq_armed;
>   };
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 8eebb31c6c28..b202dceeb4ec 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -302,7 +302,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
>   
>   	ce->vm = i915_vm_get(engine->gt->vm);
>   
> -	INIT_LIST_HEAD(&ce->signal_link);
> +	/* NB ce->signal_link is used under RCU */
> +	spin_lock_init(&ce->signal_lock);
>   	INIT_LIST_HEAD(&ce->signals);
>   
>   	atomic_set(&ce->pin_count, 0);
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 18622f1a0249..edb50cbc0eb3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -57,7 +57,14 @@ struct intel_context {
>   	struct i915_address_space *vm;
>   	struct i915_gem_context __rcu *gem_context;
>   
> -	struct list_head signal_link;
> +	/*
> +	 * @signal_lock protects the list of requests that need signaling,
> +	 * @signals. While there are any requests that need signaling,
> +	 * we add the context to the breadcrumbs worker, and remove it
> +	 * upon completion/cancellation of the last request.
> +	 */
> +	spinlock_t signal_lock;
> +	struct list_head signal_link; /* Accessed under RCU */

Isn't signal_lock used under RCU as well? Are you sure it is safe to do 
spin_lock_init on it?

>   	struct list_head signals;
>   
>   	struct i915_vma *state;
> 

Regards,

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

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-08-07 15:25:53)
> 
> On 07/08/2020 13:54, Chris Wilson wrote:
> > As we funnel more and more contexts into the breadcrumbs on an engine,
> > the hold time of b->irq_lock grows. As we may then contend with the
> > b->irq_lock during request submission, this increases the burden upon
> > the engine->active.lock and so directly impacts both our execution
> > latency and client latency. If we split the b->irq_lock by introducing a
> > per-context spinlock to manage the signalers within a context, we then
> > only need the b->irq_lock for enabling/disabling the interrupt and can
> > avoid taking the lock for walking the list of contexts within the signal
> > worker. Even with the current setup, this greatly reduces the number of
> > times we have to take and fight for b->irq_lock.
> > 
> > Furthermore, this closes the race between enabling the signaling context
> > while it is in the process of being signaled and removed:
> > 
> > <4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
> > <4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
> > <4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> > <4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
> > <4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> > <4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
> > <4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
> > <4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
> > <4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
> > <4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
> > <4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
> > <4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
> > <4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
> > <4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> > <4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> > <4>[  416.208675] PKRU: 55555554
> > <4>[  416.208677] Call Trace:
> > <4>[  416.208679]  <IRQ>
> > <4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
> > <4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
> > <4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
> > <4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
> > <4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
> > <4>[  416.208954]  __do_softirq+0xdf/0x498
> > <4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
> > <4>[  416.208964]  asm_call_on_stack+0xf/0x20
> > <4>[  416.208966]  </IRQ>
> > <4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
> > <4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
> > <4>[  416.208976]  common_interrupt+0xf7/0x260
> > <4>[  416.208980]  asm_common_interrupt+0x1e/0x40
> > <4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
> > <4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
> > <4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
> > <4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
> > <4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
> > <4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
> > <4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
> > <4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
> > <4>[  416.209012]  cpuidle_enter+0x24/0x40
> > <4>[  416.209016]  do_idle+0x22f/0x2d0
> > <4>[  416.209022]  cpu_startup_entry+0x14/0x20
> > <4>[  416.209025]  start_secondary+0x158/0x1a0
> > <4>[  416.209030]  secondary_startup_64+0xa4/0xb0
> > <4>[  416.209039] irq event stamp: 10186977
> > <4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
> > <4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
> > <4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
> > <4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20
> > 
> > <4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
> > <4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
> > <4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> > <4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
> > <4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> > <4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
> > <4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
> > <4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
> > <4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
> > <4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
> > <4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
> > <4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
> > <4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
> > <4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> > <4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> > <4>[  416.209317] PKRU: 55555554
> > <4>[  416.209317] Call Trace:
> > <4>[  416.209317]  <IRQ>
> > <4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
> > <4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]
> > 
> > This is caused by virtual engines where although we take the breadcrumb
> > lock on each of the active engines, they may be different engines on
> > different requests, It turns out that the b->irq_lock was not a
> > sufficient proxy for the engine->active.lock in the case of more than
> > one request, so introduce an explicit lock around ce->signals.
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2276
> > Fixes: f94343d0a622 ("drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs")
> > Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 173 +++++++++++-------
> >   .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
> >   drivers/gpu/drm/i915/gt/intel_context.c       |   3 +-
> >   drivers/gpu/drm/i915/gt/intel_context_types.h |   9 +-
> >   4 files changed, 114 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > index a077ef3d02b4..e28efc1bb41d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > @@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> >       intel_gt_pm_put_async(b->irq_engine->gt);
> >   }
> >   
> > +static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> > +{
> > +     spin_lock(&b->irq_lock);
> > +     if (b->irq_armed)
> > +             __intel_breadcrumbs_disarm_irq(b);
> > +     spin_unlock(&b->irq_lock);
> > +}
> > +
> >   static void add_signaling_context(struct intel_breadcrumbs *b,
> >                                 struct intel_context *ce)
> >   {
> > -     intel_context_get(ce);
> > -     list_add_tail(&ce->signal_link, &b->signalers);
> > +     lockdep_assert_held(&b->signalers_lock);
> > +     list_add_rcu(&ce->signal_link, &b->signalers);
> >   }
> >   
> >   static void remove_signaling_context(struct intel_breadcrumbs *b,
> >                                    struct intel_context *ce)
> >   {
> > -     list_del(&ce->signal_link);
> > -     intel_context_put(ce);
> > +     spin_lock(&b->signalers_lock);
> > +     list_del_rcu(&ce->signal_link);
> > +     spin_unlock(&b->signalers_lock);
> >   }
> >   
> >   static inline bool __request_completed(const struct i915_request *rq)
> > @@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
> >       struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
> >       const ktime_t timestamp = ktime_get();
> >       struct llist_node *signal, *sn;
> > -     struct intel_context *ce, *cn;
> > -     struct list_head *pos, *next;
> > +     struct intel_context *ce;
> >   
> >       signal = NULL;
> >       if (unlikely(!llist_empty(&b->signaled_requests)))
> >               signal = llist_del_all(&b->signaled_requests);
> >   
> > -     spin_lock(&b->irq_lock);
> > -
> >       /*
> >        * Keep the irq armed until the interrupt after all listeners are gone.
> >        *
> > @@ -230,10 +236,32 @@ static void signal_irq_work(struct irq_work *work)
> >        * like powertop.
> >        */
> >       if (!signal && b->irq_armed && list_empty(&b->signalers))
> > -             __intel_breadcrumbs_disarm_irq(b);
> > +             intel_breadcrumbs_disarm_irq(b);
> 
> Disarming checks are now all unlocked so are you confident code won't 
> miss to re-arm if here it races with someone else adding signalers?

I pushed the spin_lock into a wrapper.

intel_breadcrumbs_disarm_irq() takes the spinlock around
__intel_breadcrumbs_disarm_irq(). It had pleasing symmetry with
intel_breadcrumbs_arm_irq() & __intel_breadcrumbs_arm_irq().

We very much need spinlocks here as the irq-worker can run concurrently
on multiple cores. That did come as a nasty shock.

> > @@ -298,14 +331,15 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
> >       if (!b)
> >               return NULL;
> >   
> > -     spin_lock_init(&b->irq_lock);
> > +     b->irq_engine = irq_engine;
> > +
> > +     spin_lock_init(&b->signalers_lock);
> >       INIT_LIST_HEAD(&b->signalers);
> >       init_llist_head(&b->signaled_requests);
> >   
> > +     spin_lock_init(&b->irq_lock);
> >       init_irq_work(&b->irq_work, signal_irq_work);
> >   
> > -     b->irq_engine = irq_engine;
> > -
> 
> Why these changes?

I was trying to group up the blocks of neighbours with their locking.

Since we have signal lists & lock in one block, I thought putting
irq_lock & irq_work into a second block made sense. Then I thought it was
better to separate the b->irq_engine from the irq_work/lock, as it was
an input parameter and not guarded by the irq_lock.

> > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > index 18622f1a0249..edb50cbc0eb3 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > @@ -57,7 +57,14 @@ struct intel_context {
> >       struct i915_address_space *vm;
> >       struct i915_gem_context __rcu *gem_context;
> >   
> > -     struct list_head signal_link;
> > +     /*
> > +      * @signal_lock protects the list of requests that need signaling,
> > +      * @signals. While there are any requests that need signaling,
> > +      * we add the context to the breadcrumbs worker, and remove it
> > +      * upon completion/cancellation of the last request.
> > +      */
> > +     spinlock_t signal_lock;
> > +     struct list_head signal_link; /* Accessed under RCU */
> 
> Isn't signal_lock used under RCU as well? Are you sure it is safe to do 
> spin_lock_init on it?

We call intel_context_put() upon removing the context after
spin_unlock(ce->signal_lock).

My original intention was to be able to use signal_lock on the zombie by
keeping it alive under RCU (and so why I originally had to use the ctor).
But it turns out that debugobjects is checked at kmem_cache_free() even
for TYPESAFE_BY_RCU and that complains if you have an active spinlock at
that time. My dastardly plan was foiled, but it does mean that all but
one field is under the reference.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Tvrtko Ursulin]

On 07/08/2020 15:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-08-07 15:25:53)
>>
>> On 07/08/2020 13:54, Chris Wilson wrote:
>>> As we funnel more and more contexts into the breadcrumbs on an engine,
>>> the hold time of b->irq_lock grows. As we may then contend with the
>>> b->irq_lock during request submission, this increases the burden upon
>>> the engine->active.lock and so directly impacts both our execution
>>> latency and client latency. If we split the b->irq_lock by introducing a
>>> per-context spinlock to manage the signalers within a context, we then
>>> only need the b->irq_lock for enabling/disabling the interrupt and can
>>> avoid taking the lock for walking the list of contexts within the signal
>>> worker. Even with the current setup, this greatly reduces the number of
>>> times we have to take and fight for b->irq_lock.
>>>
>>> Furthermore, this closes the race between enabling the signaling context
>>> while it is in the process of being signaled and removed:
>>>
>>> <4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
>>> <4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
>>> <4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
>>> <4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
>>> <4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
>>> <4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
>>> <4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
>>> <4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
>>> <4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
>>> <4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
>>> <4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
>>> <4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
>>> <4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
>>> <4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
>>> <4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> <4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
>>> <4>[  416.208675] PKRU: 55555554
>>> <4>[  416.208677] Call Trace:
>>> <4>[  416.208679]  <IRQ>
>>> <4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
>>> <4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
>>> <4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
>>> <4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
>>> <4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
>>> <4>[  416.208954]  __do_softirq+0xdf/0x498
>>> <4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
>>> <4>[  416.208964]  asm_call_on_stack+0xf/0x20
>>> <4>[  416.208966]  </IRQ>
>>> <4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
>>> <4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
>>> <4>[  416.208976]  common_interrupt+0xf7/0x260
>>> <4>[  416.208980]  asm_common_interrupt+0x1e/0x40
>>> <4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
>>> <4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
>>> <4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
>>> <4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
>>> <4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
>>> <4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
>>> <4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
>>> <4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
>>> <4>[  416.209012]  cpuidle_enter+0x24/0x40
>>> <4>[  416.209016]  do_idle+0x22f/0x2d0
>>> <4>[  416.209022]  cpu_startup_entry+0x14/0x20
>>> <4>[  416.209025]  start_secondary+0x158/0x1a0
>>> <4>[  416.209030]  secondary_startup_64+0xa4/0xb0
>>> <4>[  416.209039] irq event stamp: 10186977
>>> <4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
>>> <4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
>>> <4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
>>> <4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20
>>>
>>> <4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
>>> <4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
>>> <4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
>>> <4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
>>> <4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
>>> <4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
>>> <4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
>>> <4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
>>> <4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
>>> <4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
>>> <4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
>>> <4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
>>> <4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
>>> <4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
>>> <4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> <4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
>>> <4>[  416.209317] PKRU: 55555554
>>> <4>[  416.209317] Call Trace:
>>> <4>[  416.209317]  <IRQ>
>>> <4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
>>> <4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]
>>>
>>> This is caused by virtual engines where although we take the breadcrumb
>>> lock on each of the active engines, they may be different engines on
>>> different requests, It turns out that the b->irq_lock was not a
>>> sufficient proxy for the engine->active.lock in the case of more than
>>> one request, so introduce an explicit lock around ce->signals.
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2276
>>> Fixes: f94343d0a622 ("drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs")
>>> Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 173 +++++++++++-------
>>>    .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
>>>    drivers/gpu/drm/i915/gt/intel_context.c       |   3 +-
>>>    drivers/gpu/drm/i915/gt/intel_context_types.h |   9 +-
>>>    4 files changed, 114 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> index a077ef3d02b4..e28efc1bb41d 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> @@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
>>>        intel_gt_pm_put_async(b->irq_engine->gt);
>>>    }
>>>    
>>> +static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
>>> +{
>>> +     spin_lock(&b->irq_lock);
>>> +     if (b->irq_armed)
>>> +             __intel_breadcrumbs_disarm_irq(b);
>>> +     spin_unlock(&b->irq_lock);
>>> +}
>>> +
>>>    static void add_signaling_context(struct intel_breadcrumbs *b,
>>>                                  struct intel_context *ce)
>>>    {
>>> -     intel_context_get(ce);
>>> -     list_add_tail(&ce->signal_link, &b->signalers);
>>> +     lockdep_assert_held(&b->signalers_lock);
>>> +     list_add_rcu(&ce->signal_link, &b->signalers);
>>>    }
>>>    
>>>    static void remove_signaling_context(struct intel_breadcrumbs *b,
>>>                                     struct intel_context *ce)
>>>    {
>>> -     list_del(&ce->signal_link);
>>> -     intel_context_put(ce);
>>> +     spin_lock(&b->signalers_lock);
>>> +     list_del_rcu(&ce->signal_link);
>>> +     spin_unlock(&b->signalers_lock);
>>>    }
>>>    
>>>    static inline bool __request_completed(const struct i915_request *rq)
>>> @@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
>>>        struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
>>>        const ktime_t timestamp = ktime_get();
>>>        struct llist_node *signal, *sn;
>>> -     struct intel_context *ce, *cn;
>>> -     struct list_head *pos, *next;
>>> +     struct intel_context *ce;
>>>    
>>>        signal = NULL;
>>>        if (unlikely(!llist_empty(&b->signaled_requests)))
>>>                signal = llist_del_all(&b->signaled_requests);
>>>    
>>> -     spin_lock(&b->irq_lock);
>>> -
>>>        /*
>>>         * Keep the irq armed until the interrupt after all listeners are gone.
>>>         *
>>> @@ -230,10 +236,32 @@ static void signal_irq_work(struct irq_work *work)
>>>         * like powertop.
>>>         */
>>>        if (!signal && b->irq_armed && list_empty(&b->signalers))
>>> -             __intel_breadcrumbs_disarm_irq(b);
>>> +             intel_breadcrumbs_disarm_irq(b);
>>
>> Disarming checks are now all unlocked so are you confident code won't
>> miss to re-arm if here it races with someone else adding signalers?
> 
> I pushed the spin_lock into a wrapper.
> 
> intel_breadcrumbs_disarm_irq() takes the spinlock around
> __intel_breadcrumbs_disarm_irq(). It had pleasing symmetry with
> intel_breadcrumbs_arm_irq() & __intel_breadcrumbs_arm_irq().
> 
> We very much need spinlocks here as the irq-worker can run concurrently
> on multiple cores. That did come as a nasty shock.

Yes, the actual enable count and that, but the list empty status? If is 
sees empty here but someone actually adds in parallel will it get armed 
somewhere else?

Hm or pull up the rcu_read_lock to above the disarm check even if it 
means little or nothing?

> 
>>> @@ -298,14 +331,15 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
>>>        if (!b)
>>>                return NULL;
>>>    
>>> -     spin_lock_init(&b->irq_lock);
>>> +     b->irq_engine = irq_engine;
>>> +
>>> +     spin_lock_init(&b->signalers_lock);
>>>        INIT_LIST_HEAD(&b->signalers);
>>>        init_llist_head(&b->signaled_requests);
>>>    
>>> +     spin_lock_init(&b->irq_lock);
>>>        init_irq_work(&b->irq_work, signal_irq_work);
>>>    
>>> -     b->irq_engine = irq_engine;
>>> -
>>
>> Why these changes?
> 
> I was trying to group up the blocks of neighbours with their locking.
> 
> Since we have signal lists & lock in one block, I thought putting
> irq_lock & irq_work into a second block made sense. Then I thought it was
> better to separate the b->irq_engine from the irq_work/lock, as it was
> an input parameter and not guarded by the irq_lock.

Ok.

> 
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
>>> index 18622f1a0249..edb50cbc0eb3 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
>>> @@ -57,7 +57,14 @@ struct intel_context {
>>>        struct i915_address_space *vm;
>>>        struct i915_gem_context __rcu *gem_context;
>>>    
>>> -     struct list_head signal_link;
>>> +     /*
>>> +      * @signal_lock protects the list of requests that need signaling,
>>> +      * @signals. While there are any requests that need signaling,
>>> +      * we add the context to the breadcrumbs worker, and remove it
>>> +      * upon completion/cancellation of the last request.
>>> +      */
>>> +     spinlock_t signal_lock;
>>> +     struct list_head signal_link; /* Accessed under RCU */
>>
>> Isn't signal_lock used under RCU as well? Are you sure it is safe to do
>> spin_lock_init on it?
> 
> We call intel_context_put() upon removing the context after
> spin_unlock(ce->signal_lock).
> 
> My original intention was to be able to use signal_lock on the zombie by
> keeping it alive under RCU (and so why I originally had to use the ctor).
> But it turns out that debugobjects is checked at kmem_cache_free() even
> for TYPESAFE_BY_RCU and that complains if you have an active spinlock at
> that time. My dastardly plan was foiled, but it does mean that all but
> one field is under the reference.

How do I translate this - you think it is safe? Spinlock access is 
always with a strong reference? In print_signals at least it doesn't 
appear so. Lock won't be freed but can't be re-cycled mid loop. Or you 
think it can't?

Regards,

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

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-08-07 15:45:54)
> 
> On 07/08/2020 15:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-08-07 15:25:53)
> >>
> >> On 07/08/2020 13:54, Chris Wilson wrote:
> >>> As we funnel more and more contexts into the breadcrumbs on an engine,
> >>> the hold time of b->irq_lock grows. As we may then contend with the
> >>> b->irq_lock during request submission, this increases the burden upon
> >>> the engine->active.lock and so directly impacts both our execution
> >>> latency and client latency. If we split the b->irq_lock by introducing a
> >>> per-context spinlock to manage the signalers within a context, we then
> >>> only need the b->irq_lock for enabling/disabling the interrupt and can
> >>> avoid taking the lock for walking the list of contexts within the signal
> >>> worker. Even with the current setup, this greatly reduces the number of
> >>> times we have to take and fight for b->irq_lock.
> >>>
> >>> Furthermore, this closes the race between enabling the signaling context
> >>> while it is in the process of being signaled and removed:
> >>>
> >>> <4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
> >>> <4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
> >>> <4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> >>> <4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
> >>> <4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> >>> <4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
> >>> <4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
> >>> <4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
> >>> <4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
> >>> <4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
> >>> <4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
> >>> <4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
> >>> <4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
> >>> <4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> >>> <4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>> <4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> >>> <4>[  416.208675] PKRU: 55555554
> >>> <4>[  416.208677] Call Trace:
> >>> <4>[  416.208679]  <IRQ>
> >>> <4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
> >>> <4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
> >>> <4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
> >>> <4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
> >>> <4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
> >>> <4>[  416.208954]  __do_softirq+0xdf/0x498
> >>> <4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
> >>> <4>[  416.208964]  asm_call_on_stack+0xf/0x20
> >>> <4>[  416.208966]  </IRQ>
> >>> <4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
> >>> <4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
> >>> <4>[  416.208976]  common_interrupt+0xf7/0x260
> >>> <4>[  416.208980]  asm_common_interrupt+0x1e/0x40
> >>> <4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
> >>> <4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
> >>> <4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
> >>> <4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
> >>> <4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
> >>> <4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
> >>> <4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
> >>> <4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
> >>> <4>[  416.209012]  cpuidle_enter+0x24/0x40
> >>> <4>[  416.209016]  do_idle+0x22f/0x2d0
> >>> <4>[  416.209022]  cpu_startup_entry+0x14/0x20
> >>> <4>[  416.209025]  start_secondary+0x158/0x1a0
> >>> <4>[  416.209030]  secondary_startup_64+0xa4/0xb0
> >>> <4>[  416.209039] irq event stamp: 10186977
> >>> <4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
> >>> <4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
> >>> <4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
> >>> <4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20
> >>>
> >>> <4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
> >>> <4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
> >>> <4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> >>> <4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
> >>> <4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> >>> <4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
> >>> <4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
> >>> <4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
> >>> <4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
> >>> <4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
> >>> <4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
> >>> <4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
> >>> <4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
> >>> <4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> >>> <4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>> <4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> >>> <4>[  416.209317] PKRU: 55555554
> >>> <4>[  416.209317] Call Trace:
> >>> <4>[  416.209317]  <IRQ>
> >>> <4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
> >>> <4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]
> >>>
> >>> This is caused by virtual engines where although we take the breadcrumb
> >>> lock on each of the active engines, they may be different engines on
> >>> different requests, It turns out that the b->irq_lock was not a
> >>> sufficient proxy for the engine->active.lock in the case of more than
> >>> one request, so introduce an explicit lock around ce->signals.
> >>>
> >>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2276
> >>> Fixes: f94343d0a622 ("drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs")
> >>> Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 173 +++++++++++-------
> >>>    .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
> >>>    drivers/gpu/drm/i915/gt/intel_context.c       |   3 +-
> >>>    drivers/gpu/drm/i915/gt/intel_context_types.h |   9 +-
> >>>    4 files changed, 114 insertions(+), 77 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> >>> index a077ef3d02b4..e28efc1bb41d 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> >>> @@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> >>>        intel_gt_pm_put_async(b->irq_engine->gt);
> >>>    }
> >>>    
> >>> +static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> >>> +{
> >>> +     spin_lock(&b->irq_lock);
> >>> +     if (b->irq_armed)
> >>> +             __intel_breadcrumbs_disarm_irq(b);
> >>> +     spin_unlock(&b->irq_lock);
> >>> +}
> >>> +
> >>>    static void add_signaling_context(struct intel_breadcrumbs *b,
> >>>                                  struct intel_context *ce)
> >>>    {
> >>> -     intel_context_get(ce);
> >>> -     list_add_tail(&ce->signal_link, &b->signalers);
> >>> +     lockdep_assert_held(&b->signalers_lock);
> >>> +     list_add_rcu(&ce->signal_link, &b->signalers);
> >>>    }
> >>>    
> >>>    static void remove_signaling_context(struct intel_breadcrumbs *b,
> >>>                                     struct intel_context *ce)
> >>>    {
> >>> -     list_del(&ce->signal_link);
> >>> -     intel_context_put(ce);
> >>> +     spin_lock(&b->signalers_lock);
> >>> +     list_del_rcu(&ce->signal_link);
> >>> +     spin_unlock(&b->signalers_lock);
> >>>    }
> >>>    
> >>>    static inline bool __request_completed(const struct i915_request *rq)
> >>> @@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
> >>>        struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
> >>>        const ktime_t timestamp = ktime_get();
> >>>        struct llist_node *signal, *sn;
> >>> -     struct intel_context *ce, *cn;
> >>> -     struct list_head *pos, *next;
> >>> +     struct intel_context *ce;
> >>>    
> >>>        signal = NULL;
> >>>        if (unlikely(!llist_empty(&b->signaled_requests)))
> >>>                signal = llist_del_all(&b->signaled_requests);
> >>>    
> >>> -     spin_lock(&b->irq_lock);
> >>> -
> >>>        /*
> >>>         * Keep the irq armed until the interrupt after all listeners are gone.
> >>>         *
> >>> @@ -230,10 +236,32 @@ static void signal_irq_work(struct irq_work *work)
> >>>         * like powertop.
> >>>         */
> >>>        if (!signal && b->irq_armed && list_empty(&b->signalers))
> >>> -             __intel_breadcrumbs_disarm_irq(b);
> >>> +             intel_breadcrumbs_disarm_irq(b);
> >>
> >> Disarming checks are now all unlocked so are you confident code won't
> >> miss to re-arm if here it races with someone else adding signalers?
> > 
> > I pushed the spin_lock into a wrapper.
> > 
> > intel_breadcrumbs_disarm_irq() takes the spinlock around
> > __intel_breadcrumbs_disarm_irq(). It had pleasing symmetry with
> > intel_breadcrumbs_arm_irq() & __intel_breadcrumbs_arm_irq().
> > 
> > We very much need spinlocks here as the irq-worker can run concurrently
> > on multiple cores. That did come as a nasty shock.
> 
> Yes, the actual enable count and that, but the list empty status? If is 
> sees empty here but someone actually adds in parallel will it get armed 
> somewhere else?

The thread adding to the list will run the irq-worker after themselves,
and we rearm at the end of the irq-worker.

> Hm or pull up the rcu_read_lock to above the disarm check even if it 
> means little or nothing?

It means nothing, as there is no dereference and READ_ONCE used by
list_empty() suffices. I like the look of

        rcu_read_lock();
        list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
	}
        rcu_read_unlock();

that's the only reason I'm hesitant to change.

> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> >>> index 18622f1a0249..edb50cbc0eb3 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> >>> @@ -57,7 +57,14 @@ struct intel_context {
> >>>        struct i915_address_space *vm;
> >>>        struct i915_gem_context __rcu *gem_context;
> >>>    
> >>> -     struct list_head signal_link;
> >>> +     /*
> >>> +      * @signal_lock protects the list of requests that need signaling,
> >>> +      * @signals. While there are any requests that need signaling,
> >>> +      * we add the context to the breadcrumbs worker, and remove it
> >>> +      * upon completion/cancellation of the last request.
> >>> +      */
> >>> +     spinlock_t signal_lock;
> >>> +     struct list_head signal_link; /* Accessed under RCU */
> >>
> >> Isn't signal_lock used under RCU as well? Are you sure it is safe to do
> >> spin_lock_init on it?
> > 
> > We call intel_context_put() upon removing the context after
> > spin_unlock(ce->signal_lock).
> > 
> > My original intention was to be able to use signal_lock on the zombie by
> > keeping it alive under RCU (and so why I originally had to use the ctor).
> > But it turns out that debugobjects is checked at kmem_cache_free() even
> > for TYPESAFE_BY_RCU and that complains if you have an active spinlock at
> > that time. My dastardly plan was foiled, but it does mean that all but
> > one field is under the reference.
> 
> How do I translate this - you think it is safe? Spinlock access is 
> always with a strong reference? In print_signals at least it doesn't 
> appear so. Lock won't be freed but can't be re-cycled mid loop. Or you 
> think it can't?

No. print_signals() I completely forgot about.

Hmm.

Nope. You are right, even in the main loop since we take ce->signal_lock
prior to checking ce->signals that is a pure RCU access.

That means the spin_lock_init() in intel_context_init() is dangerous and
we need the ctor again. /o\
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 7/7] drm/i915/gt: Free stale request on destroying the virtual engine

[Chris Wilson]

Since preempt-to-busy, we may unsubmit a request while it is still on
the HW and completes asynchronously. That means it may be retired and in
the process destroy the virtual engine (as the user has closed their
context), but that engine may still be holding onto the unsubmitted
compelted request. Therefore we need to potentially cleanup the old
request on destroying the virtual engine. We also have to keep the
virtual_engine alive until after the sibling's execlists_dequeue() have
finished peeking into the virtual engines, for which we serialise with
RCU.

v2: Be paranoid and flush the tasklet as well.
v3: And flush the tasklet before the engines, as the tasklet may
re-attach an rb_node after our removal from the siblings.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 58 +++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 0c632f15f677..bc00d066ceea 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -182,6 +182,7 @@
 struct virtual_engine {
 	struct intel_engine_cs base;
 	struct intel_context context;
+	struct rcu_work rcu;
 
 	/*
 	 * We allow only a single request through the virtual engine at a time
@@ -5387,33 +5388,57 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
 	return &ve->base.execlists.default_priolist.requests[0];
 }
 
-static void virtual_context_destroy(struct kref *kref)
+static void rcu_virtual_context_destroy(struct work_struct *wrk)
 {
 	struct virtual_engine *ve =
-		container_of(kref, typeof(*ve), context.ref);
+		container_of(wrk, typeof(*ve), rcu.work);
 	unsigned int n;
 
-	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
-	GEM_BUG_ON(ve->request);
 	GEM_BUG_ON(ve->context.inflight);
 
+	/* Preempt-to-busy may leave a stale request behind. */
+	if (unlikely(ve->request)) {
+		struct i915_request *old;
+
+		spin_lock_irq(&ve->base.active.lock);
+
+		old = fetch_and_zero(&ve->request);
+		if (old) {
+			GEM_BUG_ON(!i915_request_completed(old));
+			__i915_request_submit(old);
+			i915_request_put(old);
+		}
+
+		spin_unlock_irq(&ve->base.active.lock);
+	}
+
+	/*
+	 * Flush the tasklet in case it is still running on another core.
+	 *
+	 * This needs to be done before we remove ourselves from the siblings'
+	 * rbtrees as in the case it is running in parallel, it may reinsert
+	 * the rb_node into a sibling.
+	 */
+	tasklet_kill(&ve->base.execlists.tasklet);
+
+	/* Decouple ourselves from the siblings, no more access allowed. */
 	for (n = 0; n < ve->num_siblings; n++) {
 		struct intel_engine_cs *sibling = ve->siblings[n];
 		struct rb_node *node = &ve->nodes[sibling->id].rb;
-		unsigned long flags;
 
 		if (RB_EMPTY_NODE(node))
 			continue;
 
-		spin_lock_irqsave(&sibling->active.lock, flags);
+		spin_lock_irq(&sibling->active.lock);
 
 		/* Detachment is lazily performed in the execlists tasklet */
 		if (!RB_EMPTY_NODE(node))
 			rb_erase_cached(node, &sibling->execlists.virtual);
 
-		spin_unlock_irqrestore(&sibling->active.lock, flags);
+		spin_unlock_irq(&sibling->active.lock);
 	}
 	GEM_BUG_ON(__tasklet_is_scheduled(&ve->base.execlists.tasklet));
+	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
 
 	if (ve->context.state)
 		__execlists_context_fini(&ve->context);
@@ -5425,6 +5450,25 @@ static void virtual_context_destroy(struct kref *kref)
 	kfree(ve);
 }
 
+static void virtual_context_destroy(struct kref *kref)
+{
+	struct virtual_engine *ve =
+		container_of(kref, typeof(*ve), context.ref);
+
+	/*
+	 * When destroying the virtual engine, we have to be aware that
+	 * it may still be in use from an hardirq/softirq context causing
+	 * the resubmission of a completed request (background completion
+	 * due to preempt-to-busy). Before we can free the engine, we need
+	 * to flush the submission code and tasklets that are still potentially
+	 * accessing the engine. Flushing the tasklets require process context,
+	 * and since we can guard the resubmit onto the engine with an RCU read
+	 * lock, we can delegate the free of the engine to an RCU worker.
+	 */
+	INIT_RCU_WORK(&ve->rcu, rcu_virtual_context_destroy);
+	queue_rcu_work(system_wq, &ve->rcu);
+}
+
 static void virtual_engine_initial_hint(struct virtual_engine *ve)
 {
 	int swp;
-- 
2.20.1

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()
URL   : https://patchwork.freedesktop.org/series/80393/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f82f8ef253eb drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()
9d375c4552bf drm/i915/gt: Protect context lifetime with RCU
9676cd4fcbab drm/i915/gt: Defer enabling the breadcrumb interrupt to after submission
45d3f0fee88b drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
7f54c977c742 drm/i915/gt: Don't cancel the interrupt shadow too early
435242a104ce drm/i915/gt: Split the breadcrumb spinlock between global and contexts
-:21: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#21: 
<4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).

total: 0 errors, 1 warnings, 0 checks, 322 lines checked
cfd370c67224 drm/i915/gt: Free stale request on destroying the virtual engine


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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()
URL   : https://patchwork.freedesktop.org/series/80393/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.


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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 11307 bytes --]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()
URL   : https://patchwork.freedesktop.org/series/80393/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8856 -> Patchwork_18322
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18322 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18322, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18322:

### CI changes ###

#### Possible regressions ####

  * boot:
    - fi-kbl-8809g:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-8809g/boot.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-8809g/boot.html
    - fi-icl-y:           [PASS][3] -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-icl-y/boot.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-icl-y/boot.html
    - fi-snb-2520m:       [PASS][5] -> [FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-snb-2520m/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-snb-2520m/boot.html
    - fi-bsw-nick:        [PASS][7] -> [FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bsw-nick/boot.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bsw-nick/boot.html
    - fi-gdg-551:         [PASS][9] -> [FAIL][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-gdg-551/boot.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-gdg-551/boot.html
    - fi-icl-u2:          [PASS][11] -> [FAIL][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-icl-u2/boot.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-icl-u2/boot.html
    - fi-cfl-8109u:       [PASS][13] -> [FAIL][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-cfl-8109u/boot.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-cfl-8109u/boot.html
    - fi-skl-6600u:       [PASS][15] -> [FAIL][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-skl-6600u/boot.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-skl-6600u/boot.html
    - fi-byt-j1900:       [PASS][17] -> [FAIL][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-byt-j1900/boot.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-byt-j1900/boot.html
    - fi-cfl-8700k:       [PASS][19] -> [FAIL][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-cfl-8700k/boot.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-cfl-8700k/boot.html
    - fi-cml-u2:          [PASS][21] -> [FAIL][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-cml-u2/boot.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-cml-u2/boot.html
    - fi-pnv-d510:        [PASS][23] -> [FAIL][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-pnv-d510/boot.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-pnv-d510/boot.html
    - fi-ilk-650:         [PASS][25] -> [FAIL][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-ilk-650/boot.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-ilk-650/boot.html
    - fi-tgl-u2:          [PASS][27] -> [FAIL][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-tgl-u2/boot.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-tgl-u2/boot.html
    - fi-bsw-n3050:       [PASS][29] -> [FAIL][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bsw-n3050/boot.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bsw-n3050/boot.html
    - fi-skl-6700k2:      [PASS][31] -> [FAIL][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-skl-6700k2/boot.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-skl-6700k2/boot.html
    - fi-hsw-4770:        [PASS][33] -> [FAIL][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-hsw-4770/boot.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-hsw-4770/boot.html
    - fi-kbl-soraka:      [PASS][35] -> [FAIL][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-soraka/boot.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-soraka/boot.html
    - fi-cml-s:           [PASS][37] -> [FAIL][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-cml-s/boot.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-cml-s/boot.html
    - fi-elk-e7500:       [PASS][39] -> [FAIL][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-elk-e7500/boot.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-elk-e7500/boot.html
    - fi-skl-lmem:        [PASS][41] -> [FAIL][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-skl-lmem/boot.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-skl-lmem/boot.html
    - fi-ivb-3770:        [PASS][43] -> [FAIL][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-ivb-3770/boot.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-ivb-3770/boot.html
    - fi-snb-2600:        [PASS][45] -> [FAIL][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-snb-2600/boot.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-snb-2600/boot.html
    - fi-kbl-guc:         [PASS][47] -> [FAIL][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-guc/boot.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-guc/boot.html
    - fi-bsw-kefka:       [PASS][49] -> [FAIL][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bsw-kefka/boot.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bsw-kefka/boot.html
    - fi-kbl-x1275:       [PASS][51] -> [FAIL][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-x1275/boot.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-x1275/boot.html
    - fi-bdw-gvtdvm:      [PASS][53] -> [FAIL][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bdw-gvtdvm/boot.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bdw-gvtdvm/boot.html
    - fi-kbl-7500u:       [PASS][55] -> [FAIL][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-7500u/boot.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-7500u/boot.html
    - fi-blb-e6850:       [PASS][57] -> [FAIL][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-blb-e6850/boot.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-blb-e6850/boot.html
    - fi-bwr-2160:        [PASS][59] -> [FAIL][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bwr-2160/boot.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bwr-2160/boot.html
    - fi-bdw-5557u:       [PASS][61] -> [FAIL][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bdw-5557u/boot.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bdw-5557u/boot.html
    - fi-skl-guc:         [PASS][63] -> [FAIL][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-skl-guc/boot.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-skl-guc/boot.html
    - fi-kbl-r:           [PASS][65] -> [FAIL][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-r/boot.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-r/boot.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * boot:
    - {fi-tgl-dsi}:       [PASS][67] -> [FAIL][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-tgl-dsi/boot.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-tgl-dsi/boot.html
    - {fi-kbl-7560u}:     [PASS][69] -> [FAIL][70]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-kbl-7560u/boot.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-kbl-7560u/boot.html
    - {fi-ehl-1}:         [PASS][71] -> [FAIL][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-ehl-1/boot.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-ehl-1/boot.html

  
Known issues
------------

  Here are the changes found in Patchwork_18322 that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - fi-apl-guc:         [PASS][73] -> [FAIL][74] ([i915#1635] / [i915#348])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-apl-guc/boot.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-apl-guc/boot.html
    - fi-bxt-dsi:         [PASS][75] -> [FAIL][76] ([i915#1635])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8856/fi-bxt-dsi/boot.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/fi-bxt-dsi/boot.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#348]: https://gitlab.freedesktop.org/drm/intel/issues/348


Participating hosts (44 -> 38)
------------------------------

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_5764 -> None
  * Linux: CI_DRM_8856 -> Patchwork_18322

  CI-20190529: 20190529
  CI_DRM_8856: 238c742f0beea85fc171bfc3eef05cf284af6d4d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5764: 0f91c80b4c809cf48afff65a2ea68590389aa5ba @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18322: cfd370c67224a0b3d3b0440e2738c12b39d63247 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

cfd370c67224 drm/i915/gt: Free stale request on destroying the virtual engine
435242a104ce drm/i915/gt: Split the breadcrumb spinlock between global and contexts
7f54c977c742 drm/i915/gt: Don't cancel the interrupt shadow too early
45d3f0fee88b drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
9676cd4fcbab drm/i915/gt: Defer enabling the breadcrumb interrupt to after submission
9d375c4552bf drm/i915/gt: Protect context lifetime with RCU
f82f8ef253eb drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18322/index.html

[-- Attachment #1.2: Type: text/html, Size: 12611 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)
URL   : https://patchwork.freedesktop.org/series/80393/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
423786160985 drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()
b60c43f1b554 drm/i915/gt: Protect context lifetime with RCU
c1f2e1672d57 drm/i915/gt: Defer enabling the breadcrumb interrupt to after submission
b73b5a3bfec2 drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
2debc1598e3e drm/i915/gt: Don't cancel the interrupt shadow too early
0d54a2698fa2 drm/i915/gt: Split the breadcrumb spinlock between global and contexts
-:21: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#21: 
<4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).

total: 0 errors, 1 warnings, 0 checks, 322 lines checked
b59671512b13 drm/i915/gt: Free stale request on destroying the virtual engine


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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)
URL   : https://patchwork.freedesktop.org/series/80393/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.


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

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 4836 bytes --]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)
URL   : https://patchwork.freedesktop.org/series/80393/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8857 -> Patchwork_18323
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/index.html

Known issues
------------

  Here are the changes found in Patchwork_18323 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@basic@flip:
    - fi-kbl-x1275:       [PASS][1] -> [DMESG-WARN][2] ([i915#62] / [i915#92] / [i915#95])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/fi-kbl-x1275/igt@kms_busy@basic@flip.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/fi-kbl-x1275/igt@kms_busy@basic@flip.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-bsw-kefka:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-icl-u2:          [PASS][5] -> [DMESG-WARN][6] ([i915#1982]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - {fi-kbl-7560u}:     [WARN][7] ([i915#2249]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/fi-kbl-7560u/igt@i915_pm_rpm@module-reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/fi-kbl-7560u/igt@i915_pm_rpm@module-reload.html

  
#### Warnings ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-kbl-x1275:       [DMESG-WARN][9] ([i915#62] / [i915#92]) -> [DMESG-WARN][10] ([i915#62] / [i915#92] / [i915#95]) +5 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_force_connector_basic@force-edid:
    - fi-kbl-x1275:       [DMESG-WARN][11] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][12] ([i915#62] / [i915#92]) +4 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2249]: https://gitlab.freedesktop.org/drm/intel/issues/2249
  [i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (44 -> 38)
------------------------------

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_8857 -> Patchwork_18323

  CI-20190529: 20190529
  CI_DRM_8857: 009d7480effe9b4d0d70a15f97fe21f80bfcbc25 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5765: 9f0977284d54ed37496260988dfcd6d2ad72dd1e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18323: b59671512b13c003557926c0c0a16630eea0266a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b59671512b13 drm/i915/gt: Free stale request on destroying the virtual engine
0d54a2698fa2 drm/i915/gt: Split the breadcrumb spinlock between global and contexts
2debc1598e3e drm/i915/gt: Don't cancel the interrupt shadow too early
b73b5a3bfec2 drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
c1f2e1672d57 drm/i915/gt: Defer enabling the breadcrumb interrupt to after submission
b60c43f1b554 drm/i915/gt: Protect context lifetime with RCU
423786160985 drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/index.html

[-- Attachment #1.2: Type: text/html, Size: 6263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 15019 bytes --]

== Series Details ==

Series: series starting with [1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling() (rev2)
URL   : https://patchwork.freedesktop.org/series/80393/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8857_full -> Patchwork_18323_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18323_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18323_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18323_full:

### IGT changes ###

#### Possible regressions ####

  * igt@perf_pmu@init-wait@vcs0:
    - shard-snb:          [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-snb6/igt@perf_pmu@init-wait@vcs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-snb6/igt@perf_pmu@init-wait@vcs0.html

  
Known issues
------------

  Here are the changes found in Patchwork_18323_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_persistence@legacy-engines-mixed-process@render:
    - shard-skl:          [PASS][3] -> [FAIL][4] ([i915#1528])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl3/igt@gem_ctx_persistence@legacy-engines-mixed-process@render.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl8/igt@gem_ctx_persistence@legacy-engines-mixed-process@render.html

  * igt@gem_exec_whisper@basic-queues-forked-all:
    - shard-glk:          [PASS][5] -> [DMESG-WARN][6] ([i915#118] / [i915#95])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-glk4/igt@gem_exec_whisper@basic-queues-forked-all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-glk3/igt@gem_exec_whisper@basic-queues-forked-all.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-iclb:         [PASS][7] -> [FAIL][8] ([i915#454])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-iclb8/igt@i915_pm_dc@dc6-psr.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-iclb2/igt@i915_pm_dc@dc6-psr.html

  * igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-mmap-cpu:
    - shard-kbl:          [PASS][9] -> [DMESG-WARN][10] ([i915#1982])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-mmap-cpu.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-mmap-cpu.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-mmap-cpu:
    - shard-tglb:         [PASS][11] -> [DMESG-WARN][12] ([i915#1982]) +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-tglb2/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-mmap-cpu.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-tglb7/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-mmap-cpu.html

  * igt@kms_hdr@bpc-switch:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([i915#1188])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl8/igt@kms_hdr@bpc-switch.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl10/igt@kms_hdr@bpc-switch.html

  * igt@kms_plane@plane-position-covered-pipe-b-planes:
    - shard-skl:          [PASS][15] -> [DMESG-WARN][16] ([i915#1982]) +6 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl9/igt@kms_plane@plane-position-covered-pipe-b-planes.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl8/igt@kms_plane@plane-position-covered-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([fdo#108145] / [i915#265])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@psr2_no_drrs:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-iclb2/igt@kms_psr@psr2_no_drrs.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-iclb1/igt@kms_psr@psr2_no_drrs.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [PASS][21] -> [DMESG-WARN][22] ([i915#180]) +6 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-kbl2/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-kbl2/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  * igt@kms_vblank@pipe-b-wait-forked-busy-hang:
    - shard-kbl:          [PASS][23] -> [DMESG-WARN][24] ([i915#62] / [i915#92]) +6 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-kbl6/igt@kms_vblank@pipe-b-wait-forked-busy-hang.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-kbl6/igt@kms_vblank@pipe-b-wait-forked-busy-hang.html

  
#### Possible fixes ####

  * igt@i915_selftest@mock@contexts:
    - shard-skl:          [INCOMPLETE][25] ([i915#198]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl8/igt@i915_selftest@mock@contexts.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl2/igt@i915_selftest@mock@contexts.html
    - shard-apl:          [INCOMPLETE][27] ([i915#1635]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-apl8/igt@i915_selftest@mock@contexts.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-apl4/igt@i915_selftest@mock@contexts.html

  * igt@kms_big_fb@linear-64bpp-rotate-180:
    - shard-glk:          [DMESG-FAIL][29] ([i915#118] / [i915#95]) -> [PASS][30] +1 similar issue
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-glk8/igt@kms_big_fb@linear-64bpp-rotate-180.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-glk9/igt@kms_big_fb@linear-64bpp-rotate-180.html

  * igt@kms_color@pipe-b-ctm-negative:
    - shard-skl:          [DMESG-WARN][31] ([i915#1982]) -> [PASS][32] +18 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl3/igt@kms_color@pipe-b-ctm-negative.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl3/igt@kms_color@pipe-b-ctm-negative.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][33] ([i915#180]) -> [PASS][34] +6 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_edge_walk@pipe-c-128x128-bottom-edge:
    - shard-glk:          [DMESG-WARN][35] ([i915#1982]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-glk5/igt@kms_cursor_edge_walk@pipe-c-128x128-bottom-edge.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-glk1/igt@kms_cursor_edge_walk@pipe-c-128x128-bottom-edge.html

  * igt@kms_cursor_legacy@pipe-a-torture-move:
    - shard-glk:          [DMESG-WARN][37] ([i915#118] / [i915#95]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-glk8/igt@kms_cursor_legacy@pipe-a-torture-move.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-glk9/igt@kms_cursor_legacy@pipe-a-torture-move.html

  * igt@kms_flip@2x-flip-vs-fences-interruptible@ab-vga1-hdmi-a1:
    - shard-hsw:          [DMESG-WARN][39] ([i915#1982]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-hsw6/igt@kms_flip@2x-flip-vs-fences-interruptible@ab-vga1-hdmi-a1.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-hsw4/igt@kms_flip@2x-flip-vs-fences-interruptible@ab-vga1-hdmi-a1.html

  * igt@kms_flip@flip-vs-absolute-wf_vblank@b-hdmi-a1:
    - shard-glk:          [FAIL][41] ([i915#2122]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-glk8/igt@kms_flip@flip-vs-absolute-wf_vblank@b-hdmi-a1.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-glk4/igt@kms_flip@flip-vs-absolute-wf_vblank@b-hdmi-a1.html

  * igt@kms_hdr@bpc-switch-suspend:
    - shard-skl:          [FAIL][43] ([i915#1188]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl4/igt@kms_hdr@bpc-switch-suspend.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl5/igt@kms_hdr@bpc-switch-suspend.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [FAIL][45] ([fdo#108145] / [i915#265]) -> [PASS][46] +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl8/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [SKIP][47] ([fdo#109642] / [fdo#111068]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-iclb8/igt@kms_psr2_su@frontbuffer.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_sprite_plane_onoff:
    - shard-iclb:         [SKIP][49] ([fdo#109441]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-iclb7/igt@kms_psr@psr2_sprite_plane_onoff.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-iclb2/igt@kms_psr@psr2_sprite_plane_onoff.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][51] ([i915#31]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-kbl4/igt@kms_setmode@basic.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-kbl3/igt@kms_setmode@basic.html

  
#### Warnings ####

  * igt@gem_exec_reloc@basic-concurrent16:
    - shard-snb:          [FAIL][53] ([i915#1930]) -> [TIMEOUT][54] ([i915#1958])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-snb6/igt@gem_exec_reloc@basic-concurrent16.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-snb6/igt@gem_exec_reloc@basic-concurrent16.html

  * igt@i915_pm_dc@dc3co-vpb-simulation:
    - shard-iclb:         [SKIP][55] ([i915#588]) -> [SKIP][56] ([i915#658])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-iclb4/igt@i915_pm_dc@dc3co-vpb-simulation.html

  * igt@kms_content_protection@atomic:
    - shard-kbl:          [TIMEOUT][57] ([i915#1319]) -> [TIMEOUT][58] ([i915#1319] / [i915#1958])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-kbl7/igt@kms_content_protection@atomic.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-kbl4/igt@kms_content_protection@atomic.html

  * igt@kms_flip@plain-flip-ts-check-interruptible@a-edp1:
    - shard-skl:          [DMESG-FAIL][59] ([i915#1982]) -> [DMESG-WARN][60] ([i915#1982])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-skl5/igt@kms_flip@plain-flip-ts-check-interruptible@a-edp1.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-skl4/igt@kms_flip@plain-flip-ts-check-interruptible@a-edp1.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-render:
    - shard-snb:          [SKIP][61] ([fdo#109271]) -> [TIMEOUT][62] ([i915#1958]) +3 similar issues
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8857/shard-snb6/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-render.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/shard-snb6/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-render.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#1319]: https://gitlab.freedesktop.org/drm/intel/issues/1319
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1930]: https://gitlab.freedesktop.org/drm/intel/issues/1930
  [i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (11 -> 11)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_8857 -> Patchwork_18323

  CI-20190529: 20190529
  CI_DRM_8857: 009d7480effe9b4d0d70a15f97fe21f80bfcbc25 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5765: 9f0977284d54ed37496260988dfcd6d2ad72dd1e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18323: b59671512b13c003557926c0c0a16630eea0266a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18323/index.html

[-- Attachment #1.2: Type: text/html, Size: 17652 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[Intel-gfx] [PATCH 1/7] drm/i915/gt: Remove defunct intel_virtual_engine_get_sibling()

[Chris Wilson]

As the last user was eliminated in commit e21fecdcde40 ("drm/i915/gt:
Distinguish the virtual breadcrumbs from the irq breadcrumbs"), we can
remove the function. One less implementation detail creeping beyond its
scope.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 12 ------------
 drivers/gpu/drm/i915/gt/intel_lrc.h |  4 ----
 2 files changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 417f6b0c6c61..0c632f15f677 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -5882,18 +5882,6 @@ int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine,
 	return 0;
 }
 
-struct intel_engine_cs *
-intel_virtual_engine_get_sibling(struct intel_engine_cs *engine,
-				 unsigned int sibling)
-{
-	struct virtual_engine *ve = to_virtual_engine(engine);
-
-	if (sibling >= ve->num_siblings)
-		return NULL;
-
-	return ve->siblings[sibling];
-}
-
 void intel_execlists_show_requests(struct intel_engine_cs *engine,
 				   struct drm_printer *m,
 				   void (*show_request)(struct drm_printer *m,
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h b/drivers/gpu/drm/i915/gt/intel_lrc.h
index 91fd8e452d9b..c2d287f25497 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
@@ -121,10 +121,6 @@ int intel_virtual_engine_attach_bond(struct intel_engine_cs *engine,
 				     const struct intel_engine_cs *master,
 				     const struct intel_engine_cs *sibling);
 
-struct intel_engine_cs *
-intel_virtual_engine_get_sibling(struct intel_engine_cs *engine,
-				 unsigned int sibling);
-
 bool
 intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine);
 
-- 
2.20.1

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

[Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Chris Wilson]

As we funnel more and more contexts into the breadcrumbs on an engine,
the hold time of b->irq_lock grows. As we may then contend with the
b->irq_lock during request submission, this increases the burden upon
the engine->active.lock and so directly impacts both our execution
latency and client latency. If we split the b->irq_lock by introducing a
per-context spinlock to manage the signalers within a context, we then
only need the b->irq_lock for enabling/disabling the interrupt and can
avoid taking the lock for walking the list of contexts within the signal
worker. Even with the current setup, this greatly reduces the number of
times we have to take and fight for b->irq_lock.

Furthermore, this closes the race between enabling the signaling context
while it is in the process of being signaled and removed:

<4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
<4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
<4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
<4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
<4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
<4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
<4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
<4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
<4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
<4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
<4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
<4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
<4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
<4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
<4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
<4>[  416.208675] PKRU: 55555554
<4>[  416.208677] Call Trace:
<4>[  416.208679]  <IRQ>
<4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
<4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
<4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
<4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
<4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
<4>[  416.208954]  __do_softirq+0xdf/0x498
<4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
<4>[  416.208964]  asm_call_on_stack+0xf/0x20
<4>[  416.208966]  </IRQ>
<4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
<4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
<4>[  416.208976]  common_interrupt+0xf7/0x260
<4>[  416.208980]  asm_common_interrupt+0x1e/0x40
<4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
<4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
<4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
<4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
<4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
<4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
<4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
<4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
<4>[  416.209012]  cpuidle_enter+0x24/0x40
<4>[  416.209016]  do_idle+0x22f/0x2d0
<4>[  416.209022]  cpu_startup_entry+0x14/0x20
<4>[  416.209025]  start_secondary+0x158/0x1a0
<4>[  416.209030]  secondary_startup_64+0xa4/0xb0
<4>[  416.209039] irq event stamp: 10186977
<4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
<4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
<4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
<4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20

<4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
<4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
<4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
<4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
<4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
<4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
<4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
<4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
<4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
<4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
<4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
<4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
<4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
<4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
<4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
<4>[  416.209317] PKRU: 55555554
<4>[  416.209317] Call Trace:
<4>[  416.209317]  <IRQ>
<4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
<4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]

Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 159 ++++++++++--------
 .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
 drivers/gpu/drm/i915/gt/intel_context.c       |   1 +
 drivers/gpu/drm/i915/gt/intel_context_types.h |   1 +
 4 files changed, 92 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index c290a47a96e3..7c9bde83f627 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
 	intel_gt_pm_put_async(b->irq_engine->gt);
 }
 
+static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
+{
+	spin_lock(&b->irq_lock);
+	if (b->irq_armed)
+		__intel_breadcrumbs_disarm_irq(b);
+	spin_unlock(&b->irq_lock);
+}
+
 static void add_signaling_context(struct intel_breadcrumbs *b,
 				  struct intel_context *ce)
 {
-	intel_context_get(ce);
-	list_add_tail(&ce->signal_link, &b->signalers);
+	lockdep_assert_held(&b->signalers_lock);
+	list_add_rcu(&ce->signal_link, &b->signalers);
 }
 
 static void remove_signaling_context(struct intel_breadcrumbs *b,
 				     struct intel_context *ce)
 {
-	list_del(&ce->signal_link);
-	intel_context_put(ce);
+	spin_lock(&b->signalers_lock);
+	list_del_rcu(&ce->signal_link);
+	spin_unlock(&b->signalers_lock);
 }
 
 static inline bool __request_completed(const struct i915_request *rq)
@@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
 	struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
 	const ktime_t timestamp = ktime_get();
 	struct llist_node *signal, *sn;
-	struct intel_context *ce, *cn;
-	struct list_head *pos, *next;
+	struct intel_context *ce;
 
 	signal = NULL;
 	if (unlikely(!llist_empty(&b->signaled_requests)))
 		signal = llist_del_all(&b->signaled_requests);
 
-	spin_lock(&b->irq_lock);
-
 	/*
 	 * Keep the irq armed until the interrupt after all listeners are gone.
 	 *
@@ -230,10 +236,18 @@ static void signal_irq_work(struct irq_work *work)
 	 * like powertop.
 	 */
 	if (!signal && b->irq_armed && list_empty(&b->signalers))
-		__intel_breadcrumbs_disarm_irq(b);
+		intel_breadcrumbs_disarm_irq(b);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+		struct list_head *pos, *next;
+		bool release = false;
 
-	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
-		GEM_BUG_ON(list_empty(&ce->signals));
+		if (!spin_trylock(&ce->signal_lock))
+			continue;
+
+		if (list_empty(&ce->signals))
+			goto unlock;
 
 		list_for_each_safe(pos, next, &ce->signals) {
 			struct i915_request *rq =
@@ -264,11 +278,16 @@ static void signal_irq_work(struct irq_work *work)
 			if (&ce->signals == pos) { /* now empty */
 				add_retire(b, ce->timeline);
 				remove_signaling_context(b, ce);
+				release = true;
 			}
 		}
-	}
 
-	spin_unlock(&b->irq_lock);
+unlock:
+		spin_unlock(&ce->signal_lock);
+		if (release)
+			intel_context_put(ce);
+	}
+	rcu_read_unlock();
 
 	llist_for_each_safe(signal, sn, signal) {
 		struct i915_request *rq =
@@ -297,14 +316,15 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
 	if (!b)
 		return NULL;
 
-	spin_lock_init(&b->irq_lock);
+	b->irq_engine = irq_engine;
+
+	spin_lock_init(&b->signalers_lock);
 	INIT_LIST_HEAD(&b->signalers);
 	init_llist_head(&b->signaled_requests);
 
+	spin_lock_init(&b->irq_lock);
 	init_irq_work(&b->irq_work, signal_irq_work);
 
-	b->irq_engine = irq_engine;
-
 	return b;
 }
 
@@ -343,9 +363,9 @@ void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
 	kfree(b);
 }
 
-static void insert_breadcrumb(struct i915_request *rq,
-			      struct intel_breadcrumbs *b)
+static void insert_breadcrumb(struct i915_request *rq)
 {
+	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
 	struct intel_context *ce = rq->context;
 	struct list_head *pos;
 
@@ -367,7 +387,33 @@ static void insert_breadcrumb(struct i915_request *rq,
 	}
 
 	if (list_empty(&ce->signals)) {
+		/*
+		 * rq->engine is locked by rq->engine->active.lock. That
+		 * however is not known until after rq->engine has been
+		 * dereferenced and the lock acquired. Hence we acquire the
+		 * lock and then validate that rq->engine still matches the
+		 * lock we hold for it.
+		 *
+		 * Here, we are using the breadcrumb lock as a proxy for the
+		 * rq->engine->active.lock, and we know that since the
+		 * breadcrumb will be serialised within i915_request_submit
+		 * the engine cannot change while active as long as we hold
+		 * the breadcrumb lock on that engine.
+		 *
+		 * From the dma_fence_enable_signaling() path, we are outside
+		 * of the request submit/unsubmit path, and so we must be more
+		 * careful to acquire the right lock.
+		 */
+		intel_context_get(ce);
+		spin_lock(&b->signalers_lock);
+		while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
+			spin_unlock(&b->signalers_lock);
+			b = READ_ONCE(rq->engine)->breadcrumbs;
+			spin_lock(&b->signalers_lock);
+		}
 		add_signaling_context(b, ce);
+		spin_unlock(&b->signalers_lock);
+
 		pos = &ce->signals;
 	} else {
 		/*
@@ -406,7 +452,7 @@ static void insert_breadcrumb(struct i915_request *rq,
 
 bool i915_request_enable_breadcrumb(struct i915_request *rq)
 {
-	struct intel_breadcrumbs *b;
+	struct intel_context *ce = rq->context;
 
 	/* Serialises with i915_request_retire() using rq->lock */
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
@@ -421,67 +467,37 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
 	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
 		return true;
 
-	/*
-	 * rq->engine is locked by rq->engine->active.lock. That however
-	 * is not known until after rq->engine has been dereferenced and
-	 * the lock acquired. Hence we acquire the lock and then validate
-	 * that rq->engine still matches the lock we hold for it.
-	 *
-	 * Here, we are using the breadcrumb lock as a proxy for the
-	 * rq->engine->active.lock, and we know that since the breadcrumb
-	 * will be serialised within i915_request_submit/i915_request_unsubmit,
-	 * the engine cannot change while active as long as we hold the
-	 * breadcrumb lock on that engine.
-	 *
-	 * From the dma_fence_enable_signaling() path, we are outside of the
-	 * request submit/unsubmit path, and so we must be more careful to
-	 * acquire the right lock.
-	 */
-	b = READ_ONCE(rq->engine)->breadcrumbs;
-	spin_lock(&b->irq_lock);
-	while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
-		spin_unlock(&b->irq_lock);
-		b = READ_ONCE(rq->engine)->breadcrumbs;
-		spin_lock(&b->irq_lock);
-	}
-
-	/*
-	 * Now that we are finally serialised with request submit/unsubmit,
-	 * [with b->irq_lock] and with i915_request_retire() [via checking
-	 * SIGNALED with rq->lock] confirm the request is indeed active. If
-	 * it is no longer active, the breadcrumb will be attached upon
-	 * i915_request_submit().
-	 */
+	spin_lock(&ce->signal_lock);
 	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
-		insert_breadcrumb(rq, b);
-
-	spin_unlock(&b->irq_lock);
+		insert_breadcrumb(rq);
+	spin_unlock(&ce->signal_lock);
 
 	return true;
 }
 
 void i915_request_cancel_breadcrumb(struct i915_request *rq)
 {
-	struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
+	struct intel_context *ce = rq->context;
+	bool release = false;
 
-	/*
-	 * We must wait for b->irq_lock so that we know the interrupt handler
-	 * has released its reference to the intel_context and has completed
-	 * the DMA_FENCE_FLAG_SIGNALED_BIT/I915_FENCE_FLAG_SIGNAL dance (if
-	 * required).
-	 */
-	spin_lock(&b->irq_lock);
+	if (!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
+		return;
+
+	spin_lock(&ce->signal_lock);
 	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
-		struct intel_context *ce = rq->context;
+		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 
 		list_del(&rq->signal_link);
-		if (list_empty(&ce->signals))
-			remove_signaling_context(b, ce);
+		if (list_empty(&ce->signals)) {
+			remove_signaling_context(rq->engine->breadcrumbs, ce);
+			release = true;
+		}
 
-		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
 		i915_request_put(rq);
 	}
-	spin_unlock(&b->irq_lock);
+	spin_unlock(&ce->signal_lock);
+	if (release)
+		intel_context_put(ce);
 }
 
 static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
@@ -491,18 +507,19 @@ static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
 
 	drm_printf(p, "Signals:\n");
 
-	spin_lock_irq(&b->irq_lock);
-	list_for_each_entry(ce, &b->signalers, signal_link) {
-		list_for_each_entry(rq, &ce->signals, signal_link) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+		spin_lock_irq(&ce->signal_lock);
+		list_for_each_entry(rq, &ce->signals, signal_link)
 			drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
 				   rq->fence.context, rq->fence.seqno,
 				   i915_request_completed(rq) ? "!" :
 				   i915_request_started(rq) ? "*" :
 				   "",
 				   jiffies_to_msecs(jiffies - rq->emitted_jiffies));
-		}
+		spin_unlock_irq(&ce->signal_lock);
 	}
-	spin_unlock_irq(&b->irq_lock);
+	rcu_read_unlock();
 }
 
 void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
index 3fa19820b37a..a74bb3062bd8 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -29,18 +29,16 @@
  * the overhead of waking that client is much preferred.
  */
 struct intel_breadcrumbs {
-	spinlock_t irq_lock; /* protects the lists used in hardirq context */
-
 	/* Not all breadcrumbs are attached to physical HW */
 	struct intel_engine_cs *irq_engine;
 
+	spinlock_t signalers_lock; /* protects the list of signalers */
 	struct list_head signalers;
 	struct llist_head signaled_requests;
 
+	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
 	struct irq_work irq_work; /* for use from inside irq_lock */
-
 	unsigned int irq_enabled;
-
 	bool irq_armed;
 };
 
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index cb36cc26f95a..8ff3ca884adb 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -147,6 +147,7 @@ static void __intel_context_ctor(void *arg)
 {
 	struct intel_context *ce = arg;
 
+	spin_lock_init(&ce->signal_lock);
 	INIT_LIST_HEAD(&ce->signal_link);
 	INIT_LIST_HEAD(&ce->signals);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 4954b0df4864..a78c1c225ce3 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -51,6 +51,7 @@ struct intel_context {
 	struct i915_address_space *vm;
 	struct i915_gem_context __rcu *gem_context;
 
+	spinlock_t signal_lock;
 	struct list_head signal_link;
 	struct list_head signals;
 
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Chris Wilson]

Quoting Chris Wilson (2020-08-07 09:32:55)
> As we funnel more and more contexts into the breadcrumbs on an engine,
> the hold time of b->irq_lock grows. As we may then contend with the
> b->irq_lock during request submission, this increases the burden upon
> the engine->active.lock and so directly impacts both our execution
> latency and client latency. If we split the b->irq_lock by introducing a
> per-context spinlock to manage the signalers within a context, we then
> only need the b->irq_lock for enabling/disabling the interrupt and can
> avoid taking the lock for walking the list of contexts within the signal
> worker. Even with the current setup, this greatly reduces the number of
> times we have to take and fight for b->irq_lock.
> 
> Furthermore, this closes the race between enabling the signaling context
> while it is in the process of being signaled and removed:
> 
> <4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
> <4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
> <4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> <4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
> <4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> <4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
> <4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
> <4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
> <4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
> <4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
> <4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
> <4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
> <4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
> <4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> <4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> <4>[  416.208675] PKRU: 55555554
> <4>[  416.208677] Call Trace:
> <4>[  416.208679]  <IRQ>
> <4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
> <4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
> <4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
> <4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
> <4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
> <4>[  416.208954]  __do_softirq+0xdf/0x498
> <4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
> <4>[  416.208964]  asm_call_on_stack+0xf/0x20
> <4>[  416.208966]  </IRQ>
> <4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
> <4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
> <4>[  416.208976]  common_interrupt+0xf7/0x260
> <4>[  416.208980]  asm_common_interrupt+0x1e/0x40
> <4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
> <4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
> <4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
> <4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
> <4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
> <4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
> <4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
> <4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
> <4>[  416.209012]  cpuidle_enter+0x24/0x40
> <4>[  416.209016]  do_idle+0x22f/0x2d0
> <4>[  416.209022]  cpu_startup_entry+0x14/0x20
> <4>[  416.209025]  start_secondary+0x158/0x1a0
> <4>[  416.209030]  secondary_startup_64+0xa4/0xb0
> <4>[  416.209039] irq event stamp: 10186977
> <4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
> <4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
> <4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
> <4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20
> 
> <4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
> <4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
> <4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> <4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
> <4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> <4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
> <4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
> <4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
> <4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
> <4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
> <4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
> <4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
> <4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
> <4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> <4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> <4>[  416.209317] PKRU: 55555554
> <4>[  416.209317] Call Trace:
> <4>[  416.209317]  <IRQ>
> <4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
> <4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2276
> Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Tvrtko Ursulin]

On 07/08/2020 09:32, Chris Wilson wrote:
> As we funnel more and more contexts into the breadcrumbs on an engine,
> the hold time of b->irq_lock grows. As we may then contend with the
> b->irq_lock during request submission, this increases the burden upon
> the engine->active.lock and so directly impacts both our execution
> latency and client latency. If we split the b->irq_lock by introducing a
> per-context spinlock to manage the signalers within a context, we then
> only need the b->irq_lock for enabling/disabling the interrupt and can
> avoid taking the lock for walking the list of contexts within the signal
> worker. Even with the current setup, this greatly reduces the number of
> times we have to take and fight for b->irq_lock.
> 
> Furthermore, this closes the race between enabling the signaling context
> while it is in the process of being signaled and removed:

Where is this race present? Hope not in the patch preceding this one?!

> <4>[  416.208555] list_add corruption. prev->next should be next (ffff8881951d5910), but was dead000000000100. (prev=ffff8882781bb870).
> <4>[  416.208573] WARNING: CPU: 7 PID: 0 at lib/list_debug.c:28 __list_add_valid+0x4d/0x70
> <4>[  416.208575] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> <4>[  416.208611] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G     U            5.8.0-CI-CI_DRM_8852+ #1
> <4>[  416.208614] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> <4>[  416.208627] RIP: 0010:__list_add_valid+0x4d/0x70
> <4>[  416.208631] Code: c3 48 89 d1 48 c7 c7 60 18 33 82 48 89 c2 e8 ea e0 b6 ff 0f 0b 31 c0 c3 48 89 c1 4c 89 c6 48 c7 c7 b0 18 33 82 e8 d3 e0 b6 ff <0f> 0b 31 c0 c3 48 89 f2 4c 89 c1 48 89 fe 48 c7 c7 00 19 33 82 e8
> <4>[  416.208633] RSP: 0018:ffffc90000280e18 EFLAGS: 00010086
> <4>[  416.208636] RAX: 0000000000000000 RBX: ffff888250a44880 RCX: 0000000000000105
> <4>[  416.208639] RDX: 0000000000000105 RSI: ffffffff82320c5b RDI: 00000000ffffffff
> <4>[  416.208641] RBP: ffff8882781bb870 R08: 0000000000000000 R09: 0000000000000001
> <4>[  416.208643] R10: 00000000054d2957 R11: 000000006abbd991 R12: ffff8881951d58c8
> <4>[  416.208646] R13: ffff888286073880 R14: ffff888286073848 R15: ffff8881951d5910
> <4>[  416.208669] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> <4>[  416.208671] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[  416.208673] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> <4>[  416.208675] PKRU: 55555554
> <4>[  416.208677] Call Trace:
> <4>[  416.208679]  <IRQ>
> <4>[  416.208751]  i915_request_enable_breadcrumb+0x278/0x400 [i915]
> <4>[  416.208839]  __i915_request_submit+0xca/0x2a0 [i915]
> <4>[  416.208892]  __execlists_submission_tasklet+0x480/0x1830 [i915]
> <4>[  416.208942]  execlists_submission_tasklet+0xc4/0x130 [i915]
> <4>[  416.208947]  tasklet_action_common.isra.17+0x6c/0x1c0
> <4>[  416.208954]  __do_softirq+0xdf/0x498
> <4>[  416.208960]  ? handle_fasteoi_irq+0x150/0x150
> <4>[  416.208964]  asm_call_on_stack+0xf/0x20
> <4>[  416.208966]  </IRQ>
> <4>[  416.208969]  do_softirq_own_stack+0xa1/0xc0
> <4>[  416.208972]  irq_exit_rcu+0xb5/0xc0
> <4>[  416.208976]  common_interrupt+0xf7/0x260
> <4>[  416.208980]  asm_common_interrupt+0x1e/0x40
> <4>[  416.208985] RIP: 0010:cpuidle_enter_state+0xb6/0x410
> <4>[  416.208987] Code: 00 31 ff e8 9c 3e 89 ff 80 7c 24 0b 00 74 12 9c 58 f6 c4 02 0f 85 31 03 00 00 31 ff e8 e3 6c 90 ff e8 fe a4 94 ff fb 45 85 ed <0f> 88 c7 02 00 00 49 63 c5 4c 2b 24 24 48 8d 14 40 48 8d 14 90 48
> <4>[  416.208989] RSP: 0018:ffffc90000143e70 EFLAGS: 00000206
> <4>[  416.208991] RAX: 0000000000000007 RBX: ffffe8ffffda8070 RCX: 0000000000000000
> <4>[  416.208993] RDX: 0000000000000000 RSI: ffffffff8238b4ee RDI: ffffffff8233184f
> <4>[  416.208995] RBP: ffffffff826b4e00 R08: 0000000000000000 R09: 0000000000000000
> <4>[  416.208997] R10: 0000000000000001 R11: 0000000000000000 R12: 00000060e7f24a8f
> <4>[  416.208998] R13: 0000000000000003 R14: 0000000000000003 R15: 0000000000000003
> <4>[  416.209012]  cpuidle_enter+0x24/0x40
> <4>[  416.209016]  do_idle+0x22f/0x2d0
> <4>[  416.209022]  cpu_startup_entry+0x14/0x20
> <4>[  416.209025]  start_secondary+0x158/0x1a0
> <4>[  416.209030]  secondary_startup_64+0xa4/0xb0
> <4>[  416.209039] irq event stamp: 10186977
> <4>[  416.209042] hardirqs last  enabled at (10186976): [<ffffffff810b9363>] tasklet_action_common.isra.17+0xe3/0x1c0
> <4>[  416.209044] hardirqs last disabled at (10186977): [<ffffffff81a5e5ed>] _raw_spin_lock_irqsave+0xd/0x50
> <4>[  416.209047] softirqs last  enabled at (10186968): [<ffffffff810b9a1a>] irq_enter_rcu+0x6a/0x70
> <4>[  416.209049] softirqs last disabled at (10186969): [<ffffffff81c00f4f>] asm_call_on_stack+0xf/0x20
> 
> <4>[  416.209317] list_del corruption, ffff8882781bb870->next is LIST_POISON1 (dead000000000100)
> <4>[  416.209317] WARNING: CPU: 7 PID: 46 at lib/list_debug.c:47 __list_del_entry_valid+0x4e/0x90
> <4>[  416.209317] Modules linked in: i915(+) vgem snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp x86_pkg_temp_thermal coretemp ax88179_178a usbnet mii crct10dif_pclmul snd_intel_dspcfg crc32_pclmul snd_hda_codec snd_hwdep ghash_clmulni_intel snd_hda_core e1000e snd_pcm ptp pps_core mei_me mei prime_numbers intel_lpss_pci [last unloaded: i915]
> <4>[  416.209317] CPU: 7 PID: 46 Comm: ksoftirqd/7 Tainted: G     U  W         5.8.0-CI-CI_DRM_8852+ #1
> <4>[  416.209317] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake Y LPDDR4x T4 RVP TLC, BIOS ICLSFWR1.R00.3212.A00.1905212112 05/21/2019
> <4>[  416.209317] RIP: 0010:__list_del_entry_valid+0x4e/0x90
> <4>[  416.209317] Code: 2e 48 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00 c3 48 89 fe 48 89 c2 48 c7 c7 38 19 33 82 e8 62 e0 b6 ff <0f> 0b 31 c0 c3 48 89 fe 48 c7 c7 70 19 33 82 e8 4e e0 b6 ff 0f 0b
> <4>[  416.209317] RSP: 0018:ffffc90000280de8 EFLAGS: 00010086
> <4>[  416.209317] RAX: 0000000000000000 RBX: ffff8882781bb848 RCX: 0000000000010104
> <4>[  416.209317] RDX: 0000000000010104 RSI: ffffffff8238b4ee RDI: 00000000ffffffff
> <4>[  416.209317] RBP: ffff8882781bb880 R08: 0000000000000000 R09: 0000000000000001
> <4>[  416.209317] R10: 000000009fb6666e R11: 00000000feca9427 R12: ffffc90000280e18
> <4>[  416.209317] R13: ffff8881951d5930 R14: dead0000000000d8 R15: ffff8882781bb880
> <4>[  416.209317] FS:  0000000000000000(0000) GS:ffff88829c180000(0000) knlGS:0000000000000000
> <4>[  416.209317] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[  416.209317] CR2: 0000556231326c48 CR3: 0000000005610001 CR4: 0000000000760ee0
> <4>[  416.209317] PKRU: 55555554
> <4>[  416.209317] Call Trace:
> <4>[  416.209317]  <IRQ>
> <4>[  416.209317]  remove_signaling_context.isra.13+0xd/0x70 [i915]
> <4>[  416.209513]  signal_irq_work+0x1f7/0x4b0 [i915]
> 
> Fixes: bda4d4db6dd6 ("drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c   | 159 ++++++++++--------
>   .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |   6 +-
>   drivers/gpu/drm/i915/gt/intel_context.c       |   1 +
>   drivers/gpu/drm/i915/gt/intel_context_types.h |   1 +
>   4 files changed, 92 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index c290a47a96e3..7c9bde83f627 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -101,18 +101,27 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
>   	intel_gt_pm_put_async(b->irq_engine->gt);
>   }
>   
> +static void intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> +{
> +	spin_lock(&b->irq_lock);
> +	if (b->irq_armed)
> +		__intel_breadcrumbs_disarm_irq(b);
> +	spin_unlock(&b->irq_lock);
> +}
> +
>   static void add_signaling_context(struct intel_breadcrumbs *b,
>   				  struct intel_context *ce)
>   {
> -	intel_context_get(ce);
> -	list_add_tail(&ce->signal_link, &b->signalers);
> +	lockdep_assert_held(&b->signalers_lock);
> +	list_add_rcu(&ce->signal_link, &b->signalers);
>   }
>   
>   static void remove_signaling_context(struct intel_breadcrumbs *b,
>   				     struct intel_context *ce)
>   {
> -	list_del(&ce->signal_link);
> -	intel_context_put(ce);
> +	spin_lock(&b->signalers_lock);
> +	list_del_rcu(&ce->signal_link);
> +	spin_unlock(&b->signalers_lock);
>   }
>   
>   static inline bool __request_completed(const struct i915_request *rq)
> @@ -195,15 +204,12 @@ static void signal_irq_work(struct irq_work *work)
>   	struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
>   	const ktime_t timestamp = ktime_get();
>   	struct llist_node *signal, *sn;
> -	struct intel_context *ce, *cn;
> -	struct list_head *pos, *next;
> +	struct intel_context *ce;
>   
>   	signal = NULL;
>   	if (unlikely(!llist_empty(&b->signaled_requests)))
>   		signal = llist_del_all(&b->signaled_requests);
>   
> -	spin_lock(&b->irq_lock);
> -
>   	/*
>   	 * Keep the irq armed until the interrupt after all listeners are gone.
>   	 *
> @@ -230,10 +236,18 @@ static void signal_irq_work(struct irq_work *work)
>   	 * like powertop.
>   	 */
>   	if (!signal && b->irq_armed && list_empty(&b->signalers))
> -		__intel_breadcrumbs_disarm_irq(b);
> +		intel_breadcrumbs_disarm_irq(b);
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> +		struct list_head *pos, *next;
> +		bool release = false;
>   
> -	list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
> -		GEM_BUG_ON(list_empty(&ce->signals));
> +		if (!spin_trylock(&ce->signal_lock))

Trylock makes me think the solution is not completely elegant even 
before I figured it all out. So going back to the first question - can 
you fix the race in scope of the current locking scheme to start with?

Regards,

Tvrtko

> +			continue;
> +
> +		if (list_empty(&ce->signals))
> +			goto unlock;
>   
>   		list_for_each_safe(pos, next, &ce->signals) {
>   			struct i915_request *rq =
> @@ -264,11 +278,16 @@ static void signal_irq_work(struct irq_work *work)
>   			if (&ce->signals == pos) { /* now empty */
>   				add_retire(b, ce->timeline);
>   				remove_signaling_context(b, ce);
> +				release = true;
>   			}
>   		}
> -	}
>   
> -	spin_unlock(&b->irq_lock);
> +unlock:
> +		spin_unlock(&ce->signal_lock);
> +		if (release)
> +			intel_context_put(ce);
> +	}
> +	rcu_read_unlock();
>   
>   	llist_for_each_safe(signal, sn, signal) {
>   		struct i915_request *rq =
> @@ -297,14 +316,15 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
>   	if (!b)
>   		return NULL;
>   
> -	spin_lock_init(&b->irq_lock);
> +	b->irq_engine = irq_engine;
> +
> +	spin_lock_init(&b->signalers_lock);
>   	INIT_LIST_HEAD(&b->signalers);
>   	init_llist_head(&b->signaled_requests);
>   
> +	spin_lock_init(&b->irq_lock);
>   	init_irq_work(&b->irq_work, signal_irq_work);
>   
> -	b->irq_engine = irq_engine;
> -
>   	return b;
>   }
>   
> @@ -343,9 +363,9 @@ void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
>   	kfree(b);
>   }
>   
> -static void insert_breadcrumb(struct i915_request *rq,
> -			      struct intel_breadcrumbs *b)
> +static void insert_breadcrumb(struct i915_request *rq)
>   {
> +	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
>   	struct intel_context *ce = rq->context;
>   	struct list_head *pos;
>   
> @@ -367,7 +387,33 @@ static void insert_breadcrumb(struct i915_request *rq,
>   	}
>   
>   	if (list_empty(&ce->signals)) {
> +		/*
> +		 * rq->engine is locked by rq->engine->active.lock. That
> +		 * however is not known until after rq->engine has been
> +		 * dereferenced and the lock acquired. Hence we acquire the
> +		 * lock and then validate that rq->engine still matches the
> +		 * lock we hold for it.
> +		 *
> +		 * Here, we are using the breadcrumb lock as a proxy for the
> +		 * rq->engine->active.lock, and we know that since the
> +		 * breadcrumb will be serialised within i915_request_submit
> +		 * the engine cannot change while active as long as we hold
> +		 * the breadcrumb lock on that engine.
> +		 *
> +		 * From the dma_fence_enable_signaling() path, we are outside
> +		 * of the request submit/unsubmit path, and so we must be more
> +		 * careful to acquire the right lock.
> +		 */
> +		intel_context_get(ce);
> +		spin_lock(&b->signalers_lock);
> +		while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
> +			spin_unlock(&b->signalers_lock);
> +			b = READ_ONCE(rq->engine)->breadcrumbs;
> +			spin_lock(&b->signalers_lock);
> +		}
>   		add_signaling_context(b, ce);
> +		spin_unlock(&b->signalers_lock);
> +
>   		pos = &ce->signals;
>   	} else {
>   		/*
> @@ -406,7 +452,7 @@ static void insert_breadcrumb(struct i915_request *rq,
>   
>   bool i915_request_enable_breadcrumb(struct i915_request *rq)
>   {
> -	struct intel_breadcrumbs *b;
> +	struct intel_context *ce = rq->context;
>   
>   	/* Serialises with i915_request_retire() using rq->lock */
>   	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
> @@ -421,67 +467,37 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>   	if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
>   		return true;
>   
> -	/*
> -	 * rq->engine is locked by rq->engine->active.lock. That however
> -	 * is not known until after rq->engine has been dereferenced and
> -	 * the lock acquired. Hence we acquire the lock and then validate
> -	 * that rq->engine still matches the lock we hold for it.
> -	 *
> -	 * Here, we are using the breadcrumb lock as a proxy for the
> -	 * rq->engine->active.lock, and we know that since the breadcrumb
> -	 * will be serialised within i915_request_submit/i915_request_unsubmit,
> -	 * the engine cannot change while active as long as we hold the
> -	 * breadcrumb lock on that engine.
> -	 *
> -	 * From the dma_fence_enable_signaling() path, we are outside of the
> -	 * request submit/unsubmit path, and so we must be more careful to
> -	 * acquire the right lock.
> -	 */
> -	b = READ_ONCE(rq->engine)->breadcrumbs;
> -	spin_lock(&b->irq_lock);
> -	while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
> -		spin_unlock(&b->irq_lock);
> -		b = READ_ONCE(rq->engine)->breadcrumbs;
> -		spin_lock(&b->irq_lock);
> -	}
> -
> -	/*
> -	 * Now that we are finally serialised with request submit/unsubmit,
> -	 * [with b->irq_lock] and with i915_request_retire() [via checking
> -	 * SIGNALED with rq->lock] confirm the request is indeed active. If
> -	 * it is no longer active, the breadcrumb will be attached upon
> -	 * i915_request_submit().
> -	 */
> +	spin_lock(&ce->signal_lock);
>   	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> -		insert_breadcrumb(rq, b);
> -
> -	spin_unlock(&b->irq_lock);
> +		insert_breadcrumb(rq);
> +	spin_unlock(&ce->signal_lock);
>   
>   	return true;
>   }
>   
>   void i915_request_cancel_breadcrumb(struct i915_request *rq)
>   {
> -	struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
> +	struct intel_context *ce = rq->context;
> +	bool release = false;
>   
> -	/*
> -	 * We must wait for b->irq_lock so that we know the interrupt handler
> -	 * has released its reference to the intel_context and has completed
> -	 * the DMA_FENCE_FLAG_SIGNALED_BIT/I915_FENCE_FLAG_SIGNAL dance (if
> -	 * required).
> -	 */
> -	spin_lock(&b->irq_lock);
> +	if (!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
> +		return;
> +
> +	spin_lock(&ce->signal_lock);
>   	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
> -		struct intel_context *ce = rq->context;
> +		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>   
>   		list_del(&rq->signal_link);
> -		if (list_empty(&ce->signals))
> -			remove_signaling_context(b, ce);
> +		if (list_empty(&ce->signals)) {
> +			remove_signaling_context(rq->engine->breadcrumbs, ce);
> +			release = true;
> +		}
>   
> -		clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>   		i915_request_put(rq);
>   	}
> -	spin_unlock(&b->irq_lock);
> +	spin_unlock(&ce->signal_lock);
> +	if (release)
> +		intel_context_put(ce);
>   }
>   
>   static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
> @@ -491,18 +507,19 @@ static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
>   
>   	drm_printf(p, "Signals:\n");
>   
> -	spin_lock_irq(&b->irq_lock);
> -	list_for_each_entry(ce, &b->signalers, signal_link) {
> -		list_for_each_entry(rq, &ce->signals, signal_link) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> +		spin_lock_irq(&ce->signal_lock);
> +		list_for_each_entry(rq, &ce->signals, signal_link)
>   			drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
>   				   rq->fence.context, rq->fence.seqno,
>   				   i915_request_completed(rq) ? "!" :
>   				   i915_request_started(rq) ? "*" :
>   				   "",
>   				   jiffies_to_msecs(jiffies - rq->emitted_jiffies));
> -		}
> +		spin_unlock_irq(&ce->signal_lock);
>   	}
> -	spin_unlock_irq(&b->irq_lock);
> +	rcu_read_unlock();
>   }
>   
>   void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> index 3fa19820b37a..a74bb3062bd8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> @@ -29,18 +29,16 @@
>    * the overhead of waking that client is much preferred.
>    */
>   struct intel_breadcrumbs {
> -	spinlock_t irq_lock; /* protects the lists used in hardirq context */
> -
>   	/* Not all breadcrumbs are attached to physical HW */
>   	struct intel_engine_cs *irq_engine;
>   
> +	spinlock_t signalers_lock; /* protects the list of signalers */
>   	struct list_head signalers;
>   	struct llist_head signaled_requests;
>   
> +	spinlock_t irq_lock; /* protects the interrupt from hardirq context */
>   	struct irq_work irq_work; /* for use from inside irq_lock */
> -
>   	unsigned int irq_enabled;
> -
>   	bool irq_armed;
>   };
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index cb36cc26f95a..8ff3ca884adb 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -147,6 +147,7 @@ static void __intel_context_ctor(void *arg)
>   {
>   	struct intel_context *ce = arg;
>   
> +	spin_lock_init(&ce->signal_lock);
>   	INIT_LIST_HEAD(&ce->signal_link);
>   	INIT_LIST_HEAD(&ce->signals);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 4954b0df4864..a78c1c225ce3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -51,6 +51,7 @@ struct intel_context {
>   	struct i915_address_space *vm;
>   	struct i915_gem_context __rcu *gem_context;
>   
> +	spinlock_t signal_lock;
>   	struct list_head signal_link;
>   	struct list_head signals;
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/7] drm/i915/gt: Split the breadcrumb spinlock between global and contexts

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-08-07 12:54:48)
> 
> On 07/08/2020 09:32, Chris Wilson wrote:
> > As we funnel more and more contexts into the breadcrumbs on an engine,
> > the hold time of b->irq_lock grows. As we may then contend with the
> > b->irq_lock during request submission, this increases the burden upon
> > the engine->active.lock and so directly impacts both our execution
> > latency and client latency. If we split the b->irq_lock by introducing a
> > per-context spinlock to manage the signalers within a context, we then
> > only need the b->irq_lock for enabling/disabling the interrupt and can
> > avoid taking the lock for walking the list of contexts within the signal
> > worker. Even with the current setup, this greatly reduces the number of
> > times we have to take and fight for b->irq_lock.
> > 
> > Furthermore, this closes the race between enabling the signaling context
> > while it is in the process of being signaled and removed:
> 
> Where is this race present? Hope not in the patch preceding this one?!

No, unfortunately this patch was fixing a bug as a part of the previous
series so it wasn't detected by CI until after the first pair were merged
(even though that pair were run separately just in case).

So what's happening is that as we remove the request from ce->signals,
we add a request to ce->signals ... on a different engine.

We took b->irq_lock for rq1->engine[vcs0] and then rq2->engine[vcs1].
Since we are holding the b->irq_lock on the active engine for each
request...

Ouch.

> > -     list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
> > -             GEM_BUG_ON(list_empty(&ce->signals));
> > +             if (!spin_trylock(&ce->signal_lock))
> 
> Trylock makes me think the solution is not completely elegant even 
> before I figured it all out. So going back to the first question - can 
> you fix the race in scope of the current locking scheme to start with?

The trylock is just because we can have signal_irq_worker running on
multiple cpus simultaneously. It's nothing to do with the locking
requirements, just skipping over a wait before doing no work.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx