| Message ID | 20210506191451.77768-54-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Basic GuC submission support in the i915 | expand |
On 06/05/2021 20:14, Matthew Brost wrote: > Disable semaphores when using GuC scheduling as semaphores are broken in > the current GuC firmware. What is "current"? Given that the patch itself is like year and a half old. Regards, Tvrtko > Cc: John Harrison <john.c.harrison@intel.com> > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 ++++-- > 1 file changed, 4 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 993faa213b41..d30260ffe2a7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -230,7 +230,8 @@ 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_timeslices(ce->engine)) > + intel_engine_has_timeslices(ce->engine) && > + intel_engine_has_semaphores(ce->engine)) > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us); > @@ -1939,7 +1940,8 @@ static int __apply_priority(struct intel_context *ce, void *arg) > if (!intel_engine_has_timeslices(ce->engine)) > return 0; > > - if (ctx->sched.priority >= I915_PRIORITY_NORMAL) > + if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > + intel_engine_has_semaphores(ce->engine)) > intel_context_set_use_semaphores(ce); > else > intel_context_clear_use_semaphores(ce); >
On Tue, May 25, 2021 at 10:52:01AM +0100, Tvrtko Ursulin wrote: > > On 06/05/2021 20:14, Matthew Brost wrote: > > Disable semaphores when using GuC scheduling as semaphores are broken in > > the current GuC firmware. > > What is "current"? Given that the patch itself is like year and a half old. > Stale comment. Semaphore work with the firmware we just haven't enabled them in the i915 with GuC submission as this an optimization and not required for functionality. Matt > Regards, > > Tvrtko > > > Cc: John Harrison <john.c.harrison@intel.com> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 ++++-- > > 1 file changed, 4 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 993faa213b41..d30260ffe2a7 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -230,7 +230,8 @@ 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_timeslices(ce->engine)) > > + intel_engine_has_timeslices(ce->engine) && > > + intel_engine_has_semaphores(ce->engine)) > > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us); > > @@ -1939,7 +1940,8 @@ static int __apply_priority(struct intel_context *ce, void *arg) > > if (!intel_engine_has_timeslices(ce->engine)) > > return 0; > > - if (ctx->sched.priority >= I915_PRIORITY_NORMAL) > > + if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > > + intel_engine_has_semaphores(ce->engine)) > > intel_context_set_use_semaphores(ce); > > else > > intel_context_clear_use_semaphores(ce); > >
On 25/05/2021 18:01, Matthew Brost wrote: > On Tue, May 25, 2021 at 10:52:01AM +0100, Tvrtko Ursulin wrote: >> >> On 06/05/2021 20:14, Matthew Brost wrote: >>> Disable semaphores when using GuC scheduling as semaphores are broken in >>> the current GuC firmware. >> >> What is "current"? Given that the patch itself is like year and a half old. >> > > Stale comment. Semaphore work with the firmware we just haven't enabled > them in the i915 with GuC submission as this an optimization and not > required for functionality. How will the updated commit message look in terms of remaining reasons why semaphores won't/can't be enabled? They were a nice performance win on some media workloads although granted a lot of tweaking was required to find a good balance on when to use them and when not to. Regards, Tvrtko > Matt > >> Regards, >> >> Tvrtko >> >>> Cc: John Harrison <john.c.harrison@intel.com> >>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 ++++-- >>> 1 file changed, 4 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 993faa213b41..d30260ffe2a7 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c >>> @@ -230,7 +230,8 @@ 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_timeslices(ce->engine)) >>> + intel_engine_has_timeslices(ce->engine) && >>> + intel_engine_has_semaphores(ce->engine)) >>> __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); >>> intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us); >>> @@ -1939,7 +1940,8 @@ static int __apply_priority(struct intel_context *ce, void *arg) >>> if (!intel_engine_has_timeslices(ce->engine)) >>> return 0; >>> - if (ctx->sched.priority >= I915_PRIORITY_NORMAL) >>> + if (ctx->sched.priority >= I915_PRIORITY_NORMAL && >>> + intel_engine_has_semaphores(ce->engine)) >>> intel_context_set_use_semaphores(ce); >>> else >>> intel_context_clear_use_semaphores(ce); >>>
On Wed, May 26, 2021 at 10:25:13AM +0100, Tvrtko Ursulin wrote: > > On 25/05/2021 18:01, Matthew Brost wrote: > > On Tue, May 25, 2021 at 10:52:01AM +0100, Tvrtko Ursulin wrote: > > > > > > On 06/05/2021 20:14, Matthew Brost wrote: > > > > Disable semaphores when using GuC scheduling as semaphores are broken in > > > > the current GuC firmware. > > > > > > What is "current"? Given that the patch itself is like year and a half old. > > > > > > > Stale comment. Semaphore work with the firmware we just haven't enabled > > them in the i915 with GuC submission as this an optimization and not > > required for functionality. > > How will the updated commit message look in terms of remaining reasons why > semaphores won't/can't be enabled? > Semaphores are an optimization and not required for basic GuC submission to work properly. Disable until we have time to do the implementation to enable semaphores and tune them for performance. > They were a nice performance win on some media workloads although granted a > lot of tweaking was required to find a good balance on when to use them and > when not to. > The same tweaking would have to be done for with GuC submission. Let's get basic submission then tweak for performance. Matt > Regards, > > Tvrtko > > > Matt > > > > > Regards, > > > > > > Tvrtko > > > > > > > Cc: John Harrison <john.c.harrison@intel.com> > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 ++++-- > > > > 1 file changed, 4 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 993faa213b41..d30260ffe2a7 100644 > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > > @@ -230,7 +230,8 @@ 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_timeslices(ce->engine)) > > > > + intel_engine_has_timeslices(ce->engine) && > > > > + intel_engine_has_semaphores(ce->engine)) > > > > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > > > intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us); > > > > @@ -1939,7 +1940,8 @@ static int __apply_priority(struct intel_context *ce, void *arg) > > > > if (!intel_engine_has_timeslices(ce->engine)) > > > > return 0; > > > > - if (ctx->sched.priority >= I915_PRIORITY_NORMAL) > > > > + if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > > > > + intel_engine_has_semaphores(ce->engine)) > > > > intel_context_set_use_semaphores(ce); > > > > else > > > > intel_context_clear_use_semaphores(ce); > > > >
On 26/05/2021 19:15, Matthew Brost wrote: > On Wed, May 26, 2021 at 10:25:13AM +0100, Tvrtko Ursulin wrote: >> >> On 25/05/2021 18:01, Matthew Brost wrote: >>> On Tue, May 25, 2021 at 10:52:01AM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 06/05/2021 20:14, Matthew Brost wrote: >>>>> Disable semaphores when using GuC scheduling as semaphores are broken in >>>>> the current GuC firmware. >>>> >>>> What is "current"? Given that the patch itself is like year and a half old. >>>> >>> >>> Stale comment. Semaphore work with the firmware we just haven't enabled >>> them in the i915 with GuC submission as this an optimization and not >>> required for functionality. >> >> How will the updated commit message look in terms of remaining reasons why >> semaphores won't/can't be enabled? >> > > Semaphores are an optimization and not required for basic GuC submission > to work properly. Disable until we have time to do the implementation to > enable semaphores and tune them for performance. > >> They were a nice performance win on some media workloads although granted a >> lot of tweaking was required to find a good balance on when to use them and >> when not to. >> > > The same tweaking would have to be done for with GuC submission. Let's > get basic submission then tweak for performance. I don't have fundamental complaints as longs as commit message is improved and it is understood the absence of semaphores is extremely likely to regress transcode performance tests. Latter probably doesn't matter (for some definition of it) unless either there will be a platform where both GuC and execlists can be supported, or there will be two separate platforms similar in hw performance in theory, both relevant to transcode customers, one using execlists and one using GuC. Regards, Tvrtko
On Thu, May 27, 2021 at 09:41:46AM +0100, Tvrtko Ursulin wrote: > > On 26/05/2021 19:15, Matthew Brost wrote: > > On Wed, May 26, 2021 at 10:25:13AM +0100, Tvrtko Ursulin wrote: > > > > > > On 25/05/2021 18:01, Matthew Brost wrote: > > > > On Tue, May 25, 2021 at 10:52:01AM +0100, Tvrtko Ursulin wrote: > > > > > > > > > > On 06/05/2021 20:14, Matthew Brost wrote: > > > > > > Disable semaphores when using GuC scheduling as semaphores are broken in > > > > > > the current GuC firmware. > > > > > > > > > > What is "current"? Given that the patch itself is like year and a half old. > > > > > > > > > > > > > Stale comment. Semaphore work with the firmware we just haven't enabled > > > > them in the i915 with GuC submission as this an optimization and not > > > > required for functionality. > > > > > > How will the updated commit message look in terms of remaining reasons why > > > semaphores won't/can't be enabled? > > > > > > > Semaphores are an optimization and not required for basic GuC submission > > to work properly. Disable until we have time to do the implementation to > > enable semaphores and tune them for performance. > > > > > They were a nice performance win on some media workloads although granted a > > > lot of tweaking was required to find a good balance on when to use them and > > > when not to. > > > > > > > The same tweaking would have to be done for with GuC submission. Let's > > get basic submission then tweak for performance. > > I don't have fundamental complaints as longs as commit message is improved > and it is understood the absence of semaphores is extremely likely to > regress transcode performance tests. Latter probably doesn't matter (for > some definition of it) unless either there will be a platform where both GuC > and execlists can be supported, or there will be two separate platforms > similar in hw performance in theory, both relevant to transcode customers, > one using execlists and one using GuC. > Sounds good. Already have this commit message updated in my branch and will be included in the next post. Matt > 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 993faa213b41..d30260ffe2a7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -230,7 +230,8 @@ 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_timeslices(ce->engine)) + intel_engine_has_timeslices(ce->engine) && + intel_engine_has_semaphores(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us); @@ -1939,7 +1940,8 @@ static int __apply_priority(struct intel_context *ce, void *arg) if (!intel_engine_has_timeslices(ce->engine)) return 0; - if (ctx->sched.priority >= I915_PRIORITY_NORMAL) + if (ctx->sched.priority >= I915_PRIORITY_NORMAL && + intel_engine_has_semaphores(ce->engine)) intel_context_set_use_semaphores(ce); else intel_context_clear_use_semaphores(ce);
Disable semaphores when using GuC scheduling as semaphores are broken in the current GuC firmware. Cc: John Harrison <john.c.harrison@intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)