intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing
@ 2020-05-21  8:53 Chris Wilson
  2020-05-21  8:53 ` [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release Chris Wilson
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  8:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

Since the remove of the no-semaphore boosting, we rely on timeslicing to
reorder past inter-dependency hogs across the engines. However, we
require preemption to support timeslicing into user payloads, and not all
machine support preemption so we do not universally enable timeslicing
even when it would preempt our own inter-engine semaphores.

Testcase: igt/gem_exec_schedule/semaphore-codependency # bdw/bsw
Fixes: 18e4af04d218 ("drm/i915: Drop no-semaphore boosting")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 900ea8b7fc8f..f5d59d18cd5b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -230,7 +230,7 @@ static void intel_context_set_gem(struct intel_context *ce,
 		ce->timeline = intel_timeline_get(ctx->timeline);
 
 	if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
-	    intel_engine_has_semaphores(ce->engine))
+	    intel_engine_has_timeslices(ce->engine))
 		__set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
 }
 
@@ -1969,7 +1969,7 @@ static int __apply_priority(struct intel_context *ce, void *arg)
 {
 	struct i915_gem_context *ctx = arg;
 
-	if (!intel_engine_has_semaphores(ce->engine))
+	if (!intel_engine_has_timeslices(ce->engine))
 		return 0;
 
 	if (ctx->sched.priority >= I915_PRIORITY_NORMAL)
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release
  2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
@ 2020-05-21  8:53 ` Chris Wilson
  2020-05-21  9:13   ` Tvrtko Ursulin
  2020-05-21  8:55 ` [Intel-gfx] [PATCH] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  8:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

In order to be valid to dereference during the i915_fence_release, after
retiring the fence and releasing its refererences, we assume that
rq->engine can only be a real engine (that stay intact until the device
is shutdown after all fences have been flushed). However, due to a quirk
of preempt-to-busy, we may retire a request that still belongs to a
virtual engine and so eventually free it with rq->engine being invalid.
To avoid dereferencing that invalid engine, we look at the
execution_mask which if it indicates it may be executed on more than one
engine, we know it originated on a virtual engine and may still be on
one.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 526c1e9acbd5..6e357183bece 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
 	i915_sw_fence_fini(&rq->submit);
 	i915_sw_fence_fini(&rq->semaphore);
 
