Re: [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee

Chris Wilson
2019-05-09T08:03:09Z

Quoting Imre Deak (2019-05-09 07:19:44)
> It's useful to track runtime PM refs that don't guarantee a device
> power-on state to the rest of the driver. One such case is holding a
> reference that will be put asynchronously, during which normal users
> without their own reference shouldn't access the HW. A follow-up patch
> will add support for disabling display power domains asynchronously
> which needs this.
>
> For this we can split wakeref_count into a low half-word tracking
> all references (raw-wakerefs) and a high half-word tracking
> references guaranteeing a power-on state (wakelocks).
>
> Follow-up patches will make use of the API added here.
>
> While at it add the missing docbook header for the unchecked
> display-power and runtime_pm put functions.
>
> No functional changes, except for printing leaked raw-wakerefs
> and wakelocks separately in intel_runtime_pm_cleanup().
>
> v2:
> - Track raw wakerefs/wakelocks in the low/high half-word of
> wakeref_count, instead of adding a new counter. (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 52 +++++++-
> drivers/gpu/drm/i915/intel_runtime_pm.c | 150 ++++++++++++++++++------
> 2 files changed, 160 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 247893ed1543..772ed0fedb39 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1619,6 +1619,24 @@ unsigned int i9xx_plane_max_stride(struct intel_plane *plane,
> unsigned int rotation);
>
> /* intel_runtime_pm.c */

#define struct_member(T, member) (((T *)0)->member)

and stick that in i915_utils.h. There's a few repetitions in
include/linux so maybe worth centralising.

> +#define BITS_PER_WAKEREF \
> + BITS_PER_TYPE(((struct i915_runtime_pm *)NULL)->wakeref_count)
> +#define INTEL_RPM_WAKELOCK_SHIFT (BITS_PER_WAKEREF / 2)
> +#define INTEL_RPM_WAKELOCK_BIAS (1 << INTEL_RPM_WAKELOCK_SHIFT)
> +#define INTEL_RPM_RAW_WAKEREF_MASK (INTEL_RPM_WAKELOCK_BIAS - 1)

> +static void
> +intel_runtime_pm_acquire(struct drm_i915_private *i915, bool wakelock)
> +{
> + struct i915_runtime_pm *rpm = &i915->runtime_pm;
> +
> + if (wakelock) {
> + atomic_add(1 + INTEL_RPM_WAKELOCK_BIAS, &rpm->wakeref_count);
> + assert_rpm_wakelock_held(i915);
> + } else {
> + atomic_inc(&rpm->wakeref_count);
> + assert_rpm_raw_wakeref_held(i915);
> + }
> +}
> +
> +static void
> +intel_runtime_pm_release(struct drm_i915_private *i915, int wakelock)
> +{
> + struct i915_runtime_pm *rpm = &i915->runtime_pm;
> +
> + if (wakelock) {
> + assert_rpm_wakelock_held(i915);
> + atomic_sub(INTEL_RPM_WAKELOCK_BIAS, &rpm->wakeref_count);
> + } else {
> + assert_rpm_raw_wakeref_held(i915);
> + }
> +
> + __intel_wakeref_dec_and_check_tracking(i915);

Creating a atomic_sub_and_lock_irqsave() would restore the onion?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris