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?
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.
> 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?
> 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*
> +
> +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.
> @@ -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?
> @@ -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.
> igt_spinner_fini(&spin);
> ret = -ETIMEDOUT;
> goto err;
> @@ -443,7 +450,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> }
>
> err:
> - intel_runtime_pm_put(i915);
> + intel_runtime_pm_put(i915, wakeref);
> igt_global_reset_unlock(i915);
> kernel_context_close(ctx);
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
