[Intel-gfx] [PATCH 3/3] drm/i915: Defer final intel_wakeref_put to process context
Mika Kuoppala
mika.kuoppala at linux.intel.com
Wed Aug 7 15:04:56 UTC 2019
Chris Wilson <chris at chris-wilson.co.uk> writes:
> As we need to acquire a mutex to serialise the final
> intel_wakeref_put, we need to ensure that we are in process context at
> that time. However, we want to allow operation on the intel_wakeref from
> inside timer and other hardirq context, which means that need to defer
> that final put to a workqueue.
>
> Inside the final wakeref puts, we are safe to operate in any context, as
> we are simply marking up the HW and state tracking for the potential
> sleep. It's only the serialisation with the potential sleeping getting
> that requires careful wait avoidance. This allows us to retain the
> immediate processing as before (we only need to sleep over the same
> races as the current mutex_lock).
>
> v2: Add a selftest to ensure we exercise the code while lockdep watches.
> v3: That test was extremely loud and complained about many things!
>
Loud is good if it did found out something, did it? :)
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111295
> References: https://bugs.freedesktop.org/show_bug.cgi?id=111245
> References: https://bugs.freedesktop.org/show_bug.cgi?id=111256
> Fixes: 18398904ca9e ("drm/i915: Only recover active engines")
> Fixes: 51fbd8de87dc ("drm/i915/pmu: Atomically acquire the gt_pm wakeref")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_pm.c | 13 +--
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 1 +
> drivers/gpu/drm/i915/gt/intel_engine_pm.c | 38 +++------
> drivers/gpu/drm/i915/gt/intel_engine_pm.h | 18 ++--
> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 28 +++----
> drivers/gpu/drm/i915/gt/intel_gt_pm.h | 22 ++++-
> drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +-
> drivers/gpu/drm/i915/gt/selftest_engine.c | 28 +++++++
> drivers/gpu/drm/i915/gt/selftest_engine.h | 14 ++++
> drivers/gpu/drm/i915/gt/selftest_engine_pm.c | 83 +++++++++++++++++++
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +
> drivers/gpu/drm/i915/i915_gem.c | 26 +-----
> drivers/gpu/drm/i915/intel_wakeref.c | 82 +++++++++++++-----
> drivers/gpu/drm/i915/intel_wakeref.h | 62 +++++++++-----
> .../drm/i915/selftests/i915_live_selftests.h | 5 +-
> 16 files changed, 301 insertions(+), 131 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gt/selftest_engine.c
> create mode 100644 drivers/gpu/drm/i915/gt/selftest_engine.h
> create mode 100644 drivers/gpu/drm/i915/gt/selftest_engine_pm.c
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index 72922703af49..17e3618241c5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -130,7 +130,9 @@ static bool switch_to_kernel_context_sync(struct intel_gt *gt)
> }
> } while (i915_retire_requests(gt->i915) && result);
>
> - GEM_BUG_ON(gt->awake);
> + if (intel_gt_pm_wait_for_idle(gt))
> + result = false;
> +
> return result;
> }
>
> @@ -161,13 +163,6 @@ void i915_gem_suspend(struct drm_i915_private *i915)
>
> mutex_unlock(&i915->drm.struct_mutex);
>
> - /*
> - * Assert that we successfully flushed all the work and
> - * reset the GPU back to its idle, low power state.
> - */
> - GEM_BUG_ON(i915->gt.awake);
> - flush_work(&i915->gem.idle_work);
> -
> cancel_delayed_work_sync(&i915->gt.hangcheck.work);
>
> i915_gem_drain_freed_objects(i915);
> @@ -244,8 +239,6 @@ void i915_gem_resume(struct drm_i915_private *i915)
> {
> GEM_TRACE("\n");
>
> - WARN_ON(i915->gt.awake);
> -
> mutex_lock(&i915->drm.struct_mutex);
> intel_uncore_forcewake_get(&i915->uncore, FORCEWAKE_ALL);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index d38c114b0964..b57adc973e2a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1568,5 +1568,6 @@ intel_engine_find_active_request(struct intel_engine_cs *engine)
> }
>
> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> +#include "selftest_engine.c"
> #include "selftest_engine_cs.c"
> #endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 0336204988d6..6b15e3335dd6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -37,28 +37,6 @@ static int __engine_unpark(struct intel_wakeref *wf)
> return 0;
> }
>
> -void intel_engine_pm_get(struct intel_engine_cs *engine)
> -{
> - intel_wakeref_get(&engine->i915->runtime_pm, &engine->wakeref, __engine_unpark);
> -}
> -
> -void intel_engine_park(struct intel_engine_cs *engine)
> -{
> - /*
> - * We are committed now to parking this engine, make sure there
> - * will be no more interrupts arriving later and the engine
> - * is truly idle.
> - */
> - if (wait_for(intel_engine_is_idle(engine), 10)) {
> - struct drm_printer p = drm_debug_printer(__func__);
> -
> - dev_err(engine->i915->drm.dev,
> - "%s is not idle before parking\n",
> - engine->name);
> - intel_engine_dump(engine, &p, NULL);
> - }
> -}
> -
> static bool switch_to_kernel_context(struct intel_engine_cs *engine)
> {
> struct i915_request *rq;
> @@ -136,12 +114,18 @@ static int __engine_park(struct intel_wakeref *wf)
> return 0;
> }
>
> -void intel_engine_pm_put(struct intel_engine_cs *engine)
> -{
> - intel_wakeref_put(&engine->i915->runtime_pm, &engine->wakeref, __engine_park);
> -}
> +static const struct intel_wakeref_ops wf_ops = {
> + .get = __engine_unpark,
> + .put = __engine_park,
> +};
>
> void intel_engine_init__pm(struct intel_engine_cs *engine)
> {
> - intel_wakeref_init(&engine->wakeref);
> + struct intel_runtime_pm *rpm = &engine->i915->runtime_pm;
> +
> + intel_wakeref_init(&engine->wakeref, rpm, &wf_ops);
> }
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> +#include "selftest_engine_pm.c"
> +#endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> index 015ac72d7ad0..739c50fefcef 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> @@ -10,24 +10,26 @@
> #include "intel_engine_types.h"
> #include "intel_wakeref.h"
>
> -struct drm_i915_private;
> -
> -void intel_engine_pm_get(struct intel_engine_cs *engine);
> -void intel_engine_pm_put(struct intel_engine_cs *engine);
> -
> static inline bool
> intel_engine_pm_is_awake(const struct intel_engine_cs *engine)
> {
> return intel_wakeref_is_active(&engine->wakeref);
> }
>
> -static inline bool
> -intel_engine_pm_get_if_awake(struct intel_engine_cs *engine)
> +static inline void intel_engine_pm_get(struct intel_engine_cs *engine)
> +{
> + intel_wakeref_get(&engine->wakeref);
> +}
> +
> +static inline bool intel_engine_pm_get_if_awake(struct intel_engine_cs *engine)
> {
> return intel_wakeref_get_if_active(&engine->wakeref);
> }
>
> -void intel_engine_park(struct intel_engine_cs *engine);
> +static inline void intel_engine_pm_put(struct intel_engine_cs *engine)
> +{
> + intel_wakeref_put(&engine->wakeref);
> +}
>
> void intel_engine_init__pm(struct intel_engine_cs *engine);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 6c8970271a7f..1363e069ec83 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -17,7 +17,7 @@ static void pm_notify(struct drm_i915_private *i915, int state)
> blocking_notifier_call_chain(&i915->gt.pm_notifications, state, i915);
> }
>
> -static int intel_gt_unpark(struct intel_wakeref *wf)
> +static int __gt_unpark(struct intel_wakeref *wf)
> {
> struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
> struct drm_i915_private *i915 = gt->i915;
> @@ -53,14 +53,7 @@ static int intel_gt_unpark(struct intel_wakeref *wf)
> return 0;
> }
>
> -void intel_gt_pm_get(struct intel_gt *gt)
> -{
> - struct intel_runtime_pm *rpm = >->i915->runtime_pm;
> -
> - intel_wakeref_get(rpm, >->wakeref, intel_gt_unpark);
> -}
> -
> -static int intel_gt_park(struct intel_wakeref *wf)
> +static int __gt_park(struct intel_wakeref *wf)
> {
> struct drm_i915_private *i915 =
> container_of(wf, typeof(*i915), gt.wakeref);
> @@ -74,22 +67,25 @@ static int intel_gt_park(struct intel_wakeref *wf)
> if (INTEL_GEN(i915) >= 6)
> gen6_rps_idle(i915);
>
> + /* Everything switched off, flush any residual interrupt just in case */
> + intel_synchronize_irq(i915);
Do we detect it gracefully if we get one extra afer this flush?
> +
> GEM_BUG_ON(!wakeref);
> intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ, wakeref);
>
> return 0;
> }
>
> -void intel_gt_pm_put(struct intel_gt *gt)
> -{
> - struct intel_runtime_pm *rpm = >->i915->runtime_pm;
> -
> - intel_wakeref_put(rpm, >->wakeref, intel_gt_park);
> -}
> +static const struct intel_wakeref_ops wf_ops = {
> + .get = __gt_unpark,
> + .put = __gt_park,
> + .flags = INTEL_WAKEREF_PUT_ASYNC,
> +};
>
> void intel_gt_pm_init_early(struct intel_gt *gt)
> {
> - intel_wakeref_init(>->wakeref);
> + intel_wakeref_init(>->wakeref, >->i915->runtime_pm, &wf_ops);
> +
> BLOCKING_INIT_NOTIFIER_HEAD(>->pm_notifications);
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> index e8a18d4b27c9..57633f538ddb 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -17,14 +17,32 @@ enum {
> INTEL_GT_PARK,
> };
>
> -void intel_gt_pm_get(struct intel_gt *gt);
> -void intel_gt_pm_put(struct intel_gt *gt);
> +static inline bool
> +intel_gt_pm_is_awake(const struct intel_gt *gt)
> +{
> + return intel_wakeref_is_active(>->wakeref);
> +}
> +
> +static inline void intel_gt_pm_get(struct intel_gt *gt)
> +{
> + intel_wakeref_get(>->wakeref);
> +}
>
> static inline bool intel_gt_pm_get_if_awake(struct intel_gt *gt)
> {
> return intel_wakeref_get_if_active(>->wakeref);
> }
>
> +static inline void intel_gt_pm_put(struct intel_gt *gt)
> +{
> + intel_wakeref_put(>->wakeref);
> +}
> +
> +static inline int intel_gt_pm_wait_for_idle(struct intel_gt *gt)
> +{
> + return intel_wakeref_wait_for_idle(>->wakeref);
> +}
> +
> void intel_gt_pm_init_early(struct intel_gt *gt);
>
> void intel_gt_sanitize(struct intel_gt *gt, bool force);
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 232f40fcb490..1a0116751d19 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -139,6 +139,7 @@
> #include "i915_vgpu.h"
> #include "intel_engine_pm.h"
> #include "intel_gt.h"
> +#include "intel_gt_pm.h"
> #include "intel_lrc_reg.h"
> #include "intel_mocs.h"
> #include "intel_reset.h"
> @@ -556,6 +557,7 @@ execlists_schedule_in(struct i915_request *rq, int idx)
> intel_context_get(ce);
> ce->inflight = rq->engine;
>
> + intel_gt_pm_get(ce->inflight->gt);
> execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
> intel_engine_context_in(ce->inflight);
> }
> @@ -588,6 +590,7 @@ execlists_schedule_out(struct i915_request *rq)
> if (!intel_context_inflight_count(ce)) {
> intel_engine_context_out(ce->inflight);
> execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
> + intel_gt_pm_put(ce->inflight->gt);
>
> /*
> * If this is part of a virtual engine, its next request may
> @@ -2719,7 +2722,6 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
> static void execlists_park(struct intel_engine_cs *engine)
> {
> del_timer_sync(&engine->execlists.timer);
> - intel_engine_park(engine);
> }
>
> void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine.c b/drivers/gpu/drm/i915/gt/selftest_engine.c
> new file mode 100644
> index 000000000000..f65b118e261d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine.c
> @@ -0,0 +1,28 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright © 2018 Intel Corporation
> + */
> +
> +#include "i915_selftest.h"
> +#include "selftest_engine.h"
> +
> +int intel_engine_live_selftests(struct drm_i915_private *i915)
> +{
> + static int (* const tests[])(struct intel_gt *) = {
> + live_engine_pm_selftests,
> + NULL,
> + };
> + struct intel_gt *gt = &i915->gt;
> + typeof(*tests) *fn;
> +
> + for (fn = tests; *fn; fn++) {
> + int err;
> +
> + err = (*fn)(gt);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine.h b/drivers/gpu/drm/i915/gt/selftest_engine.h
> new file mode 100644
> index 000000000000..ab32d09ec5a1
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine.h
> @@ -0,0 +1,14 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef SELFTEST_ENGINE_H
> +#define SELFTEST_ENGINE_H
> +
> +struct intel_gt;
> +
> +int live_engine_pm_selftests(struct intel_gt *gt);
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_pm.c b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
> new file mode 100644
> index 000000000000..75c197612705
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_pm.c
> @@ -0,0 +1,83 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright © 2018 Intel Corporation
> + */
> +
> +#include "i915_selftest.h"
> +#include "selftest_engine.h"
> +#include "selftests/igt_atomic.h"
> +
> +static int live_engine_pm(void *arg)
> +{
> + struct intel_gt *gt = arg;
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> +
> + /*
> + * Check we can call intel_engine_pm_put from any context. No
> + * failures are reported directly, but if we mess up lockdep should
> + * tell us.
> + */
> + if (intel_gt_pm_wait_for_idle(gt)) {
> + pr_err("Unable to flush GT pm before test\n");
> + return -EBUSY;
> + }
> +
> + GEM_BUG_ON(intel_gt_pm_is_awake(gt));
> + for_each_engine(engine, gt->i915, id) {
> + const typeof(*igt_atomic_phases) *p;
> +
> + for (p = igt_atomic_phases; p->name; p++) {
> + /*
> + * Acquisition is always synchronous, except if we
> + * know that the engine is already awale, in which
s/awale/awake
in which case?
> + * we should use intel_engine_pm_get_if_awake() to
> + * atomically grab the wakeref.
> + *
> + * In practice,
> + * intel_engine_pm_get();
> + * intel_engine_pm_put();
> + * occurs in one thread, while simultaneously
> + * intel_engine_pm_get_if_awake();
> + * intel_engine_pm_put();
> + * occurs in atomic context in another.
> + */
> + GEM_BUG_ON(intel_engine_pm_is_awake(engine));
> + intel_engine_pm_get(engine);
> +
> + p->critical_section_begin();
> + if (!intel_engine_pm_get_if_awake(engine))
> + pr_err("intel_engine_pm_get_if_awake(%s) failed under %s\n",
> + engine->name, p->name);
> + else
> + intel_engine_pm_put(engine);
> + intel_engine_pm_put(engine);
> + p->critical_section_end();
> +
> + /* engine wakeref is sync (instant) */
> + if (intel_engine_pm_is_awake(engine)) {
> + pr_err("%s is still awake after flushing pm\n",
> + engine->name);
> + return -EINVAL;
> + }
> +
> + /* gt wakeref is async (deferred to workqueue) */
> + if (intel_gt_pm_wait_for_idle(gt)) {
> + pr_err("GT failed to idle\n");
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +int live_engine_pm_selftests(struct intel_gt *gt)
> +{
> + static const struct i915_subtest tests[] = {
> + SUBTEST(live_engine_pm),
> + };
> +
> + return intel_gt_live_subtests(tests, gt);
> +}
> 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 a78bd99bc6cb..c9119b90d6aa 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -29,6 +29,7 @@
> #include "gt/intel_context.h"
> #include "gt/intel_engine_pm.h"
> #include "gt/intel_gt.h"
> +#include "gt/intel_gt_pm.h"
> #include "gt/intel_lrc_reg.h"
> #include "intel_guc_submission.h"
>
> @@ -537,6 +538,7 @@ static struct i915_request *schedule_in(struct i915_request *rq, int idx)
> if (!rq->hw_context->inflight)
> rq->hw_context->inflight = rq->engine;
> intel_context_inflight_inc(rq->hw_context);
> + intel_gt_pm_get(rq->engine->gt);
>
> return i915_request_get(rq);
> }
> @@ -549,6 +551,7 @@ static void schedule_out(struct i915_request *rq)
> if (!intel_context_inflight_count(rq->hw_context))
> rq->hw_context->inflight = NULL;
>
> + intel_gt_pm_put(rq->engine->gt);
> i915_request_put(rq);
> }
>
> @@ -1076,7 +1079,6 @@ static void guc_interrupts_release(struct intel_gt *gt)
>
> static void guc_submission_park(struct intel_engine_cs *engine)
> {
> - intel_engine_park(engine);
> intel_engine_unpin_breadcrumbs_irq(engine);
> engine->flags &= ~I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
> }
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 63b599f584db..68a6b002c9ba 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -39,6 +39,7 @@
> #include "display/intel_psr.h"
>
> #include "gem/i915_gem_context.h"
> +#include "gt/intel_gt_pm.h"
> #include "gt/intel_reset.h"
> #include "gt/uc/intel_guc_submission.h"
>
> @@ -3664,6 +3665,9 @@ i915_drop_caches_set(void *data, u64 val)
> i915_retire_requests(i915);
>
> mutex_unlock(&i915->drm.struct_mutex);
> +
> + if (ret == 0 && val & DROP_IDLE)
> + ret = intel_gt_pm_wait_for_idle(&i915->gt);
> }
>
> if (val & DROP_RESET_ACTIVE && intel_gt_terminally_wedged(&i915->gt))
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7efff14b9137..f67197336ee7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -894,19 +894,6 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
> }
> }
>
> -static int wait_for_engines(struct intel_gt *gt)
> -{
> - if (wait_for(intel_engines_are_idle(gt), I915_IDLE_ENGINES_TIMEOUT)) {
> - dev_err(gt->i915->drm.dev,
> - "Failed to idle engines, declaring wedged!\n");
> - GEM_TRACE_DUMP();
> - intel_gt_set_wedged(gt);
> - return -EIO;
> - }
> -
> - return 0;
> -}
> -
> static long
> wait_for_timelines(struct drm_i915_private *i915,
> unsigned int flags, long timeout)
> @@ -954,27 +941,20 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915,
> unsigned int flags, long timeout)
> {
> /* If the device is asleep, we have no requests outstanding */
> - if (!READ_ONCE(i915->gt.awake))
> + if (!intel_gt_pm_is_awake(&i915->gt))
> return 0;
>
> - GEM_TRACE("flags=%x (%s), timeout=%ld%s, awake?=%s\n",
> + GEM_TRACE("flags=%x (%s), timeout=%ld%s\n",
> flags, flags & I915_WAIT_LOCKED ? "locked" : "unlocked",
> - timeout, timeout == MAX_SCHEDULE_TIMEOUT ? " (forever)" : "",
> - yesno(i915->gt.awake));
> + timeout, timeout == MAX_SCHEDULE_TIMEOUT ? " (forever)" : "");
>
> timeout = wait_for_timelines(i915, flags, timeout);
> if (timeout < 0)
> return timeout;
>
> if (flags & I915_WAIT_LOCKED) {
> - int err;
> -
> lockdep_assert_held(&i915->drm.struct_mutex);
>
> - err = wait_for_engines(&i915->gt);
> - if (err)
> - return err;
> -
Hmm where does the implicit wait for idle come from now?
> i915_retire_requests(i915);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
> index 06bd8b215cc2..d4443e81c1c8 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.c
> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> @@ -4,25 +4,25 @@
> * Copyright © 2019 Intel Corporation
> */
>
> +#include <linux/wait_bit.h>
> +
> #include "intel_runtime_pm.h"
> #include "intel_wakeref.h"
>
> -static void rpm_get(struct intel_runtime_pm *rpm, struct intel_wakeref *wf)
> +static void rpm_get(struct intel_wakeref *wf)
> {
> - wf->wakeref = intel_runtime_pm_get(rpm);
> + wf->wakeref = intel_runtime_pm_get(wf->rpm);
> }
>
> -static void rpm_put(struct intel_runtime_pm *rpm, struct intel_wakeref *wf)
> +static void rpm_put(struct intel_wakeref *wf)
> {
> intel_wakeref_t wakeref = fetch_and_zero(&wf->wakeref);
>
> - intel_runtime_pm_put(rpm, wakeref);
> + intel_runtime_pm_put(wf->rpm, wakeref);
> INTEL_WAKEREF_BUG_ON(!wakeref);
> }
>
> -int __intel_wakeref_get_first(struct intel_runtime_pm *rpm,
> - struct intel_wakeref *wf,
> - int (*fn)(struct intel_wakeref *wf))
> +int __intel_wakeref_get_first(struct intel_wakeref *wf)
> {
> /*
> * Treat get/put as different subclasses, as we may need to run
> @@ -34,11 +34,11 @@ int __intel_wakeref_get_first(struct intel_runtime_pm *rpm,
> if (!atomic_read(&wf->count)) {
> int err;
>
> - rpm_get(rpm, wf);
> + rpm_get(wf);
>
> - err = fn(wf);
> + err = wf->ops->get(wf);
> if (unlikely(err)) {
> - rpm_put(rpm, wf);
> + rpm_put(wf);
> mutex_unlock(&wf->mutex);
> return err;
> }
> @@ -52,27 +52,67 @@ int __intel_wakeref_get_first(struct intel_runtime_pm *rpm,
> return 0;
> }
>
> -int __intel_wakeref_put_last(struct intel_runtime_pm *rpm,
> - struct intel_wakeref *wf,
> - int (*fn)(struct intel_wakeref *wf))
> +static void ____intel_wakeref_put_last(struct intel_wakeref *wf)
> {
> - int err;
> + if (!atomic_dec_and_test(&wf->count))
> + goto unlock;
> +
> + if (likely(!wf->ops->put(wf))) {
> + rpm_put(wf);
> + wake_up_var(&wf->wakeref);
> + } else {
> + /* ops->put() must schedule its own release on deferral */
> + atomic_set_release(&wf->count, 1);
I lose track here. On async call to gt_park we end up leaving
with a count 1.
> + }
>
> - err = fn(wf);
> - if (likely(!err))
> - rpm_put(rpm, wf);
> - else
> - atomic_inc(&wf->count);
> +unlock:
> mutex_unlock(&wf->mutex);
> +}
> +
> +void __intel_wakeref_put_last(struct intel_wakeref *wf)
> +{
> + INTEL_WAKEREF_BUG_ON(work_pending(&wf->work));
>
> - return err;
> + /* Assume we are not in process context and so cannot sleep. */
> + if (wf->ops->flags & INTEL_WAKEREF_PUT_ASYNC ||
> + !mutex_trylock(&wf->mutex)) {
Didn't found anything in trylock that would prevent you
from shooting you on your own leg.
So it is up to caller (and eventually lockdep spam) if
the async usage fails to follow the rules?
> + schedule_work(&wf->work);
> + return;
> + }
> +
> + ____intel_wakeref_put_last(wf);
> }
>
> -void __intel_wakeref_init(struct intel_wakeref *wf, struct lock_class_key *key)
> +static void __intel_wakeref_put_work(struct work_struct *wrk)
No need for underscore before the name here?
-Mika
> {
> + struct intel_wakeref *wf = container_of(wrk, typeof(*wf), work);
> +
> + if (atomic_add_unless(&wf->count, -1, 1))
> + return;
> +
> + mutex_lock(&wf->mutex);
> + ____intel_wakeref_put_last(wf);
> +}
> +
> +void __intel_wakeref_init(struct intel_wakeref *wf,
> + struct intel_runtime_pm *rpm,
> + const struct intel_wakeref_ops *ops,
> + struct lock_class_key *key)
> +{
> + wf->rpm = rpm;
> + wf->ops = ops;
> +
> __mutex_init(&wf->mutex, "wakeref", key);
> atomic_set(&wf->count, 0);
> wf->wakeref = 0;
> +
> + INIT_WORK(&wf->work, __intel_wakeref_put_work);
> +}
> +
> +int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
> +{
> + return wait_var_event_killable(&wf->wakeref,
> + !intel_wakeref_is_active(wf));
> }
>
> static void wakeref_auto_timeout(struct timer_list *t)
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.h b/drivers/gpu/drm/i915/intel_wakeref.h
> index 1d6f5986e4e5..535a3a12864b 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.h
> +++ b/drivers/gpu/drm/i915/intel_wakeref.h
> @@ -8,10 +8,12 @@
> #define INTEL_WAKEREF_H
>
> #include <linux/atomic.h>
> +#include <linux/bits.h>
> #include <linux/mutex.h>
> #include <linux/refcount.h>
> #include <linux/stackdepot.h>
> #include <linux/timer.h>
> +#include <linux/workqueue.h>
>
> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
> #define INTEL_WAKEREF_BUG_ON(expr) BUG_ON(expr)
> @@ -20,29 +22,42 @@
> #endif
>
> struct intel_runtime_pm;
> +struct intel_wakeref;
>
> typedef depot_stack_handle_t intel_wakeref_t;
>
> +struct intel_wakeref_ops {
> + int (*get)(struct intel_wakeref *wf);
> + int (*put)(struct intel_wakeref *wf);
> +
> + unsigned long flags;
> +#define INTEL_WAKEREF_PUT_ASYNC BIT(0)
> +};
> +
> struct intel_wakeref {
> atomic_t count;
> struct mutex mutex;
> +
> intel_wakeref_t wakeref;
> +
> + struct intel_runtime_pm *rpm;
> + const struct intel_wakeref_ops *ops;
> +
> + struct work_struct work;
> };
>
> void __intel_wakeref_init(struct intel_wakeref *wf,
> + struct intel_runtime_pm *rpm,
> + const struct intel_wakeref_ops *ops,
> struct lock_class_key *key);
> -#define intel_wakeref_init(wf) do { \
> +#define intel_wakeref_init(wf, rpm, ops) do { \
> static struct lock_class_key __key; \
> \
> - __intel_wakeref_init((wf), &__key); \
> + __intel_wakeref_init((wf), (rpm), (ops), &__key); \
> } while (0)
>
> -int __intel_wakeref_get_first(struct intel_runtime_pm *rpm,
> - struct intel_wakeref *wf,
> - int (*fn)(struct intel_wakeref *wf));
> -int __intel_wakeref_put_last(struct intel_runtime_pm *rpm,
> - struct intel_wakeref *wf,
> - int (*fn)(struct intel_wakeref *wf));
> +int __intel_wakeref_get_first(struct intel_wakeref *wf);
> +void __intel_wakeref_put_last(struct intel_wakeref *wf);
>
> /**
> * intel_wakeref_get: Acquire the wakeref
> @@ -61,12 +76,10 @@ int __intel_wakeref_put_last(struct intel_runtime_pm *rpm,
> * code otherwise.
> */
> static inline int
> -intel_wakeref_get(struct intel_runtime_pm *rpm,
> - struct intel_wakeref *wf,
> - int (*fn)(struct intel_wakeref *wf))
> +intel_wakeref_get(struct intel_wakeref *wf)
> {
> if (unlikely(!atomic_inc_not_zero(&wf->count)))
> - return __intel_wakeref_get_first(rpm, wf, fn);
> + return __intel_wakeref_get_first(wf);
>
> return 0;
> }
> @@ -102,16 +115,12 @@ intel_wakeref_get_if_active(struct intel_wakeref *wf)
> * Returns: 0 if the wakeref was released successfully, or a negative error
> * code otherwise.
> */
> -static inline int
> -intel_wakeref_put(struct intel_runtime_pm *rpm,
> - struct intel_wakeref *wf,
> - int (*fn)(struct intel_wakeref *wf))
> +static inline void
> +intel_wakeref_put(struct intel_wakeref *wf)
> {
> INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
> - if (atomic_dec_and_mutex_lock(&wf->count, &wf->mutex))
> - return __intel_wakeref_put_last(rpm, wf, fn);
> -
> - return 0;
> + if (unlikely(!atomic_add_unless(&wf->count, -1, 1)))
> + __intel_wakeref_put_last(wf);
> }
>
> /**
> @@ -154,6 +163,19 @@ intel_wakeref_is_active(const struct intel_wakeref *wf)
> return READ_ONCE(wf->wakeref);
> }
>
> +/**
> + * intel_wakeref_wait_for_idle: Wait until the wakeref is idle
> + * @wf: the wakeref
> + *
> + * Wait for the earlier asynchronous release of the wakeref. Note
> + * this will wait for any third party as well, so make sure you only wait
> + * when you have control over the wakeref and trust no one else is acquiring
> + * it.
> + *
> + * Return: 0 on success, error code if killed.
> + */
> +int intel_wakeref_wait_for_idle(struct intel_wakeref *wf);
> +
> struct intel_wakeref_auto {
> struct intel_runtime_pm *rpm;
> struct timer_list timer;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> index a841d3f9bedc..1ccf0f731ac0 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> @@ -12,10 +12,11 @@
> selftest(sanitycheck, i915_live_sanitycheck) /* keep first (igt selfcheck) */
> selftest(uncore, intel_uncore_live_selftests)
> selftest(workarounds, intel_workarounds_live_selftests)
> -selftest(timelines, intel_timeline_live_selftests)
> +selftest(gt_engines, intel_engine_live_selftests)
> +selftest(gt_timelines, intel_timeline_live_selftests)
> +selftest(gt_contexts, intel_context_live_selftests)
> selftest(requests, i915_request_live_selftests)
> selftest(active, i915_active_live_selftests)
> -selftest(gt_contexts, intel_context_live_selftests)
> selftest(objects, i915_gem_object_live_selftests)
> selftest(mman, i915_gem_mman_live_selftests)
> selftest(dmabuf, i915_gem_dmabuf_live_selftests)
> --
> 2.23.0.rc1
More information about the Intel-gfx
mailing list