On Fri, Jul 09, 2021 at 04:53:37PM -0700, John Harrison wrote: > On 6/24/2021 00:04, Matthew Brost wrote: > > 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. Also long direction is > > just to delete semaphores from the i915 so another reason to not enable > > these for GuC submission. > > > > v2: Reword commit message > > > > Cc: John Harrison <john.c.harrison@xxxxxxxxx> > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx> > I think the commit description does not really match the patch content. The > description is valid but the 'disable' is done by simply not setting the > enable flag (done in the execlist back end and presumably not done in the > GuC back end). However, what the patch is actually doing seems to be fixing > bugs with the 'are semaphores enabled' mechanism. I.e. correcting pieces of > code that used semaphores without checking if they are enabled. And > presumably this would be broken if someone tried to disable semaphores in > execlist mode for any reason? > > So I think keeping the existing comment text is fine but something should be > added to explain the actual changes. > Yes, commit is wrong. This more or less bug fix to the existing code. Will update. Matt > John. > > > > --- > > 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 7720b8c22c81..5c07e6abf16a 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); > > @@ -1938,7 +1939,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); >
