On Wed, Aug 11, 2021 at 06:06:36PM +0000, Matthew Brost wrote:
> On Tue, Aug 10, 2021 at 11:07:55AM +0200, Daniel Vetter wrote:
> > On Tue, Aug 10, 2021 at 10:53:37AM +0200, Daniel Vetter wrote:
> > > On Mon, Aug 09, 2021 at 06:58:23PM +0000, Matthew Brost wrote:
> > > > On Mon, Aug 09, 2021 at 05:17:34PM +0200, Daniel Vetter wrote:
> > > > > On Tue, Aug 03, 2021 at 03:29:13PM -0700, Matthew Brost wrote:
> > > > > > Implement GuC parent-child context pin / unpin functions in which in any
> > > > > > contexts in the relationship are pinned all the contexts are pinned. The
> > > > > > parent owns most of the pinning / unpinning process and the children
> > > > > > direct any pins / unpins to the parent.
> > > > > >
> > > > > > Patch implements a number of unused functions that will be connected
> > > > > > later in the series.
> > > > > >
> > > > > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/gt/intel_context.c | 187 ++++++++++++++++--
> > > > > > drivers/gpu/drm/i915/gt/intel_context.h | 43 +---
> > > > > > drivers/gpu/drm/i915/gt/intel_context_types.h | 4 +-
> > > > > > .../drm/i915/gt/intel_execlists_submission.c | 25 ++-
> > > > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 26 +--
> > > > > > drivers/gpu/drm/i915/gt/intel_lrc.h | 6 +-
> > > > > > .../gpu/drm/i915/gt/intel_ring_submission.c | 5 +-
> > > > > > drivers/gpu/drm/i915/gt/mock_engine.c | 4 +-
> > > > > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 183 +++++++++++++++--
> > > > > > 9 files changed, 371 insertions(+), 112 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > index 8cb92b10b547..bb4c14656067 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > > > > > @@ -158,8 +158,8 @@ static void __ring_retire(struct intel_ring *ring)
> > > > > > intel_ring_unpin(ring);
> > > > > > }
> > > > > >
> > > > > > -static int intel_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww)
> > > > > > +static int __intel_context_pre_pin(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > int err;
> > > > > >
> > > > > > @@ -190,7 +190,7 @@ static int intel_context_pre_pin(struct intel_context *ce,
> > > > > > return err;
> > > > > > }
> > > > > >
> > > > > > -static void intel_context_post_unpin(struct intel_context *ce)
> > > > > > +static void __intel_context_post_unpin(struct intel_context *ce)
> > > > > > {
> > > > > > if (ce->state)
> > > > > > __context_unpin_state(ce->state);
> > > > > > @@ -199,13 +199,85 @@ static void intel_context_post_unpin(struct intel_context *ce)
> > > > > > __ring_retire(ce->ring);
> > > > > > }
> > > > > >
> > > > > > -int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww)
> > > > > > +static int intel_context_pre_pin(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > - bool handoff = false;
> > > > > > - void *vaddr;
> > > > > > + struct intel_context *child;
> > > > > > + int err, i = 0;
> > > > > > +
> > > > > > + GEM_BUG_ON(intel_context_is_child(ce));
> > > > > > +
> > > > > > + for_each_child(ce, child) {
> > > > > > + err = __intel_context_pre_pin(child, ww);
> > > > > > + if (unlikely(err))
> > > > > > + goto unwind;
> > > > > > + ++i;
> > > > > > + }
> > > > > > +
> > > > > > + err = __intel_context_pre_pin(ce, ww);
> > > > > > + if (unlikely(err))
> > > > > > + goto unwind;
> > > > > > +
> > > > > > + return 0;
> > > > > > +
> > > > > > +unwind:
> > > > > > + for_each_child(ce, child) {
> > > > > > + if (!i--)
> > > > > > + break;
> > > > > > + __intel_context_post_unpin(ce);
> > > > > > + }
> > > > > > +
> > > > > > + return err;
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_context_post_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + struct intel_context *child;
> > > > > > +
> > > > > > + GEM_BUG_ON(intel_context_is_child(ce));
> > > > > > +
> > > > > > + for_each_child(ce, child)
> > > > > > + __intel_context_post_unpin(child);
> > > > > > +
> > > > > > + __intel_context_post_unpin(ce);
> > > > > > +}
> > > > > > +
> > > > > > +static int __do_ww_lock(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > +{
> > > > > > + int err = i915_gem_object_lock(ce->timeline->hwsp_ggtt->obj, ww);
> > > > > > +
> > > > > > + if (!err && ce->ring->vma->obj)
> > > > > > + err = i915_gem_object_lock(ce->ring->vma->obj, ww);
> > > > > > + if (!err && ce->state)
> > > > > > + err = i915_gem_object_lock(ce->state->obj, ww);
> > > > > > +
> > > > > > + return err;
> > > > > > +}
> > > > > > +
> > > > > > +static int do_ww_lock(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > +{
> > > > > > + struct intel_context *child;
> > > > > > int err = 0;
> > > > > >
> > > > > > + GEM_BUG_ON(intel_context_is_child(ce));
> > > > > > +
> > > > > > + for_each_child(ce, child) {
> > > > > > + err = __do_ww_lock(child, ww);
> > > > > > + if (unlikely(err))
> > > > > > + return err;
> > > > > > + }
> > > > > > +
> > > > > > + return __do_ww_lock(ce, ww);
> > > > > > +}
> > > > > > +
> > > > > > +static int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > +{
> > > > > > + bool handoff = false;
> > > > > > + int err;
> > > > > > +
> > > > > > if (unlikely(!test_bit(CONTEXT_ALLOC_BIT, &ce->flags))) {
> > > > > > err = intel_context_alloc_state(ce);
> > > > > > if (err)
> > > > > > @@ -217,14 +289,11 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > * refcount for __intel_context_active(), which prevent a lock
> > > > > > * inversion of ce->pin_mutex vs dma_resv_lock().
> > > > > > */
> > > > > > + err = do_ww_lock(ce, ww);
> > > > > > + if (err)
> > > > > > + return err;
> > > > > >
> > > > > > - err = i915_gem_object_lock(ce->timeline->hwsp_ggtt->obj, ww);
> > > > > > - if (!err && ce->ring->vma->obj)
> > > > > > - err = i915_gem_object_lock(ce->ring->vma->obj, ww);
> > > > > > - if (!err && ce->state)
> > > > > > - err = i915_gem_object_lock(ce->state->obj, ww);
> > > > > > - if (!err)
> > > > > > - err = intel_context_pre_pin(ce, ww);
> > > > > > + err = intel_context_pre_pin(ce, ww);
> > > > > > if (err)
> > > > > > return err;
> > > > > >
> > > > > > @@ -232,7 +301,7 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > if (err)
> > > > > > goto err_ctx_unpin;
> > > > > >
> > > > > > - err = ce->ops->pre_pin(ce, ww, &vaddr);
> > > > > > + err = ce->ops->pre_pin(ce, ww);
> > > > > > if (err)
> > > > > > goto err_release;
> > > > > >
> > > > > > @@ -250,7 +319,7 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > if (unlikely(err))
> > > > > > goto err_unlock;
> > > > > >
> > > > > > - err = ce->ops->pin(ce, vaddr);
> > > > > > + err = ce->ops->pin(ce);
> > > > > > if (err) {
> > > > > > intel_context_active_release(ce);
> > > > > > goto err_unlock;
> > > > > > @@ -290,7 +359,7 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > return err;
> > > > > > }
> > > > > >
> > > > > > -int __intel_context_do_pin(struct intel_context *ce)
> > > > > > +static int __intel_context_do_pin(struct intel_context *ce)
> > > > > > {
> > > > > > struct i915_gem_ww_ctx ww;
> > > > > > int err;
> > > > > > @@ -337,7 +406,7 @@ static void __intel_context_retire(struct i915_active *active)
> > > > > > intel_context_get_avg_runtime_ns(ce));
> > > > > >
> > > > > > set_bit(CONTEXT_VALID_BIT, &ce->flags);
> > > > > > - intel_context_post_unpin(ce);
> > > > > > + __intel_context_post_unpin(ce);
> > > > > > intel_context_put(ce);
> > > > > > }
> > > > > >
> > > > > > @@ -562,6 +631,88 @@ void intel_context_bind_parent_child(struct intel_context *parent,
> > > > > > child->parent = parent;
> > > > > > }
> > > > > >
> > > > > > +static inline int ____intel_context_pin(struct intel_context *ce)
> > > > > > +{
> > > > > > + if (likely(intel_context_pin_if_active(ce)))
> > > > > > + return 0;
> > > > > > +
> > > > > > + return __intel_context_do_pin(ce);
> > > > > > +}
> > > > > > +
> > > > > > +static inline int __intel_context_pin_ww(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > +{
> > > > > > + if (likely(intel_context_pin_if_active(ce)))
> > > > > > + return 0;
> > > > > > +
> > > > > > + return __intel_context_do_pin_ww(ce, ww);
> > > > > > +}
> > > > > > +
> > > > > > +static inline void __intel_context_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + if (!ce->ops->sched_disable) {
> > > > > > + __intel_context_do_unpin(ce, 1);
> > > > > > + } else {
> > > > > > + /*
> > > > > > + * Move ownership of this pin to the scheduling disable which is
> > > > > > + * an async operation. When that operation completes the above
> > > > > > + * intel_context_sched_disable_unpin is called potentially
> > > > > > + * unpinning the context.
> > > > > > + */
> > > > > > + while (!atomic_add_unless(&ce->pin_count, -1, 1)) {
> > > > > > + if (atomic_cmpxchg(&ce->pin_count, 1, 2) == 1) {
> >
> > Just as an example of what I mean here on the code review side. This is an
> > endless loop, and you need to prove that there's no livelock or starvation
> > issues. Or explain how else you handle that if there is one.
> >
>
> If we pop into the while loop the pin_count = 1, so in all likelihood the
> follow if evaluates to true and the loop is broken. Only way it
> evaluates to false is the context gets pinned between the while & if, so
> on the next pass the while statement should evaluate to false breaking
> the looping unless of course the contexts gets unpinned again... In
> pratice this should be at most 3 atomic operations unless the loop is
> broken.
That's not really how this works ... E.g. the linux spinlocks are
ticketed/queued locks, exactly because the "this likely doesn't happen"
argument is not a very good one.
Can't we at least convert intel_context->pin_count into a normal counter
with a spinlock around it and then just make all this stuff a lot more
reasonable?
Yes this doesn't help the situation overall much, but at least we're not
spreading dubious hand-roll locking patterns all over the place for not
much good reasons.
Hand-crafted artisanal locking is never a quality sign.
-Daniel
>
> Matt
>
> > Because unlike hand-rolled stuff linux kernel spinlocks are not dumb
> > spinlocks, but ticketed/queued locks and therefor starvation proof. But
> > this stuff actually matters on todays multi-core and not-so-uniform (even
> > without fully NUMA) architectures.
> >
> > Also I've just found another lockless retry loop which does actually
> > degenerate into a full endless loop (if you're sufficiently unlucky in
> > your races), so this really isn't academic at all.
> > -Daniel
> >
> > > > >
> > > > > Uh man lockless algorithms.
> > > > >
> > > > > Unless this comes:
> > > > > - with essentially an academic looking paper that describes the abstract
> > > > > model of the lockless algorithm and proves it against the linux kernel
> > > > > meory model.
> > > > >
> > > > > - lockless stuff generally needs barriers, and those barriers must be all
> > > > > documented. This means a) a comment next to each barrier in the code b)
> > > > > pointing to its counterparty c) with the overall design also explained
> > > > > in the kerneldoc for those datastructres.
> > > > >
> > > > > If you don't know where your barriers are, see above point about "it
> > > > > should look more like an academic paper in the commit message"
> > > > >
> > > > > - hard perf data about how this is absolutely required, based on a
> > > > > real-world use-case (which then sometimes justifies a microbenchmark
> > > > > metric for the details, but it always needs to be real-world based). And
> > > > > also a throughrough explainer how the perf issue isn't fixable through
> > > > > better design. If that's not doable, just protect the state machine with
> > > > > a big dumb lock and move on.
> > > > >
> > > > > - Also, because the current code is in such bad shape wrt lockless
> > > > > algorithms and premature optimizations: Overall complexity should go
> > > > > down (it's way too high right now), so pay down your new lockless trick
> > > > > by removing one of the existing ones that we only have because we can.
> > > > >
> > > > > Yes this is steep, but we're way out in the woods here and need to smoehow
> > > > > get back.
> > > >
> > > > See below FIXME. At one point all of this was hidden in the backend but
> > > > the dma-resv patches that landed upstream completely broke the layering,
> > > > hence the need for the code here.
> > > >
> > > > I guess I don't really understand what mean when you say lockless alg
> > > > needs barriers, if the atomic functions are not really atomic wouldn't
> > > > the world be broken?
> > >
> > > They unordered atomics by default. Which means they're atomic itself, but
> > > entirely unordered with anything else that's going on. Except when you
> > > have one of the atomic ops which already guarantee a barrier, or you
> > > manually add the barriers yourself. And yes there's enormous amounts of
> > > bugs, and with our dgpu potentially running on non-IA cpus those bugs
> > > matter.
> > >
> > > Note that in C++ atomics the default behaviour is strongly ordered atomics
> > > with full barriers by default, because those are much easier to program
> > > against. Kernel isn't like that and defaults to "you need to add all the
> > > barriers yourself".
> > >
> > > I have a full lenght rant in the works and will work that through all
> > > channels, but essentially locking is really hard to get right. And
> > > lockless tricks practically need an academic paper with a formal
> > > correctness proof against the linux memory model, or you do have bugs.
> > >
> > > And I know that the current code is choke full of this stuff, so it's
> > > tempting to just add more, but we really cant. The amount of locking
> > > trickery we have in the codebase must go down substantially. My take is
> > > that any code that adds anything trick needs to fully justify it against
> > > the above list, _and_ also clean up some of the existing nonsense so that
> > > overall complexity doesn't increase.
> > >
> > > I'll share the full length rant with you internally, it's not yet ready
> > > for publishing (but that's planned too).
> > >
> > >
> > > > Also here I don't think it is really as simple as grab big dump lock for
> > > > a variety of reasons, at least with the current dynamic pin / unpin code
> > > > in place. If we move a perma-pinned contexts this could be cleaned up
> > > > then.
> > >
> > > Yes it's a disaster, but we need to stop the bleeding. If perma-pinned
> > > context can fix this I think we should do this asap. I'd say for parallel
> > > context we should just do it outright (special case them or whatever) so
> > > that we don't have to add even more very tricky code and tech debt.
> > >
> > > Doable?
> > >
> > > Cheers, Daniel
> > >
> > >
> > > >
> > > > Matt
> > > >
> > > > > -Daniel
> > > > >
> > > > > > + ce->ops->sched_disable(ce);
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * FIXME: This is ugly, these branches are only needed for parallel contexts in
> > > > > > + * GuC submission. Basically the idea is if any of the contexts, that are
> > > > > > + * configured for parallel submission, are pinned all the contexts need to be
> > > > > > + * pinned in order to register these contexts with the GuC. We are adding the
> > > > > > + * layer here while it should probably be pushed to the backend via a vfunc. But
> > > > > > + * since we already have ce->pin + a layer atop it is confusing. Definitely
> > > > > > + * needs a bit of rework how to properly layer / structure this code path. What
> > > > > > + * is in place works but is not ideal.
> > > > > > + */
> > > > > > +int intel_context_pin(struct intel_context *ce)
> > > > > > +{
> > > > > > + if (intel_context_is_child(ce)) {
> > > > > > + if (!atomic_fetch_add(1, &ce->pin_count))
> > > > > > + return ____intel_context_pin(ce->parent);
> > > > > > + else
> > > > > > + return 0;
> > > > > > + } else {
> > > > > > + return ____intel_context_pin(ce);
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +int intel_context_pin_ww(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > +{
> > > > > > + if (intel_context_is_child(ce)) {
> > > > > > + if (!atomic_fetch_add(1, &ce->pin_count))
> > > > > > + return __intel_context_pin_ww(ce->parent, ww);
> > > > > > + else
> > > > > > + return 0;
> > > > > > + } else {
> > > > > > + return __intel_context_pin_ww(ce, ww);
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +void intel_context_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + if (intel_context_is_child(ce)) {
> > > > > > + if (atomic_fetch_add(-1, &ce->pin_count) == 1)
> > > > > > + __intel_context_unpin(ce->parent);
> > > > > > + } else {
> > > > > > + __intel_context_unpin(ce);
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> > > > > > #include "selftest_context.c"
> > > > > > #endif
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> > > > > > index ad6ce5ac4824..c208691fc87d 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_context.h
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> > > > > > @@ -110,31 +110,15 @@ static inline void intel_context_unlock_pinned(struct intel_context *ce)
> > > > > > mutex_unlock(&ce->pin_mutex);
> > > > > > }
> > > > > >
> > > > > > -int __intel_context_do_pin(struct intel_context *ce);
> > > > > > -int __intel_context_do_pin_ww(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww);
> > > > > > -
> > > > > > static inline bool intel_context_pin_if_active(struct intel_context *ce)
> > > > > > {
> > > > > > return atomic_inc_not_zero(&ce->pin_count);
> > > > > > }
> > > > > >
> > > > > > -static inline int intel_context_pin(struct intel_context *ce)
> > > > > > -{
> > > > > > - if (likely(intel_context_pin_if_active(ce)))
> > > > > > - return 0;
> > > > > > -
> > > > > > - return __intel_context_do_pin(ce);
> > > > > > -}
> > > > > > -
> > > > > > -static inline int intel_context_pin_ww(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww)
> > > > > > -{
> > > > > > - if (likely(intel_context_pin_if_active(ce)))
> > > > > > - return 0;
> > > > > > +int intel_context_pin(struct intel_context *ce);
> > > > > >
> > > > > > - return __intel_context_do_pin_ww(ce, ww);
> > > > > > -}
> > > > > > +int intel_context_pin_ww(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww);
> > > > > >
> > > > > > static inline void __intel_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > @@ -146,28 +130,11 @@ void __intel_context_do_unpin(struct intel_context *ce, int sub);
> > > > > >
> > > > > > static inline void intel_context_sched_disable_unpin(struct intel_context *ce)
> > > > > > {
> > > > > > + GEM_BUG_ON(intel_context_is_child(ce));
> > > > > > __intel_context_do_unpin(ce, 2);
> > > > > > }
> > > > > >
> > > > > > -static inline void intel_context_unpin(struct intel_context *ce)
> > > > > > -{
> > > > > > - if (!ce->ops->sched_disable) {
> > > > > > - __intel_context_do_unpin(ce, 1);
> > > > > > - } else {
> > > > > > - /*
> > > > > > - * Move ownership of this pin to the scheduling disable which is
> > > > > > - * an async operation. When that operation completes the above
> > > > > > - * intel_context_sched_disable_unpin is called potentially
> > > > > > - * unpinning the context.
> > > > > > - */
> > > > > > - while (!atomic_add_unless(&ce->pin_count, -1, 1)) {
> > > > > > - if (atomic_cmpxchg(&ce->pin_count, 1, 2) == 1) {
> > > > > > - ce->ops->sched_disable(ce);
> > > > > > - break;
> > > > > > - }
> > > > > > - }
> > > > > > - }
> > > > > > -}
> > > > > > +void intel_context_unpin(struct intel_context *ce);
> > > > > >
> > > > > > void intel_context_enter_engine(struct intel_context *ce);
> > > > > > void intel_context_exit_engine(struct intel_context *ce);
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > > > > > index 66b22b370a72..eb82be15b7a2 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > > > > > @@ -39,8 +39,8 @@ struct intel_context_ops {
> > > > > >
> > > > > > void (*ban)(struct intel_context *ce, struct i915_request *rq);
> > > > > >
> > > > > > - int (*pre_pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww, void **vaddr);
> > > > > > - int (*pin)(struct intel_context *ce, void *vaddr);
> > > > > > + int (*pre_pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww);
> > > > > > + int (*pin)(struct intel_context *ce);
> > > > > > void (*unpin)(struct intel_context *ce);
> > > > > > void (*post_unpin)(struct intel_context *ce);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > > > > index baa1797af1c8..fc74ca28f245 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > > > > @@ -2554,16 +2554,17 @@ static void execlists_submit_request(struct i915_request *request)
> > > > > > static int
> > > > > > __execlists_context_pre_pin(struct intel_context *ce,
> > > > > > struct intel_engine_cs *engine,
> > > > > > - struct i915_gem_ww_ctx *ww, void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > int err;
> > > > > >
> > > > > > - err = lrc_pre_pin(ce, engine, ww, vaddr);
> > > > > > + err = lrc_pre_pin(ce, engine, ww);
> > > > > > if (err)
> > > > > > return err;
> > > > > >
> > > > > > if (!__test_and_set_bit(CONTEXT_INIT_BIT, &ce->flags)) {
> > > > > > - lrc_init_state(ce, engine, *vaddr);
> > > > > > + lrc_init_state(ce, engine, ce->lrc_reg_state -
> > > > > > + LRC_STATE_OFFSET / sizeof(*ce->lrc_reg_state));
> > > > > >
> > > > > > __i915_gem_object_flush_map(ce->state->obj, 0, engine->context_size);
> > > > > > }
> > > > > > @@ -2572,15 +2573,14 @@ __execlists_context_pre_pin(struct intel_context *ce,
> > > > > > }
> > > > > >
> > > > > > static int execlists_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > - return __execlists_context_pre_pin(ce, ce->engine, ww, vaddr);
> > > > > > + return __execlists_context_pre_pin(ce, ce->engine, ww);
> > > > > > }
> > > > > >
> > > > > > -static int execlists_context_pin(struct intel_context *ce, void *vaddr)
> > > > > > +static int execlists_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > - return lrc_pin(ce, ce->engine, vaddr);
> > > > > > + return lrc_pin(ce, ce->engine);
> > > > > > }
> > > > > >
> > > > > > static int execlists_context_alloc(struct intel_context *ce)
> > > > > > @@ -3570,20 +3570,19 @@ static int virtual_context_alloc(struct intel_context *ce)
> > > > > > }
> > > > > >
> > > > > > static int virtual_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > struct virtual_engine *ve = container_of(ce, typeof(*ve), context);
> > > > > >
> > > > > > /* Note: we must use a real engine class for setting up reg state */
> > > > > > - return __execlists_context_pre_pin(ce, ve->siblings[0], ww, vaddr);
> > > > > > + return __execlists_context_pre_pin(ce, ve->siblings[0], ww);
> > > > > > }
> > > > > >
> > > > > > -static int virtual_context_pin(struct intel_context *ce, void *vaddr)
> > > > > > +static int virtual_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > struct virtual_engine *ve = container_of(ce, typeof(*ve), context);
> > > > > >
> > > > > > - return lrc_pin(ce, ve->siblings[0], vaddr);
> > > > > > + return lrc_pin(ce, ve->siblings[0]);
> > > > > > }
> > > > > >
> > > > > > static void virtual_context_enter(struct intel_context *ce)
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > > > > index bb4af4977920..c466fc966005 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > > > > @@ -947,30 +947,30 @@ void lrc_reset(struct intel_context *ce)
> > > > > > int
> > > > > > lrc_pre_pin(struct intel_context *ce,
> > > > > > struct intel_engine_cs *engine,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > + void *vaddr;
> > > > > > GEM_BUG_ON(!ce->state);
> > > > > > GEM_BUG_ON(!i915_vma_is_pinned(ce->state));
> > > > > >
> > > > > > - *vaddr = i915_gem_object_pin_map(ce->state->obj,
> > > > > > - i915_coherent_map_type(ce->engine->i915,
> > > > > > - ce->state->obj,
> > > > > > - false) |
> > > > > > - I915_MAP_OVERRIDE);
> > > > > > + vaddr = i915_gem_object_pin_map(ce->state->obj,
> > > > > > + i915_coherent_map_type(ce->engine->i915,
> > > > > > + ce->state->obj,
> > > > > > + false) |
> > > > > > + I915_MAP_OVERRIDE);
> > > > > >
> > > > > > - return PTR_ERR_OR_ZERO(*vaddr);
> > > > > > + ce->lrc_reg_state = vaddr + LRC_STATE_OFFSET;
> > > > > > +
> > > > > > + return PTR_ERR_OR_ZERO(vaddr);
> > > > > > }
> > > > > >
> > > > > > int
> > > > > > lrc_pin(struct intel_context *ce,
> > > > > > - struct intel_engine_cs *engine,
> > > > > > - void *vaddr)
> > > > > > + struct intel_engine_cs *engine)
> > > > > > {
> > > > > > - ce->lrc_reg_state = vaddr + LRC_STATE_OFFSET;
> > > > > > -
> > > > > > if (!__test_and_set_bit(CONTEXT_INIT_BIT, &ce->flags))
> > > > > > - lrc_init_state(ce, engine, vaddr);
> > > > > > + lrc_init_state(ce, engine,
> > > > > > + (void *)ce->lrc_reg_state - LRC_STATE_OFFSET);
> > > > > >
> > > > > > ce->lrc.lrca = lrc_update_regs(ce, engine, ce->ring->tail);
> > > > > > return 0;
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h b/drivers/gpu/drm/i915/gt/intel_lrc.h
> > > > > > index 7f697845c4cf..837fcf00270d 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.h
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
> > > > > > @@ -38,12 +38,10 @@ void lrc_destroy(struct kref *kref);
> > > > > > int
> > > > > > lrc_pre_pin(struct intel_context *ce,
> > > > > > struct intel_engine_cs *engine,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr);
> > > > > > + struct i915_gem_ww_ctx *ww);
> > > > > > int
> > > > > > lrc_pin(struct intel_context *ce,
> > > > > > - struct intel_engine_cs *engine,
> > > > > > - void *vaddr);
> > > > > > + struct intel_engine_cs *engine);
> > > > > > void lrc_unpin(struct intel_context *ce);
> > > > > > void lrc_post_unpin(struct intel_context *ce);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > > > > > index 2958e2fae380..f4f301bfb9f7 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > > > > > @@ -472,8 +472,7 @@ static int ring_context_init_default_state(struct intel_context *ce,
> > > > > > }
> > > > > >
> > > > > > static int ring_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **unused)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > struct i915_address_space *vm;
> > > > > > int err = 0;
> > > > > > @@ -576,7 +575,7 @@ static int ring_context_alloc(struct intel_context *ce)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > -static int ring_context_pin(struct intel_context *ce, void *unused)
> > > > > > +static int ring_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > return 0;
> > > > > > }
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> > > > > > index 2c1af030310c..826b5d7a4573 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> > > > > > @@ -167,12 +167,12 @@ static int mock_context_alloc(struct intel_context *ce)
> > > > > > }
> > > > > >
> > > > > > static int mock_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww, void **unused)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > -static int mock_context_pin(struct intel_context *ce, void *unused)
> > > > > > +static int mock_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > return 0;
> > > > > > }
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > > > index dec757d319a2..c5c73c42bcf7 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > > > > @@ -1905,6 +1905,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> > > > > >
> > > > > > GEM_BUG_ON(!engine->mask);
> > > > > > GEM_BUG_ON(context_guc_id_invalid(ce));
> > > > > > + GEM_BUG_ON(intel_context_is_child(ce));
> > > > > >
> > > > > > /*
> > > > > > * Ensure LRC + CT vmas are is same region as write barrier is done
> > > > > > @@ -2008,15 +2009,13 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> > > > > >
> > > > > > static int __guc_context_pre_pin(struct intel_context *ce,
> > > > > > struct intel_engine_cs *engine,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > - return lrc_pre_pin(ce, engine, ww, vaddr);
> > > > > > + return lrc_pre_pin(ce, engine, ww);
> > > > > > }
> > > > > >
> > > > > > static int __guc_context_pin(struct intel_context *ce,
> > > > > > - struct intel_engine_cs *engine,
> > > > > > - void *vaddr)
> > > > > > + struct intel_engine_cs *engine)
> > > > > > {
> > > > > > if (i915_ggtt_offset(ce->state) !=
> > > > > > (ce->lrc.lrca & CTX_GTT_ADDRESS_MASK))
> > > > > > @@ -2027,20 +2026,33 @@ static int __guc_context_pin(struct intel_context *ce,
> > > > > > * explaination of why.
> > > > > > */
> > > > > >
> > > > > > - return lrc_pin(ce, engine, vaddr);
> > > > > > + return lrc_pin(ce, engine);
> > > > > > +}
> > > > > > +
> > > > > > +static void __guc_context_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + lrc_unpin(ce);
> > > > > > +}
> > > > > > +
> > > > > > +static void __guc_context_post_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + lrc_post_unpin(ce);
> > > > > > }
> > > > > >
> > > > > > static int guc_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > - return __guc_context_pre_pin(ce, ce->engine, ww, vaddr);
> > > > > > + return __guc_context_pre_pin(ce, ce->engine, ww);
> > > > > > }
> > > > > >
> > > > > > -static int guc_context_pin(struct intel_context *ce, void *vaddr)
> > > > > > +static int guc_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > - int ret = __guc_context_pin(ce, ce->engine, vaddr);
> > > > > > + int ret;
> > > > > >
> > > > > > + GEM_BUG_ON(intel_context_is_parent(ce) ||
> > > > > > + intel_context_is_child(ce));
> > > > > > +
> > > > > > + ret = __guc_context_pin(ce, ce->engine);
> > > > > > if (likely(!ret && !intel_context_is_barrier(ce)))
> > > > > > intel_engine_pm_get(ce->engine);
> > > > > >
> > > > > > @@ -2054,7 +2066,7 @@ static void guc_context_unpin(struct intel_context *ce)
> > > > > > GEM_BUG_ON(context_enabled(ce));
> > > > > >
> > > > > > unpin_guc_id(guc, ce, true);
> > > > > > - lrc_unpin(ce);
> > > > > > + __guc_context_unpin(ce);
> > > > > >
> > > > > > if (likely(!intel_context_is_barrier(ce)))
> > > > > > intel_engine_pm_put(ce->engine);
> > > > > > @@ -2062,7 +2074,141 @@ static void guc_context_unpin(struct intel_context *ce)
> > > > > >
> > > > > > static void guc_context_post_unpin(struct intel_context *ce)
> > > > > > {
> > > > > > - lrc_post_unpin(ce);
> > > > > > + __guc_context_post_unpin(ce);
> > > > > > +}
> > > > > > +
> > > > > > +/* Future patches will use this function */
> > > > > > +__maybe_unused
> > > > > > +static int guc_parent_context_pre_pin(struct intel_context *ce,
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > +{
> > > > > > + struct intel_context *child;
> > > > > > + int err, i = 0, j = 0;
> > > > > > +
> > > > > > + for_each_child(ce, child) {
> > > > > > + err = i915_active_acquire(&child->active);
> > > > > > + if (unlikely(err))
> > > > > > + goto unwind_active;
> > > > > > + ++i;
> > > > > > + }
> > > > > > +
> > > > > > + for_each_child(ce, child) {
> > > > > > + err = __guc_context_pre_pin(child, child->engine, ww);
> > > > > > + if (unlikely(err))
> > > > > > + goto unwind_pre_pin;
> > > > > > + ++j;
> > > > > > + }
> > > > > > +
> > > > > > + err = __guc_context_pre_pin(ce, ce->engine, ww);
> > > > > > + if (unlikely(err))
> > > > > > + goto unwind_pre_pin;
> > > > > > +
> > > > > > + return 0;
> > > > > > +
> > > > > > +unwind_pre_pin:
> > > > > > + for_each_child(ce, child) {
> > > > > > + if (!j--)
> > > > > > + break;
> > > > > > + __guc_context_post_unpin(child);
> > > > > > + }
> > > > > > +
> > > > > > +unwind_active:
> > > > > > + for_each_child(ce, child) {
> > > > > > + if (!i--)
> > > > > > + break;
> > > > > > + i915_active_release(&child->active);
> > > > > > + }
> > > > > > +
> > > > > > + return err;
> > > > > > +}
> > > > > > +
> > > > > > +/* Future patches will use this function */
> > > > > > +__maybe_unused
> > > > > > +static void guc_parent_context_post_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + struct intel_context *child;
> > > > > > +
> > > > > > + for_each_child(ce, child)
> > > > > > + __guc_context_post_unpin(child);
> > > > > > + __guc_context_post_unpin(ce);
> > > > > > +
> > > > > > + for_each_child(ce, child) {
> > > > > > + intel_context_get(child);
> > > > > > + i915_active_release(&child->active);
> > > > > > + intel_context_put(child);
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +/* Future patches will use this function */
> > > > > > +__maybe_unused
> > > > > > +static int guc_parent_context_pin(struct intel_context *ce)
> > > > > > +{
> > > > > > + int ret, i = 0, j = 0;
> > > > > > + struct intel_context *child;
> > > > > > + struct intel_engine_cs *engine;
> > > > > > + intel_engine_mask_t tmp;
> > > > > > +
> > > > > > + GEM_BUG_ON(!intel_context_is_parent(ce));
> > > > > > +
> > > > > > + for_each_child(ce, child) {
> > > > > > + ret = __guc_context_pin(child, child->engine);
> > > > > > + if (unlikely(ret))
> > > > > > + goto unwind_pin;
> > > > > > + ++i;
> > > > > > + }
> > > > > > + ret = __guc_context_pin(ce, ce->engine);
> > > > > > + if (unlikely(ret))
> > > > > > + goto unwind_pin;
> > > > > > +
> > > > > > + for_each_child(ce, child)
> > > > > > + if (test_bit(CONTEXT_LRCA_DIRTY, &child->flags)) {
> > > > > > + set_bit(CONTEXT_LRCA_DIRTY, &ce->flags);
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + for_each_engine_masked(engine, ce->engine->gt,
> > > > > > + ce->engine->mask, tmp)
> > > > > > + intel_engine_pm_get(engine);
> > > > > > + for_each_child(ce, child)
> > > > > > + for_each_engine_masked(engine, child->engine->gt,
> > > > > > + child->engine->mask, tmp)
> > > > > > + intel_engine_pm_get(engine);
> > > > > > +
> > > > > > + return 0;
> > > > > > +
> > > > > > +unwind_pin:
> > > > > > + for_each_child(ce, child) {
> > > > > > + if (++j > i)
> > > > > > + break;
> > > > > > + __guc_context_unpin(child);
> > > > > > + }
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +/* Future patches will use this function */
> > > > > > +__maybe_unused
> > > > > > +static void guc_parent_context_unpin(struct intel_context *ce)
> > > > > > +{
> > > > > > + struct intel_context *child;
> > > > > > + struct intel_engine_cs *engine;
> > > > > > + intel_engine_mask_t tmp;
> > > > > > +
> > > > > > + GEM_BUG_ON(!intel_context_is_parent(ce));
> > > > > > + GEM_BUG_ON(context_enabled(ce));
> > > > > > +
> > > > > > + unpin_guc_id(ce_to_guc(ce), ce, true);
> > > > > > + for_each_child(ce, child)
> > > > > > + __guc_context_unpin(child);
> > > > > > + __guc_context_unpin(ce);
> > > > > > +
> > > > > > + for_each_engine_masked(engine, ce->engine->gt,
> > > > > > + ce->engine->mask, tmp)
> > > > > > + intel_engine_pm_put(engine);
> > > > > > + for_each_child(ce, child)
> > > > > > + for_each_engine_masked(engine, child->engine->gt,
> > > > > > + child->engine->mask, tmp)
> > > > > > + intel_engine_pm_put(engine);
> > > > > > }
> > > > > >
> > > > > > static void __guc_context_sched_enable(struct intel_guc *guc,
> > > > > > @@ -2993,18 +3139,17 @@ static int guc_request_alloc(struct i915_request *rq)
> > > > > > }
> > > > > >
> > > > > > static int guc_virtual_context_pre_pin(struct intel_context *ce,
> > > > > > - struct i915_gem_ww_ctx *ww,
> > > > > > - void **vaddr)
> > > > > > + struct i915_gem_ww_ctx *ww)
> > > > > > {
> > > > > > struct intel_engine_cs *engine = guc_virtual_get_sibling(ce->engine, 0);
> > > > > >
> > > > > > - return __guc_context_pre_pin(ce, engine, ww, vaddr);
> > > > > > + return __guc_context_pre_pin(ce, engine, ww);
> > > > > > }
> > > > > >
> > > > > > -static int guc_virtual_context_pin(struct intel_context *ce, void *vaddr)
> > > > > > +static int guc_virtual_context_pin(struct intel_context *ce)
> > > > > > {
> > > > > > struct intel_engine_cs *engine = guc_virtual_get_sibling(ce->engine, 0);
> > > > > > - int ret = __guc_context_pin(ce, engine, vaddr);
> > > > > > + int ret = __guc_context_pin(ce, engine);
> > > > > > intel_engine_mask_t tmp, mask = ce->engine->mask;
> > > > > >
> > > > > > if (likely(!ret))
> > > > > > @@ -3024,7 +3169,7 @@ static void guc_virtual_context_unpin(struct intel_context *ce)
> > > > > > GEM_BUG_ON(intel_context_is_barrier(ce));
> > > > > >
> > > > > > unpin_guc_id(guc, ce, true);
> > > > > > - lrc_unpin(ce);
> > > > > > + __guc_context_unpin(ce);
> > > > > >
> > > > > > for_each_engine_masked(engine, ce->engine->gt, mask, tmp)
> > > > > > intel_engine_pm_put(engine);
> > > > > > --
> > > > > > 2.28.0
> > > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
