On Fri, Sep 10, 2021 at 09:36:17AM +0100, Tvrtko Ursulin wrote:
>
> On 20/08/2021 23:44, Matthew Brost wrote:
> > Sometimes it is desirable to queue work up for later if the GT PM isn't
> > held and run that work on next GT PM unpark.
>
> Sounds maybe plausible, but it depends how much work can happen on unpark
> and whether it can have too much of a negative impact on latency for
> interactive loads? Or from a reverse angle, why the work wouldn't be done on
All it is does is add an interface to kick a work queue on unpark. i.e.
All the actually work is done async in the work queue so it shouldn't
add any latency.
> parking?
>
> Also what kind of mechanism for dealing with too much stuff being put on
> this list you have? Can there be pressure which triggers (or would need to
No limits on pressure. See above, I don't think this is a concern.
> trigger) these deregistrations to happen at runtime (no park/unpark
> transitions)?
>
> > Implemented with a list in the GT of all pending work, workqueues in
> > the list, a callback to add a workqueue to the list, and finally a
> > wakeref post_get callback that iterates / drains the list + queues the
> > workqueues.
> >
> > First user of this is deregistration of GuC contexts.
>
> Does first imply there are more incoming?
>
Haven't found another user yet but this is generic mechanism so we can
add more in the future if other use cases arrise.
> > Signed-off-by: Matthew Brost <matthew.br...@intel.com>
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/gt/intel_gt.c | 3 ++
> > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 8 ++++
> > .../gpu/drm/i915/gt/intel_gt_pm_unpark_work.c | 35 ++++++++++++++++
> > .../gpu/drm/i915/gt/intel_gt_pm_unpark_work.h | 40 +++++++++++++++++++
> > drivers/gpu/drm/i915/gt/intel_gt_types.h | 10 +++++
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 8 ++--
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 15 +++++--
> > drivers/gpu/drm/i915/intel_wakeref.c | 5 +++
> > drivers/gpu/drm/i915/intel_wakeref.h | 1 +
> > 10 files changed, 119 insertions(+), 7 deletions(-)
> > create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.c
> > create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 642a5b5a1b81..579bdc069f25 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -103,6 +103,7 @@ gt-y += \
> > gt/intel_gt_clock_utils.o \
> > gt/intel_gt_irq.o \
> > gt/intel_gt_pm.o \
> > + gt/intel_gt_pm_unpark_work.o \
> > gt/intel_gt_pm_irq.o \
> > gt/intel_gt_requests.o \
> > gt/intel_gtt.o \
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 62d40c986642..7e690e74baa2 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -29,6 +29,9 @@ void intel_gt_init_early(struct intel_gt *gt, struct
> > drm_i915_private *i915)
> > spin_lock_init(>->irq_lock);
> > + spin_lock_init(>->pm_unpark_work_lock);
> > + INIT_LIST_HEAD(>->pm_unpark_work_list);
> > +
> > INIT_LIST_HEAD(>->closed_vma);
> > spin_lock_init(>->closed_lock);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > index dea8e2479897..564c11a3748b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > @@ -90,6 +90,13 @@ static int __gt_unpark(struct intel_wakeref *wf)
> > return 0;
> > }
> > +static void __gt_unpark_work_queue(struct intel_wakeref *wf)
> > +{
> > + struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
> > +
> > + intel_gt_pm_unpark_work_queue(gt);
> > +}
> > +
> > static int __gt_park(struct intel_wakeref *wf)
> > {
> > struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
> > @@ -118,6 +125,7 @@ static int __gt_park(struct intel_wakeref *wf)
> > static const struct intel_wakeref_ops wf_ops = {
> > .get = __gt_unpark,
> > + .post_get = __gt_unpark_work_queue,
> > .put = __gt_park,
> > };
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.c
> > b/drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.c
> > new file mode 100644
> > index 000000000000..23162dbd0c35
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.c
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2021 Intel Corporation
> > + */
> > +
> > +#include "i915_drv.h"
> > +#include "intel_runtime_pm.h"
> > +#include "intel_gt_pm.h"
> > +
> > +void intel_gt_pm_unpark_work_queue(struct intel_gt *gt)
> > +{
> > + struct intel_gt_pm_unpark_work *work, *next;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(>->pm_unpark_work_lock, flags);
> > + list_for_each_entry_safe(work, next,
> > + >->pm_unpark_work_list, link) {
> > + list_del_init(&work->link);
> > + queue_work(system_unbound_wq, &work->worker);
> > + }
> > + spin_unlock_irqrestore(>->pm_unpark_work_lock, flags);
> > +}
> > +
> > +void intel_gt_pm_unpark_work_add(struct intel_gt *gt,
> > + struct intel_gt_pm_unpark_work *work)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(>->pm_unpark_work_lock, flags);
> > + if (intel_gt_pm_is_awake(gt))
> > + queue_work(system_unbound_wq, &work->worker);
> > + else if (list_empty(&work->link))
>
> What's the list_empty check for, something can race by design?
>
This function is allowed to be called twice, e.g. Two contexts can be
tried to be deregistered while the GT PM is ideal but only first context
results in the worker being added to the list of workers to be kicked on
unpark.
> > + list_add_tail(&work->link, >->pm_unpark_work_list);
> > + spin_unlock_irqrestore(>->pm_unpark_work_lock, flags);
> > +}
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.h
> > new file mode 100644
> > index 000000000000..eaf1dc313aa2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_unpark_work.h
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2021 Intel Corporation
> > + */
> > +
> > +#ifndef INTEL_GT_PM_UNPARK_WORK_H
> > +#define INTEL_GT_PM_UNPARK_WORK_H
> > +
> > +#include <linux/list.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct intel_gt;
> > +
> > +/**
> > + * struct intel_gt_pm_unpark_work - work to be scheduled when GT unparked
> > + */
> > +struct intel_gt_pm_unpark_work {
> > + /**
> > + * @link: link into gt->pm_unpark_work_list of workers that need to be
> > + * scheduled when GT is unpark, protected by gt->pm_unpark_work_lock
> > + */
> > + struct list_head link;
> > + /** @worker: will be scheduled when GT unparked */
> > + struct work_struct worker;
> > +};
> > +
> > +void intel_gt_pm_unpark_work_queue(struct intel_gt *gt);
> > +
> > +void intel_gt_pm_unpark_work_add(struct intel_gt *gt,
> > + struct intel_gt_pm_unpark_work *work);
> > +
> > +static inline void
> > +intel_gt_pm_unpark_work_init(struct intel_gt_pm_unpark_work *work,
> > + work_func_t fn)
> > +{
> > + INIT_LIST_HEAD(&work->link);
> > + INIT_WORK(&work->worker, fn);
> > +}
> > +
> > +#endif /* INTEL_GT_PM_UNPARK_WORK_H */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index a81e21bf1bd1..4480312f0add 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -96,6 +96,16 @@ struct intel_gt {
> > struct intel_wakeref wakeref;
> > atomic_t user_wakeref;
> > + /**
> > + * @pm_unpark_work_list: list of delayed work to scheduled which GT is
> > + * unparked, protected by pm_unpark_work_lock
> > + */
> > + struct list_head pm_unpark_work_list;
> > + /**
> > + * @pm_unpark_work_lock: protects pm_unpark_work_list
> > + */
> > + spinlock_t pm_unpark_work_lock;
> > +
> > struct list_head closed_vma;
> > spinlock_t closed_lock; /* guards the list of closed_vma */
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 7358883f1540..023953e77553 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -19,6 +19,7 @@
> > #include "intel_uc_fw.h"
> > #include "i915_utils.h"
> > #include "i915_vma.h"
> > +#include "gt/intel_gt_pm_unpark_work.h"
> > struct __guc_ads_blob;
> > @@ -78,11 +79,12 @@ struct intel_guc {
> > */
> > struct list_head destroyed_contexts;
> > /**
> > - * @destroyed_worker: worker to deregister contexts, need as we
> > + * @destroyed_worker: Worker to deregister contexts, need as we
> > * need to take a GT PM reference and can't from destroy
> > - * function as it might be in an atomic context (no sleeping)
> > + * function as it might be in an atomic context (no sleeping).
> > + * Worker only issues deregister when GT is unparked.
> > */
> > - struct work_struct destroyed_worker;
> > + struct intel_gt_pm_unpark_work destroyed_worker;
> > } submission_state;
> > bool submission_supported;
> > 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 f835e06e5f9f..dbf919801de2 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -1135,7 +1135,8 @@ int intel_guc_submission_init(struct intel_guc *guc)
> > INIT_LIST_HEAD(&guc->submission_state.guc_id_list);
> > ida_init(&guc->submission_state.guc_ids);
> > INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
> > - INIT_WORK(&guc->submission_state.destroyed_worker,
> > destroyed_worker_func);
> > + intel_gt_pm_unpark_work_init(&guc->submission_state.destroyed_worker,
> > + destroyed_worker_func);
> > return 0;
> > }
> > @@ -1942,13 +1943,18 @@ static void deregister_destroyed_contexts(struct
> > intel_guc *guc)
> > static void destroyed_worker_func(struct work_struct *w)
> > {
> > - struct intel_guc *guc = container_of(w, struct intel_guc,
> > + struct intel_gt_pm_unpark_work *destroyed_worker =
> > + container_of(w, struct intel_gt_pm_unpark_work, worker);
> > + struct intel_guc *guc = container_of(destroyed_worker, struct intel_guc,
> > submission_state.destroyed_worker);
> > struct intel_gt *gt = guc_to_gt(guc);
> > int tmp;
> > - with_intel_gt_pm(gt, tmp)
> > + with_intel_gt_pm_if_awake(gt, tmp)
> > deregister_destroyed_contexts(guc);
> > +
> > + if (!list_empty(&guc->submission_state.destroyed_contexts))
> > + intel_gt_pm_unpark_work_add(gt, destroyed_worker);
>
> This is the worker itself, right?
>
Yes.
> There's a "if awake" here followed by another "if awake" inside
> intel_gt_pm_unpark_work_add which raises questions.
>
Even in the worker we only deregister the context if the GT is awake.
> Second question is what's the list_empty for - why is the state of the list
> itself relevant to a single worker deciding whether to re-add itself to it
> or not? And is there a lock protecting this list?
>
Yes we have locking - pm_unpark_work_lock. It is basically all 2
threads to call intel_gt_pm_unpark_work_add while the GT is idle - in
that case only the first call adds the worker to the list of workers to
kick when unparked (same explaination as above).
> On the overall it feels questionable to have unpark work which apparently
> can race with subsequent parking. Presumably you cannot have it run sync on
> unpark due execution context issues?
>
No race, just allowed to be called twice.
> > }
> > static void guc_context_destroy(struct kref *kref)
> > @@ -1985,7 +1991,8 @@ static void guc_context_destroy(struct kref *kref)
> > * take the GT PM for the first time which isn't allowed from an atomic
> > * context.
> > */
> > - queue_work(system_unbound_wq, &guc->submission_state.destroyed_worker);
> > + intel_gt_pm_unpark_work_add(guc_to_gt(guc),
> > + &guc->submission_state.destroyed_worker);
> > }
> > static int guc_context_alloc(struct intel_context *ce)
> > diff --git a/drivers/gpu/drm/i915/intel_wakeref.c
> > b/drivers/gpu/drm/i915/intel_wakeref.c
> > index dfd87d082218..282fc4f312e3 100644
> > --- a/drivers/gpu/drm/i915/intel_wakeref.c
> > +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> > @@ -24,6 +24,8 @@ static void rpm_put(struct intel_wakeref *wf)
> > int __intel_wakeref_get_first(struct intel_wakeref *wf)
> > {
> > + bool do_post = false;
> > +
> > /*
> > * Treat get/put as different subclasses, as we may need to run
> > * the put callback from under the shrinker and do not want to
> > @@ -44,8 +46,11 @@ int __intel_wakeref_get_first(struct intel_wakeref *wf)
> > }
> > smp_mb__before_atomic(); /* release wf->count */
> > + do_post = true;
> > }
> > atomic_inc(&wf->count);
> > + if (do_post && wf->ops->post_get)
> > + wf->ops->post_get(wf);
>
> You want this hook under the wf->mutex and why?
>
I didn't really think about this but everything else in under the mutex
so I included this under the mutex too. In this case this post_get op
could likely be outside the mutex but I think it harmless to keep it
under the mutex. For future safety, I think it should stay under the
mutex.
Matt
> Regards,
>
> Tvrtko
>
> > mutex_unlock(&wf->mutex);
> > INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
> > diff --git a/drivers/gpu/drm/i915/intel_wakeref.h
> > b/drivers/gpu/drm/i915/intel_wakeref.h
> > index 545c8f277c46..ef7e6a698e8a 100644
> > --- a/drivers/gpu/drm/i915/intel_wakeref.h
> > +++ b/drivers/gpu/drm/i915/intel_wakeref.h
> > @@ -30,6 +30,7 @@ typedef depot_stack_handle_t intel_wakeref_t;
> > struct intel_wakeref_ops {
> > int (*get)(struct intel_wakeref *wf);
> > + void (*post_get)(struct intel_wakeref *wf);
> > int (*put)(struct intel_wakeref *wf);
> > };
> >
