| Message ID | 20200521085320.906-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Disable semaphore inter-engine sync without timeslicing | expand |
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
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
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
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
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)
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(-)