On Tue, May 11, 2021 at 05:37:54PM +0200, Daniel Vetter wrote:
> On Thu, May 06, 2021 at 12:14:03PM -0700, 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@xxxxxxxxx>
> > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>
> Instead of these ifs in the code, can we push this barrier business down
> into backends?
>
Not a bad idea. This is an example of what I think of implict behavior of the
backend creeping into the higher levels.
> Not in this series, but as one of the things to sort out as part of the
> conversion to drm/scheduler.
Agree. After basic GuC submission gets merged maybe we go through the code and
remove all the implict backend assumptions.
Matt
> -Daniel
>
> > ---
> > 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 ++++++++++
> > drivers/gpu/drm/i915/i915_active.c | 3 +++
> > 4 files changed, 15 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;
> >
> > /* Preallocate tracking nodes */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> > index 92ecbab8c1cd..9b211ca5ecc7 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",
> > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> > index b1aa1c482c32..9a264898bb91 100644
> > --- a/drivers/gpu/drm/i915/i915_active.c
> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -968,6 +968,9 @@ void i915_active_acquire_barrier(struct i915_active *ref)
> >
> > GEM_BUG_ON(i915_active_is_idle(ref));
> >
> > + if (llist_empty(&ref->preallocated_barriers))
> > + return;
> > +
> > /*
> > * Transfer the list of preallocated barriers into the
> > * i915_active rbtree, but only as proto-nodes. They will be
> > --
> > 2.28.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