-	/* Keep one request on each engine for reserved use under mempressure */
-	if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
+	/*
+	 * Keep one request on each engine for reserved use under mempressure
+	 *
+	 * We do not hold a reference to the engine here and so have to be
+	 * very careful in what rq->engine we poke. The virtual engine is
+	 * referenced via the rq->context and we released that ref during
+	 * i915_request_retire(), ergo we must not dereference a virtual
+	 * engine here. Not that we would want to, as the only consumer of
+	 * the reserved engine->request_pool is the powermanagent parking,
+	 * which must-not-fail, and that is only run on the physical engines.
+	 *
+	 * Since the request must have been executed to be have completed,
+	 * we know that it will have been processed by the HW and will
+	 * not be unsubmitted again, so rq->engine and rq->execution_mask
+	 * at this point is stable. rq->execution_mask will be a single
+	 * bit if the last and only engine it could execution on was a
+	 * physical engine, if it's multiple bits then it started on and
+	 * could still be on a virtual engine. Thus if the mask is not a
+	 * power-of-two we assume that rq->engine may still be a virtual
+	 * engien and so a dangling invalid pointer that we cannot dereference
+	 */
+	if (is_power_of_2(rq->execution_mask) &&
+	    !cmpxchg(&rq->engine->request_pool, NULL, rq))
 		return;
 
 	kmem_cache_free(global.slab_requests, rq);
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Intel-gfx] [PATCH] drm/i915: Disable semaphore inter-engine sync without timeslicing
  2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
  2020-05-21  8:53 ` [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release Chris Wilson
@ 2020-05-21  8:55 ` Chris Wilson
  2020-05-21  9:10 ` [Intel-gfx] [PATCH 1/2] " Tvrtko Ursulin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  8:55 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

Since the removal of the no-semaphore boosting, we rely on timeslicing to
reorder passed inter-dependency hogs across the engines. However, we
require preemption to support timeslicing into user payloads, and not all
machine support preemption so we do not universally enable timeslicing,
even when it would correctly preempt our own inter-engine semaphores.
Since timeslicing and semaphore priority deboosting is now disabled on
Broadwell/Braswell, we have to follow suite and not use semaphores.

Testcase: igt/gem_exec_schedule/semaphore-codependency # bdw/bsw
Fixes: 18e4af04d218 ("drm/i915: Drop no-semaphore boosting")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 900ea8b7fc8f..f5d59d18cd5b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -230,7 +230,7 @@ static void intel_context_set_gem(struct intel_context *ce,
 		ce->timeline = intel_timeline_get(ctx->timeline);
 
 	if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
-	    intel_engine_has_semaphores(ce->engine))
+	    intel_engine_has_timeslices(ce->engine))
 		__set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
 }
 
@@ -1969,7 +1969,7 @@ static int __apply_priority(struct intel_context *ce, void *arg)
 {
 	struct i915_gem_context *ctx = arg;
 
-	if (!intel_engine_has_semaphores(ce->engine))
+	if (!intel_engine_has_timeslices(ce->engine))
 		return 0;
 
 	if (ctx->sched.priority >= I915_PRIORITY_NORMAL)
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing
  2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
  2020-05-21  8:53 ` [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release Chris Wilson
  2020-05-21  8:55 ` [Intel-gfx] [PATCH] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
@ 2020-05-21  9:10 ` Tvrtko Ursulin
  2020-05-21  9:42   ` Chris Wilson
  2020-05-21 10:00 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2) Patchwork
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2020-05-21  9:10 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 21/05/2020 09:53, Chris Wilson wrote:
> Since the remove of the no-semaphore boosting, we rely on timeslicing to
> reorder past inter-dependency hogs across the engines. However, we
> require preemption to support timeslicing into user payloads, and not all
> machine support preemption so we do not universally enable timeslicing
> even when it would preempt our own inter-engine semaphores.
> 
> Testcase: igt/gem_exec_schedule/semaphore-codependency # bdw/bsw
> Fixes: 18e4af04d218 ("drm/i915: Drop no-semaphore boosting")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 900ea8b7fc8f..f5d59d18cd5b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -230,7 +230,7 @@ static void intel_context_set_gem(struct intel_context *ce,
>   		ce->timeline = intel_timeline_get(ctx->timeline);
>   
>   	if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
> -	    intel_engine_has_semaphores(ce->engine))
> +	    intel_engine_has_timeslices(ce->engine))
>   		__set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
>   }
>   
> @@ -1969,7 +1969,7 @@ static int __apply_priority(struct intel_context *ce, void *arg)
>   {
>   	struct i915_gem_context *ctx = arg;
>   
> -	if (!intel_engine_has_semaphores(ce->engine))
> +	if (!intel_engine_has_timeslices(ce->engine))
>   		return 0;
>   
>   	if (ctx->sched.priority >= I915_PRIORITY_NORMAL)
> 

__i915_request_await_execution is okay to keep using semaphores?

Regards,

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release
  2020-05-21  8:53 ` [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release Chris Wilson
@ 2020-05-21  9:13   ` Tvrtko Ursulin
  2020-05-21  9:27     ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2020-05-21  9:13 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 21/05/2020 09:53, Chris Wilson wrote:
> In order to be valid to dereference during the i915_fence_release, after
> retiring the fence and releasing its refererences, we assume that
> rq->engine can only be a real engine (that stay intact until the device
> is shutdown after all fences have been flushed). However, due to a quirk
> of preempt-to-busy, we may retire a request that still belongs to a
> virtual engine and so eventually free it with rq->engine being invalid.
> To avoid dereferencing that invalid engine, we look at the
> execution_mask which if it indicates it may be executed on more than one
> engine, we know it originated on a virtual engine and may still be on
> one.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
> Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 526c1e9acbd5..6e357183bece 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
>   	i915_sw_fence_fini(&rq->submit);
>   	i915_sw_fence_fini(&rq->semaphore);
>   
> -	/* Keep one request on each engine for reserved use under mempressure */
> -	if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
> +	/*
> +	 * Keep one request on each engine for reserved use under mempressure
> +	 *
> +	 * We do not hold a reference to the engine here and so have to be
> +	 * very careful in what rq->engine we poke. The virtual engine is
> +	 * referenced via the rq->context and we released that ref during
> +	 * i915_request_retire(), ergo we must not dereference a virtual
> +	 * engine here. Not that we would want to, as the only consumer of
> +	 * the reserved engine->request_pool is the powermanagent parking,

power management

> +	 * which must-not-fail, and that is only run on the physical engines.
> +	 *
> +	 * Since the request must have been executed to be have completed,
> +	 * we know that it will have been processed by the HW and will
> +	 * not be unsubmitted again, so rq->engine and rq->execution_mask
> +	 * at this point is stable. rq->execution_mask will be a single
> +	 * bit if the last and only engine it could execution on was a
> +	 * physical engine, if it's multiple bits then it started on and
> +	 * could still be on a virtual engine. Thus if the mask is not a
> +	 * power-of-two we assume that rq->engine may still be a virtual
> +	 * engien and so a dangling invalid pointer that we cannot 

engine

But.. submit fence can mask out execution_mask bits and make it appear 
the request was on a physical engine. What then?

Regards,

Tvrtko

dereference
> +	 */
> +	if (is_power_of_2(rq->execution_mask) &&
> +	    !cmpxchg(&rq->engine->request_pool, NULL, rq))
>   		return;
>   
>   	kmem_cache_free(global.slab_requests, rq);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release
  2020-05-21  9:13   ` Tvrtko Ursulin
@ 2020-05-21  9:27     ` Chris Wilson
  2020-05-21  9:32       ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  9:27 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2020-05-21 10:13:14)
> 
> On 21/05/2020 09:53, Chris Wilson wrote:
> > In order to be valid to dereference during the i915_fence_release, after
> > retiring the fence and releasing its refererences, we assume that
> > rq->engine can only be a real engine (that stay intact until the device
> > is shutdown after all fences have been flushed). However, due to a quirk
> > of preempt-to-busy, we may retire a request that still belongs to a
> > virtual engine and so eventually free it with rq->engine being invalid.
> > To avoid dereferencing that invalid engine, we look at the
> > execution_mask which if it indicates it may be executed on more than one
> > engine, we know it originated on a virtual engine and may still be on
> > one.
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
> > Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
> >   1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 526c1e9acbd5..6e357183bece 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
> >       i915_sw_fence_fini(&rq->submit);
> >       i915_sw_fence_fini(&rq->semaphore);
> >   
> > -     /* Keep one request on each engine for reserved use under mempressure */
> > -     if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
> > +     /*
> > +      * Keep one request on each engine for reserved use under mempressure
> > +      *
> > +      * We do not hold a reference to the engine here and so have to be
> > +      * very careful in what rq->engine we poke. The virtual engine is
> > +      * referenced via the rq->context and we released that ref during
> > +      * i915_request_retire(), ergo we must not dereference a virtual
> > +      * engine here. Not that we would want to, as the only consumer of
> > +      * the reserved engine->request_pool is the powermanagent parking,
> 
> power management
> 
> > +      * which must-not-fail, and that is only run on the physical engines.
> > +      *
> > +      * Since the request must have been executed to be have completed,
> > +      * we know that it will have been processed by the HW and will
> > +      * not be unsubmitted again, so rq->engine and rq->execution_mask
> > +      * at this point is stable. rq->execution_mask will be a single
> > +      * bit if the last and only engine it could execution on was a
> > +      * physical engine, if it's multiple bits then it started on and
> > +      * could still be on a virtual engine. Thus if the mask is not a
> > +      * power-of-two we assume that rq->engine may still be a virtual
> > +      * engien and so a dangling invalid pointer that we cannot 
> 
> engine
> 
> But.. submit fence can mask out execution_mask bits and make it appear 
> the request was on a physical engine. What then?

Then we execute along a single engine and it is never returned to the
virtual engine (in __unwind_incomplete_requests). 

+      * at this point is stable. rq->execution_mask will be a single
+      * bit if the last and only engine it could execution on was a
+      * physical engine, if it's multiple bits then it started on and
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release
  2020-05-21  9:27     ` Chris Wilson
@ 2020-05-21  9:32       ` Chris Wilson
  2020-05-21  9:44         ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  9:32 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Chris Wilson (2020-05-21 10:27:16)
> Quoting Tvrtko Ursulin (2020-05-21 10:13:14)
> > 
> > On 21/05/2020 09:53, Chris Wilson wrote:
> > > In order to be valid to dereference during the i915_fence_release, after
> > > retiring the fence and releasing its refererences, we assume that
> > > rq->engine can only be a real engine (that stay intact until the device
> > > is shutdown after all fences have been flushed). However, due to a quirk
> > > of preempt-to-busy, we may retire a request that still belongs to a
> > > virtual engine and so eventually free it with rq->engine being invalid.
> > > To avoid dereferencing that invalid engine, we look at the
> > > execution_mask which if it indicates it may be executed on more than one
> > > engine, we know it originated on a virtual engine and may still be on
> > > one.
> > > 
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
> > > Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
> > >   1 file changed, 23 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > > index 526c1e9acbd5..6e357183bece 100644
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
> > >       i915_sw_fence_fini(&rq->submit);
> > >       i915_sw_fence_fini(&rq->semaphore);
> > >   
> > > -     /* Keep one request on each engine for reserved use under mempressure */
> > > -     if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
> > > +     /*
> > > +      * Keep one request on each engine for reserved use under mempressure
> > > +      *
> > > +      * We do not hold a reference to the engine here and so have to be
> > > +      * very careful in what rq->engine we poke. The virtual engine is
> > > +      * referenced via the rq->context and we released that ref during
> > > +      * i915_request_retire(), ergo we must not dereference a virtual
> > > +      * engine here. Not that we would want to, as the only consumer of
> > > +      * the reserved engine->request_pool is the powermanagent parking,
> > 
> > power management
> > 
> > > +      * which must-not-fail, and that is only run on the physical engines.
> > > +      *
> > > +      * Since the request must have been executed to be have completed,
> > > +      * we know that it will have been processed by the HW and will
> > > +      * not be unsubmitted again, so rq->engine and rq->execution_mask
> > > +      * at this point is stable. rq->execution_mask will be a single
> > > +      * bit if the last and only engine it could execution on was a
> > > +      * physical engine, if it's multiple bits then it started on and
> > > +      * could still be on a virtual engine. Thus if the mask is not a
> > > +      * power-of-two we assume that rq->engine may still be a virtual
> > > +      * engien and so a dangling invalid pointer that we cannot 
> > 
> > engine
> > 
> > But.. submit fence can mask out execution_mask bits and make it appear 
> > the request was on a physical engine. What then?
> 
> Then we execute along a single engine and it is never returned to the
> virtual engine (in __unwind_incomplete_requests). 

         * For example, consider the flow of a bonded request through a virtual
         * engine. The request is created with a wide engine mask (all engines
         * that we might execute on). On processing the bond, the request mask
         * is reduced to one or more engines. If the request is subsequently
         * bound to a single engine, it will then be constrained to only
         * execute on that engine and never returned to the virtual engine
         * after timeslicing away, see __unwind_incomplete_requests(). Thus we
         * know that if the rq->execution_mask is a single bit, only rq->engine
         * can be the exact corresponding engine->mask.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing
  2020-05-21  9:10 ` [Intel-gfx] [PATCH 1/2] " Tvrtko Ursulin
@ 2020-05-21  9:42   ` Chris Wilson
  2020-05-21 10:17     ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  9:42 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2020-05-21 10:10:10)
> 
> On 21/05/2020 09:53, Chris Wilson wrote:
> > Since the remove of the no-semaphore boosting, we rely on timeslicing to
> > reorder past inter-dependency hogs across the engines. However, we
> > require preemption to support timeslicing into user payloads, and not all
> > machine support preemption so we do not universally enable timeslicing
> > even when it would preempt our own inter-engine semaphores.
> > 
> > Testcase: igt/gem_exec_schedule/semaphore-codependency # bdw/bsw
> > Fixes: 18e4af04d218 ("drm/i915: Drop no-semaphore boosting")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 900ea8b7fc8f..f5d59d18cd5b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -230,7 +230,7 @@ static void intel_context_set_gem(struct intel_context *ce,
> >               ce->timeline = intel_timeline_get(ctx->timeline);
> >   
> >       if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
> > -         intel_engine_has_semaphores(ce->engine))
> > +         intel_engine_has_timeslices(ce->engine))
> >               __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
> >   }
> >   
> > @@ -1969,7 +1969,7 @@ static int __apply_priority(struct intel_context *ce, void *arg)
> >   {
> >       struct i915_gem_context *ctx = arg;
> >   
> > -     if (!intel_engine_has_semaphores(ce->engine))
> > +     if (!intel_engine_has_timeslices(ce->engine))
> >               return 0;
> >   
> >       if (ctx->sched.priority >= I915_PRIORITY_NORMAL)
> > 
> 
> __i915_request_await_execution is okay to keep using semaphores?

I think so. Using semaphores there still benefits from synchronising
with a master in ELSP[1]. The danger is that it does increase the
hangcheck possibility for the bond request, such that a slow request
before the master would result in us declaring the bond hung. The
question is whether that is worse than executing the bond before the
master.

I should be able to write a test to demonstrate the hang in the bond.
For example, if we do something like:

on master engine:
	submit spin
	submit master -> submit fence -> submit bond
	for(;;)
		submit high priority spin
		terminate previous spin
	
Hmm. But without preemption... master will execute before we get a
chance to submit the high priority spinners. So this will not actually
hang.

Ok, so this is safer than it seems :)

Just need to write that test and execute it on broadwell to verify my
claim.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release
  2020-05-21  9:32       ` Chris Wilson
@ 2020-05-21  9:44         ` Chris Wilson
  2020-05-21 13:57           ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2020-05-21  9:44 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Chris Wilson (2020-05-21 10:32:49)
> Quoting Chris Wilson (2020-05-21 10:27:16)
> > Quoting Tvrtko Ursulin (2020-05-21 10:13:14)
> > > 
> > > On 21/05/2020 09:53, Chris Wilson wrote:
> > > > In order to be valid to dereference during the i915_fence_release, after
> > > > retiring the fence and releasing its refererences, we assume that
> > > > rq->engine can only be a real engine (that stay intact until the device
> > > > is shutdown after all fences have been flushed). However, due to a quirk
> > > > of preempt-to-busy, we may retire a request that still belongs to a
> > > > virtual engine and so eventually free it with rq->engine being invalid.
> > > > To avoid dereferencing that invalid engine, we look at the
> > > > execution_mask which if it indicates it may be executed on more than one
> > > > engine, we know it originated on a virtual engine and may still be on
> > > > one.
> > > > 
> > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
> > > > Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
> > > >   1 file changed, 23 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > > > index 526c1e9acbd5..6e357183bece 100644
> > > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > > @@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
> > > >       i915_sw_fence_fini(&rq->submit);
> > > >       i915_sw_fence_fini(&rq->semaphore);
> > > >   
> > > > -     /* Keep one request on each engine for reserved use under mempressure */
> > > > -     if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
> > > > +     /*
> > > > +      * Keep one request on each engine for reserved use under mempressure
> > > > +      *
> > > > +      * We do not hold a reference to the engine here and so have to be
> > > > +      * very careful in what rq->engine we poke. The virtual engine is
> > > > +      * referenced via the rq->context and we released that ref during
> > > > +      * i915_request_retire(), ergo we must not dereference a virtual
> > > > +      * engine here. Not that we would want to, as the only consumer of
> > > > +      * the reserved engine->request_pool is the powermanagent parking,
> > > 
> > > power management
> > > 
> > > > +      * which must-not-fail, and that is only run on the physical engines.
> > > > +      *
> > > > +      * Since the request must have been executed to be have completed,
> > > > +      * we know that it will have been processed by the HW and will
> > > > +      * not be unsubmitted again, so rq->engine and rq->execution_mask
> > > > +      * at this point is stable. rq->execution_mask will be a single
> > > > +      * bit if the last and only engine it could execution on was a
> > > > +      * physical engine, if it's multiple bits then it started on and
> > > > +      * could still be on a virtual engine. Thus if the mask is not a
> > > > +      * power-of-two we assume that rq->engine may still be a virtual
> > > > +      * engien and so a dangling invalid pointer that we cannot 
> > > 
> > > engine
> > > 
> > > But.. submit fence can mask out execution_mask bits and make it appear 
> > > the request was on a physical engine. What then?
> > 
> > Then we execute along a single engine and it is never returned to the
> > virtual engine (in __unwind_incomplete_requests). 
> 
>          * For example, consider the flow of a bonded request through a virtual
>          * engine. The request is created with a wide engine mask (all engines
>          * that we might execute on). On processing the bond, the request mask
>          * is reduced to one or more engines. If the request is subsequently
>          * bound to a single engine, it will then be constrained to only
>          * execute on that engine and never returned to the virtual engine
>          * after timeslicing away, see __unwind_incomplete_requests(). Thus we
>          * know that if the rq->execution_mask is a single bit, only rq->engine

rq->engine can only be a physical engine, with the exact corresponding mask.

>          * can be the exact corresponding engine->mask.
> 
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2)
  2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
                   ` (2 preceding siblings ...)
  2020-05-21  9:10 ` [Intel-gfx] [PATCH 1/2] " Tvrtko Ursulin
@ 2020-05-21 10:00 ` Patchwork
  2020-05-21 10:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
  2020-05-22  2:28 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
  5 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2020-05-21 10:00 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2)
URL   : https://patchwork.freedesktop.org/series/77503/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.
-
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2274:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2275:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2276:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2277:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2278:17: error: bad integer constant expression
+drivers/gpu/drm/i915/gem/i915_gem_context.c:2279:17: error: bad integer constant expression

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing
  2020-05-21  9:42   ` Chris Wilson
@ 2020-05-21 10:17     ` Chris Wilson
  2020-05-21 13:58       ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2020-05-21 10:17 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Chris Wilson (2020-05-21 10:42:26)
> Quoting Tvrtko Ursulin (2020-05-21 10:10:10)
> > 
> > On 21/05/2020 09:53, Chris Wilson wrote:
> > > Since the remove of the no-semaphore boosting, we rely on timeslicing to
> > > reorder past inter-dependency hogs across the engines. However, we
> > > require preemption to support timeslicing into user payloads, and not all
> > > machine support preemption so we do not universally enable timeslicing
> > > even when it would preempt our own inter-engine semaphores.
> > > 
> > > Testcase: igt/gem_exec_schedule/semaphore-codependency # bdw/bsw
> > > Fixes: 18e4af04d218 ("drm/i915: Drop no-semaphore boosting")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > index 900ea8b7fc8f..f5d59d18cd5b 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > @@ -230,7 +230,7 @@ static void intel_context_set_gem(struct intel_context *ce,
> > >               ce->timeline = intel_timeline_get(ctx->timeline);
> > >   
> > >       if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
> > > -         intel_engine_has_semaphores(ce->engine))
> > > +         intel_engine_has_timeslices(ce->engine))
> > >               __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
> > >   }
> > >   
> > > @@ -1969,7 +1969,7 @@ static int __apply_priority(struct intel_context *ce, void *arg)
> > >   {
> > >       struct i915_gem_context *ctx = arg;
> > >   
> > > -     if (!intel_engine_has_semaphores(ce->engine))
> > > +     if (!intel_engine_has_timeslices(ce->engine))
> > >               return 0;
> > >   
> > >       if (ctx->sched.priority >= I915_PRIORITY_NORMAL)
> > > 
> > 
> > __i915_request_await_execution is okay to keep using semaphores?
> 
> I think so. Using semaphores there still benefits from synchronising
> with a master in ELSP[1]. The danger is that it does increase the
> hangcheck possibility for the bond request, such that a slow request
> before the master would result in us declaring the bond hung. The
> question is whether that is worse than executing the bond before the
> master.
> 
> I should be able to write a test to demonstrate the hang in the bond.
> For example, if we do something like:
> 
> on master engine:
>         submit spin
>         submit master -> submit fence -> submit bond
>         for(;;)
>                 submit high priority spin
>                 terminate previous spin
>         
> Hmm. But without preemption... master will execute before we get a
> chance to submit the high priority spinners. So this will not actually
> hang.
> 
> Ok, so this is safer than it seems :)

