Re: [PATCH 09/39] drm/i915: Markup paired operations on wakerefs

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 02 Jan 2019, Chris Wilson <chris@chris-wilson.co.uk> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx