Quoting Jani Nikula (2019-01-03 09:38:56)
> On Wed, 02 Jan 2019, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> > The majority of runtime-pm operations are bounded and scoped within a
> > function; these are easy to verify that the wakeref are handled
> > correctly. We can employ the compiler to help us, and reduce the number
> > of wakerefs tracked when debugging, by passing around cookies provided
> > by the various rpm_get functions to their rpm_put counterpart. This
> > makes the pairing explicit, and given the required wakeref cookie the
> > compiler can verify that we pass an initialised value to the rpm_put
> > (quite handy for double checking error paths).
>
> What a monster patch! :o Can't say I reviewed it all, but ack on the
> approach.
>
> The series could use a cover letter... seems like this should be chopped
> up to smaller series perhaps. Or too many conflicts that way?
It had a cover letter about June when it was sent separately, since then
I've depended upon the bug fixes.
> Some minor nits inline.
>
> BR,
> Jani.
>
> >
> > For regular builds, the compiler should be able to eliminate the unused
> > local variables and the program growth should be minimal. Fwiw, it came
> > out as a net improvement as gcc was able to refactor rpm_get and
> > rpm_get_if_in_use together,
> >
> > add/remove: 1/1 grow/shrink: 20/9 up/down: 191/-268 (-77)
> > Function old new delta
> > intel_runtime_pm_put_unchecked - 136 +136
> > i915_gem_unpark 396 406 +10
> > intel_runtime_pm_get 135 141 +6
> > intel_runtime_pm_get_noresume 136 141 +5
> > i915_perf_open_ioctl 4375 4379 +4
> > i915_gpu_busy 72 76 +4
> > i915_gem_idle_work_handler 954 958 +4
> > capture 6814 6818 +4
> > mock_gem_device 1433 1436 +3
> > __execlists_submission_tasklet 2573 2576 +3
> > i915_sample 756 758 +2
> > intel_guc_submission_disable 364 365 +1
> > igt_mmap_offset_exhaustion 1035 1036 +1
> > i915_runtime_pm_status 257 258 +1
> > i915_rps_boost_info 1358 1359 +1
> > i915_hangcheck_info 1229 1230 +1
> > i915_gem_switch_to_kernel_context 682 683 +1
> > i915_gem_suspend 410 411 +1
> > i915_gem_resume 254 255 +1
> > i915_gem_park 190 191 +1
> > i915_engine_info 279 280 +1
> > intel_rps_mark_interactive 194 193 -1
> > i915_hangcheck_elapsed 1526 1525 -1
> > i915_gem_wait_for_idle 298 297 -1
> > i915_drop_caches_set 555 554 -1
> > execlists_submission_tasklet 126 125 -1
> > aliasing_gtt_bind_vma 235 234 -1
> > i915_gem_retire_work_handler 144 142 -2
> > igt_evict_contexts.part 916 910 -6
> > intel_runtime_pm_get_if_in_use 141 23 -118
> > intel_runtime_pm_put 136 - -136
>
>
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0ebde13620cb..41d253e8c09e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -139,6 +139,8 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
> >
> > static u32 __i915_gem_park(struct drm_i915_private *i915)
> > {
> > + intel_wakeref_t wakeref;
> > +
> > GEM_TRACE("\n");
> >
> > lockdep_assert_held(&i915->drm.struct_mutex);
> > @@ -169,14 +171,15 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
> > i915_pmu_gt_parked(i915);
> > i915_vma_parked(i915);
> >
> > - i915->gt.awake = false;
> > + wakeref = fetch_and_zero(&i915->gt.awake);
> > + GEM_BUG_ON(!wakeref);
>
> Mmh, I wonder if this should warrant a separate patch.
>
> >
> > if (INTEL_GEN(i915) >= 6)
> > gen6_rps_idle(i915);
> >
> > intel_display_power_put(i915, POWER_DOMAIN_GT_IRQ);
> >
> > - intel_runtime_pm_put(i915);
> > + intel_runtime_pm_put(i915, wakeref);
> >
> > return i915->gt.epoch;
> > }
> > @@ -205,7 +208,8 @@ void i915_gem_unpark(struct drm_i915_private *i915)
> > if (i915->gt.awake)
> > return;
> >
> > - intel_runtime_pm_get_noresume(i915);
> > + i915->gt.awake = intel_runtime_pm_get_noresume(i915);
> > + GEM_BUG_ON(!i915->gt.awake);
>
> Ditto.
I thought this was a simple transformation, and the tracking is a
fundamental part of this patch. There's a later patch to realise we do
the same thing twice and turn it into one.
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 4288c0e02f0c..44566dc2f9cc 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -1364,14 +1364,14 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
> >
> > free_oa_buffer(dev_priv);
> >
> > + put_oa_config(dev_priv, stream->oa_config);
> > +
> > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > - intel_runtime_pm_put(dev_priv);
> > + intel_runtime_pm_put(dev_priv, stream->wakeref);
> >
> > if (stream->ctx)
> > oa_put_render_ctx_id(stream);
> >
> > - put_oa_config(dev_priv, stream->oa_config);
> > -
>
> Can we extract this ordering change to a separate patch?
Hmm, doesn't look justified. I hope it was a rebase undoing a later
patch.
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index ac513fd70315..a1e4e1033289 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -29,6 +29,7 @@
> > #include <linux/i2c.h>
> > #include <linux/hdmi.h>
> > #include <linux/sched/clock.h>
> > +#include <linux/stackdepot.h>
> > #include <drm/i915_drm.h>
> > #include "i915_drv.h"
> > #include <drm/drm_crtc.h>
> > @@ -2182,10 +2183,16 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *i915)
> > atomic_dec(&i915->runtime_pm.wakeref_count);
> > }
> >
> > -void intel_runtime_pm_get(struct drm_i915_private *i915);
> > -bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915);
> > -void intel_runtime_pm_get_noresume(struct drm_i915_private *i915);
> > -void intel_runtime_pm_put(struct drm_i915_private *i915);
> > +intel_wakeref_t intel_runtime_pm_get(struct drm_i915_private *i915);
> > +intel_wakeref_t intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915);
> > +intel_wakeref_t intel_runtime_pm_get_noresume(struct drm_i915_private *i915);
>
> __must_check would be an interesting annotation for these. It would help
> catch the blunders with DEBUG_RUNTIME_PM=n by ensuring you assign the
> return value somewhere. It's just that not assigning is valid for the
> put_unchecked cases. *shrug*
Yeah, I dare say once we've broken the back of _unchecked and track
wakerefs everywhere (gvt and atomic modesetting are the hold outs), then
it should be must_check.
> > +void intel_runtime_pm_put_unchecked(struct drm_i915_private *i915);
> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > +void intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref);
> > +#else
> > +#define intel_runtime_pm_put(i915, wref) intel_runtime_pm_put_unchecked(i915)
> > +#endif
>
> Normally I'd probably like to see an actual function here, just to get
> the wref type checked, but I presume this helps the compiler toss away
> the local variable.
Right.
> > @@ -94,8 +94,55 @@ track_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
> > if (stacks) {
> > stacks[rpm->debug_count++] = stack;
> > rpm->debug_owners = stacks;
> > + } else {
> > + stack = -1;
> > }
> > spin_unlock_irqrestore(&rpm->debug_lock, flags);
> > +
> > + return stack;
> > +}
> > +
> > +static void cancel_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
> > + depot_stack_handle_t stack)
> > +{
> > + struct i915_runtime_pm *rpm = &i915->runtime_pm;
> > + unsigned long flags, n;
> > + bool found = false;
> > +
> > + if (unlikely(stack == -1))
> > + return;
>
> Unlikely of the should not happen magnitude? WARN_ON?
It's following an allocation error, so not improbable but still expected
at some point. But -1 is improbable for any other reason.
> > @@ -379,6 +383,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> > struct igt_spinner spin;
> > enum intel_engine_id id;
> > struct i915_request *rq;
> > + intel_wakeref_t wakeref;
> > int ret = 0;
> >
> > if (!intel_has_reset_engine(i915))
> > @@ -389,7 +394,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> > return PTR_ERR(ctx);
> >
> > igt_global_reset_lock(i915);
> > - intel_runtime_pm_get(i915);
> > + wakeref = intel_runtime_pm_get(i915);
> >
> > for_each_engine(engine, i915, id) {
> > bool ok;
> > @@ -417,6 +422,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> > rq = igt_spinner_create_request(&spin, ctx, engine, MI_NOOP);
> > if (IS_ERR(rq)) {
> > ret = PTR_ERR(rq);
> > + intel_runtime_pm_put(i915, wakeref);
>
> This looks suspect.
>
> > igt_spinner_fini(&spin);
> > goto err;
> > }
> > @@ -425,6 +431,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> >
> > if (!igt_wait_for_spinner(&spin, rq)) {
> > pr_err("Spinner failed to start\n");
> > + intel_runtime_pm_put(i915, wakeref);
>
> Ditto.
Ta.
-Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