Even more so, since we do preempt the semaphore for the hangcheck.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2)
  2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
                   ` (3 preceding siblings ...)
  2020-05-21 10:00 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2) Patchwork
@ 2020-05-21 10:21 ` Patchwork
  2020-05-22  2:28 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
  5 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2020-05-21 10:21 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2)
URL   : https://patchwork.freedesktop.org/series/77503/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8517 -> Patchwork_17746
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Warnings ####

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-x1275:       [FAIL][1] ([i915#62]) -> [FAIL][2] ([i915#62] / [i915#95])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html

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


Participating hosts (46 -> 42)
------------------------------

  Additional (1): fi-kbl-7560u 
  Missing    (5): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_8517 -> Patchwork_17746

  CI-20190529: 20190529
  CI_DRM_8517: 7c5c05e694bf83e9d4ef64172ef6c9d55aa334a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5666: dfa3b1fdc9813a48314a43faaacb7dacc06112d6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17746: 4e08f95c12826459d82860a64ead6e39d4f04418 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4e08f95c1282 drm/i915: Avoid using rq->engine after free during i915_fence_release
17642944c3fe drm/i915: Disable semaphore inter-engine sync without timeslicing

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release
  2020-05-21  9:44         ` Chris Wilson
@ 2020-05-21 13:57           ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2020-05-21 13:57 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 21/05/2020 10:44, Chris Wilson wrote:
> Quoting Chris Wilson (2020-05-21 10:32:49)
>> Quoting Chris Wilson (2020-05-21 10:27:16)
>>> Quoting Tvrtko Ursulin (2020-05-21 10:13:14)
>>>>
>>>> On 21/05/2020 09:53, Chris Wilson wrote:
>>>>> In order to be valid to dereference during the i915_fence_release, after
>>>>> retiring the fence and releasing its refererences, we assume that
>>>>> rq->engine can only be a real engine (that stay intact until the device
>>>>> is shutdown after all fences have been flushed). However, due to a quirk
>>>>> of preempt-to-busy, we may retire a request that still belongs to a
>>>>> virtual engine and so eventually free it with rq->engine being invalid.
>>>>> To avoid dereferencing that invalid engine, we look at the
>>>>> execution_mask which if it indicates it may be executed on more than one
>>>>> engine, we know it originated on a virtual engine and may still be on
>>>>> one.
>>>>>
>>>>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
>>>>> Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
>>>>>    1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>>>> index 526c1e9acbd5..6e357183bece 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>>>> @@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
>>>>>        i915_sw_fence_fini(&rq->submit);
>>>>>        i915_sw_fence_fini(&rq->semaphore);
>>>>>    
>>>>> -     /* Keep one request on each engine for reserved use under mempressure */
>>>>> -     if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
>>>>> +     /*
>>>>> +      * Keep one request on each engine for reserved use under mempressure
>>>>> +      *
>>>>> +      * We do not hold a reference to the engine here and so have to be
>>>>> +      * very careful in what rq->engine we poke. The virtual engine is
>>>>> +      * referenced via the rq->context and we released that ref during
>>>>> +      * i915_request_retire(), ergo we must not dereference a virtual
>>>>> +      * engine here. Not that we would want to, as the only consumer of
>>>>> +      * the reserved engine->request_pool is the powermanagent parking,
>>>>
>>>> power management
>>>>
>>>>> +      * which must-not-fail, and that is only run on the physical engines.
>>>>> +      *
>>>>> +      * Since the request must have been executed to be have completed,
>>>>> +      * we know that it will have been processed by the HW and will
>>>>> +      * not be unsubmitted again, so rq->engine and rq->execution_mask
>>>>> +      * at this point is stable. rq->execution_mask will be a single
>>>>> +      * bit if the last and only engine it could execution on was a
>>>>> +      * physical engine, if it's multiple bits then it started on and
>>>>> +      * could still be on a virtual engine. Thus if the mask is not a
>>>>> +      * power-of-two we assume that rq->engine may still be a virtual
>>>>> +      * engien and so a dangling invalid pointer that we cannot
>>>>
>>>> engine
>>>>
>>>> But.. submit fence can mask out execution_mask bits and make it appear
>>>> the request was on a physical engine. What then?
>>>
>>> Then we execute along a single engine and it is never returned to the
>>> virtual engine (in __unwind_incomplete_requests).
>>
>>           * For example, consider the flow of a bonded request through a virtual
>>           * engine. The request is created with a wide engine mask (all engines
>>           * that we might execute on). On processing the bond, the request mask
>>           * is reduced to one or more engines. If the request is subsequently
>>           * bound to a single engine, it will then be constrained to only
>>           * execute on that engine and never returned to the virtual engine
>>           * after timeslicing away, see __unwind_incomplete_requests(). Thus we
>>           * know that if the rq->execution_mask is a single bit, only rq->engine
> 
> rq->engine can only be a physical engine, with the exact corresponding mask.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 
>>           * can be the exact corresponding engine->mask.
>>
>> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing
  2020-05-21 10:17     ` Chris Wilson
@ 2020-05-21 13:58       ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2020-05-21 13:58 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 21/05/2020 11:17, Chris Wilson wrote:
> Quoting Chris Wilson (2020-05-21 10:42:26)
>> Quoting Tvrtko Ursulin (2020-05-21 10:10:10)
>>>
>>> On 21/05/2020 09:53, Chris Wilson wrote:
>>>> Since the remove of the no-semaphore boosting, we rely on timeslicing to
>>>> reorder past inter-dependency hogs across the engines. However, we
>>>> require preemption to support timeslicing into user payloads, and not all
>>>> machine support preemption so we do not universally enable timeslicing
>>>> even when it would preempt our own inter-engine semaphores.
>>>>
>>>> Testcase: igt/gem_exec_schedule/semaphore-codependency # bdw/bsw
>>>> Fixes: 18e4af04d218 ("drm/i915: Drop no-semaphore boosting")
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> index 900ea8b7fc8f..f5d59d18cd5b 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>> @@ -230,7 +230,7 @@ static void intel_context_set_gem(struct intel_context *ce,
>>>>                ce->timeline = intel_timeline_get(ctx->timeline);
>>>>    
>>>>        if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
>>>> -         intel_engine_has_semaphores(ce->engine))
>>>> +         intel_engine_has_timeslices(ce->engine))
>>>>                __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
>>>>    }
>>>>    
>>>> @@ -1969,7 +1969,7 @@ static int __apply_priority(struct intel_context *ce, void *arg)
>>>>    {
>>>>        struct i915_gem_context *ctx = arg;
>>>>    
>>>> -     if (!intel_engine_has_semaphores(ce->engine))
>>>> +     if (!intel_engine_has_timeslices(ce->engine))
>>>>                return 0;
>>>>    
>>>>        if (ctx->sched.priority >= I915_PRIORITY_NORMAL)
>>>>
>>>
>>> __i915_request_await_execution is okay to keep using semaphores?
>>
>> I think so. Using semaphores there still benefits from synchronising
>> with a master in ELSP[1]. The danger is that it does increase the
>> hangcheck possibility for the bond request, such that a slow request
>> before the master would result in us declaring the bond hung. The
>> question is whether that is worse than executing the bond before the
>> master.
>>
>> I should be able to write a test to demonstrate the hang in the bond.
>> For example, if we do something like:
>>
>> on master engine:
>>          submit spin
>>          submit master -> submit fence -> submit bond
>>          for(;;)
>>                  submit high priority spin
>>                  terminate previous spin
>>          
>> Hmm. But without preemption... master will execute before we get a
>> chance to submit the high priority spinners. So this will not actually
>> hang.
>>
>> Ok, so this is safer than it seems :)
> 
> Even more so, since we do preempt the semaphore for the hangcheck.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2)
  2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
                   ` (4 preceding siblings ...)
  2020-05-21 10:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2020-05-22  2:28 ` Patchwork
  5 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2020-05-22  2:28 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2)
