Re: [PATCH 20/47] drm/i915/guc: Disable semaphores when using GuC scheduling

[Matthew Brost <matthew.brost@intel.com>]

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@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> 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);
>