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

[Imre Deak]

On Thu, May 09, 2019 at 09:03:04AM +0100, Chris Wilson wrote:
> 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 at chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak at 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.

Ok.

> There's a few repetitions in include/linux so maybe worth
> centralising.

For reference, the corresponding spatch, found a few tens of uses across
the tree (not sure if 'identifier I;' is the best way to denote a struct
member):

@nc@
type T;
identifier I;
@@

- ((T *)NULL)->I
+ struct_member(T, I)

> 
> > +#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?

Yep. One day I may go through the architecture maze/header magic I
suspect that involves, or if someone could add it I'd be happy to use
that instead.

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