URL   : https://patchwork.freedesktop.org/series/77503/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8517_full -> Patchwork_17746_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-suspend:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2] ([i915#69])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-skl6/igt@gem_eio@in-flight-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-skl5/igt@gem_eio@in-flight-suspend.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([i915#1436] / [i915#716])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl8/igt@gen9_exec_parse@allowed-all.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl4/igt@gen9_exec_parse@allowed-all.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-apl:          [PASS][5] -> [DMESG-WARN][6] ([i915#180]) +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl8/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109349])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-iclb6/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled:
    - shard-kbl:          [PASS][9] -> [FAIL][10] ([i915#177] / [i915#52] / [i915#54] / [i915#93] / [i915#95])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-kbl4/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-kbl7/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled.html

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([i915#53] / [i915#93] / [i915#95])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-kbl2/igt@kms_pipe_crc_basic@hang-read-crc-pipe-a.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-kbl1/igt@kms_pipe_crc_basic@hang-read-crc-pipe-a.html
    - shard-apl:          [PASS][13] -> [FAIL][14] ([i915#53] / [i915#95])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl1/igt@kms_pipe_crc_basic@hang-read-crc-pipe-a.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl1/igt@kms_pipe_crc_basic@hang-read-crc-pipe-a.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#108145] / [i915#265]) +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#109642] / [fdo#111068])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-iclb6/igt@kms_psr2_su@frontbuffer.html

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

  * igt@kms_setmode@basic:
    - shard-skl:          [PASS][21] -> [FAIL][22] ([i915#31])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-skl8/igt@kms_setmode@basic.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-skl7/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [DMESG-WARN][23] ([i915#180]) -> [PASS][24] +5 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl1/igt@gem_softpin@noreloc-s3.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl7/igt@gem_softpin@noreloc-s3.html

  * igt@kms_cursor_crc@pipe-a-cursor-64x21-random:
    - shard-skl:          [FAIL][25] ([i915#54]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-skl2/igt@kms_cursor_crc@pipe-a-cursor-64x21-random.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-skl4/igt@kms_cursor_crc@pipe-a-cursor-64x21-random.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic:
    - shard-skl:          [FAIL][27] ([IGT#5]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-skl2/igt@kms_cursor_legacy@flip-vs-cursor-atomic.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-skl4/igt@kms_cursor_legacy@flip-vs-cursor-atomic.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          [DMESG-WARN][29] ([i915#180]) -> [PASS][30] +5 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-kbl2/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-kbl1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [FAIL][31] ([i915#173]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-iclb1/igt@kms_psr@no_drrs.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-iclb6/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         [SKIP][33] ([fdo#109441]) -> [PASS][34] +2 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-iclb6/igt@kms_psr@psr2_cursor_mmap_cpu.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html

  
#### Warnings ####

  * igt@kms_content_protection@legacy:
    - shard-apl:          [TIMEOUT][35] ([i915#1319]) -> [FAIL][36] ([fdo#110321] / [fdo#110336])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl7/igt@kms_content_protection@legacy.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl8/igt@kms_content_protection@legacy.html

  * igt@kms_content_protection@lic:
    - shard-apl:          [TIMEOUT][37] ([i915#1319]) -> [FAIL][38] ([fdo#110321])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl8/igt@kms_content_protection@lic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl4/igt@kms_content_protection@lic.html

  * igt@kms_content_protection@srm:
    - shard-kbl:          [TIMEOUT][39] ([i915#1319]) -> [FAIL][40] ([fdo#110321] / [i915#93] / [i915#95])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-kbl6/igt@kms_content_protection@srm.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-kbl2/igt@kms_content_protection@srm.html
    - shard-apl:          [TIMEOUT][41] ([i915#1319]) -> [FAIL][42] ([fdo#110321] / [i915#95])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8517/shard-apl4/igt@kms_content_protection@srm.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17746/shard-apl7/igt@kms_content_protection@srm.html

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

  [IGT#5]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/5
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#110336]: https://bugs.freedesktop.org/show_bug.cgi?id=110336
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#1319]: https://gitlab.freedesktop.org/drm/intel/issues/1319
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#173]: https://gitlab.freedesktop.org/drm/intel/issues/173
  [i915#177]: https://gitlab.freedesktop.org/drm/intel/issues/177
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
  [i915#53]: https://gitlab.freedesktop.org/drm/intel/issues/53
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


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

  No changes in participating hosts


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

  * Linux: CI_DRM_8517 -> Patchwork_17746

  CI-20190529: 20190529
  CI_DRM_8517: 7c5c05e694bf83e9d4ef64172ef6c9d55aa334a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5666: dfa3b1fdc9813a48314a43faaacb7dacc06112d6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17746: 4e08f95c12826459d82860a64ead6e39d4f04418 @ 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_17746/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-05-22  2:28 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21  8:53 [Intel-gfx] [PATCH 1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
2020-05-21  8:53 ` [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release Chris Wilson
2020-05-21  9:13   ` Tvrtko Ursulin
2020-05-21  9:27     ` Chris Wilson
2020-05-21  9:32       ` Chris Wilson
2020-05-21  9:44         ` Chris Wilson
2020-05-21 13:57           ` Tvrtko Ursulin
2020-05-21  8:55 ` [Intel-gfx] [PATCH] drm/i915: Disable semaphore inter-engine sync without timeslicing Chris Wilson
2020-05-21  9:10 ` [Intel-gfx] [PATCH 1/2] " Tvrtko Ursulin
2020-05-21  9:42   ` Chris Wilson
2020-05-21 10:17     ` Chris Wilson
2020-05-21 13:58       ` Tvrtko Ursulin
2020-05-21 10:00 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with drm/i915: Disable semaphore inter-engine sync without timeslicing (rev2) Patchwork
2020-05-21 10:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-05-22  2:28 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).