[Intel-gfx] [PATCH 16/47] drm/i915/guc: Disable engine barriers with GuC during unpin

[Matthew Brost]

On Fri, Jul 09, 2021 at 03:53:29PM -0700, John Harrison wrote:
> On 6/24/2021 00:04, Matthew Brost wrote:
> > Disable engine barriers for unpinning with GuC. This feature isn't
> > needed with the GuC as it disables context scheduling before unpinning
> > which guarantees the HW will not reference the context. Hence it is
> > not necessary to defer unpinning until a kernel context request
> > completes on each engine in the context engine mask.
> > 
> > Cc: John Harrison <john.c.harrison at intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_context.c    |  2 +-
> >   drivers/gpu/drm/i915/gt/intel_context.h    |  1 +
> >   drivers/gpu/drm/i915/gt/selftest_context.c | 10 ++++++++++
> >   3 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 1499b8aace2a..7f97753ab164 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -80,7 +80,7 @@ static int intel_context_active_acquire(struct intel_context *ce)
> >   	__i915_active_acquire(&ce->active);
> > -	if (intel_context_is_barrier(ce))
> > +	if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine))
> >   		return 0;
> Would be better to have a scheduler flag to say whether barriers are
> required or not. That would prevent polluting front end code with back end
> details.
> 

I guess an engine flag is slightly better but I still don't love that
as we have to test if the context is a barrier (kernel context) and then
call a function that is basically backend specific after. IMO we really
need to push all of this to a vfunc. If you really want me to make this
an engine flag I can, but in the end it just seems like that will
trash the code (adding an engine flag just to remove it). I think this
is just a clean up we write down, and figure out a bit later as nothing
is functionally wrong + quite clear that it is something that should be
cleaned up.

Matt

> John.
> 
> 
> >   	/* Preallocate tracking nodes */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> > index 8a7199afbe61..a592a9605dc8 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> > @@ -16,6 +16,7 @@
> >   #include "intel_engine_types.h"
> >   #include "intel_ring_types.h"
> >   #include "intel_timeline_types.h"
> > +#include "uc/intel_guc_submission.h"
> >   #define CE_TRACE(ce, fmt, ...) do {					\
> >   	const struct intel_context *ce__ = (ce);			\
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
> > index 26685b927169..fa7b99a671dd 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> > @@ -209,7 +209,13 @@ static int __live_active_context(struct intel_engine_cs *engine)
> >   	 * This test makes sure that the context is kept alive until a
> >   	 * subsequent idle-barrier (emitted when the engine wakeref hits 0
> >   	 * with no more outstanding requests).
> > +	 *
> > +	 * In GuC submission mode we don't use idle barriers and we instead
> > +	 * get a message from the GuC to signal that it is safe to unpin the
> > +	 * context from memory.
> >   	 */
> > +	if (intel_engine_uses_guc(engine))
> > +		return 0;
> >   	if (intel_engine_pm_is_awake(engine)) {
> >   		pr_err("%s is awake before starting %s!\n",
> > @@ -357,7 +363,11 @@ static int __live_remote_context(struct intel_engine_cs *engine)
> >   	 * on the context image remotely (intel_context_prepare_remote_request),
> >   	 * which inserts foreign fences into intel_context.active, does not
> >   	 * clobber the idle-barrier.
> > +	 *
> > +	 * In GuC submission mode we don't use idle barriers.
> >   	 */
> > +	if (intel_engine_uses_guc(engine))
> > +		return 0;
> >   	if (intel_engine_pm_is_awake(engine)) {
> >   		pr_err("%s is awake before starting %s!\n",
>