Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee
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
> > Signed-off-by: Imre Deak
> > ---
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee
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
> Signed-off-by: Imre Deak
> ---
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee
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
Signed-off-by: Imre Deak
---
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 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 inline int
+intel_rpm_raw_wakeref_count(int wakeref_count)
+{
+ return wakeref_count & INTEL_RPM_RAW_WAKEREF_MASK;
+}
+
+static inline int
+intel_rpm_wakelock_count(int wakeref_count)
+{
+ return wakeref_count >> INTEL_RPM_WAKELOCK_SHIFT;
+}
+
static inline void
assert_rpm_device_not_suspended(struct i915_runtime_pm *rpm)
{
@@ -1627,11 +1645,33 @@ assert_rpm_device_not_suspended(struct i915_runtime_pm
*rpm)
}
static inline void
-__assert_rpm_wakelock_held(struct i915_runtime_pm *rpm)
+assert_rpm_raw_wakeref_held(struct i915_runtime_pm *rpm, int wakeref_count)
{
assert_rpm_device_not_suspended(rpm);
- WARN_ONCE(!atomic_read(&rpm->wakeref_count),
- "RPM wakelock ref not held during HW access");
+ WARN_ONCE(!intel_rpm_raw_wakeref_count(wakeref_count),
+ "RPM raw-wakeref not held\n");
+}
+
+static inline void
+assert_rpm_wakelock_held(struct i915_runtime_pm *rpm, int wakeref_count)
+{
+ assert_rpm_raw_wakeref_held(rpm, wakeref_count);
+ WARN_ONCE(!intel_rpm_wakelock_count(wakeref_count),
+ "RPM wakelock ref not held during HW access\n");
+}
+
+static inline void
+assert_rpm_raw_wakeref_held(struct drm_i915_private *i915)
+{
+ struct i915_runtime_pm *rpm = &i915->runtime_pm;
+
+ assert_rpm_raw_wakeref_held(rpm, atomic_read(&rpm->wakeref_count));
+}
+
+static inline void
+__assert_rpm_wakelock_held(struct i915_runtime_pm *rpm)
+{
+ assert_rpm_wakelock_held(rpm, atomic_read(&rpm->wakeref_count));
}
static inline void
@@ -1661,7 +1701,8 @@ assert_rpm_wakelock_held(struct drm_i915_private *i915)
static inline void
disable_rpm_wakeref_asserts(struct drm_i915_private *i915)
{
- atomic_inc(&i915->runtime_pm.wakeref_count);
+ atomic_add(INTEL_RPM_WAKELOCK_BIAS + 1,
+ &i915->runtime_pm.wakeref_count);
}
/**
@@ -1678,7 +1719,8 @@ disable_rpm_wakeref_asserts(struct drm_i915_private *i915)
static inline void
enable_rpm_wakeref_asserts(struct drm_i915_private *i915)
{
- atomic_dec(&i915->runtime_pm.wakeref_count);
+ atomic_sub(INTEL_RPM_WAKELOCK_BIAS + 1,
+ &i915->runtime_pm.wakeref_count);
}
#endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b8fadd1b685c..84a18d8b942c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -110,9 +110,6 @@ track_intel_runtime_pm_wakeref(struct drm_i915_private
*i915)
depot_stack_handle_t stack, *stacks;
unsigned long flags;
- atomic_inc(&rpm->wakeref_count);
- assert_rpm_wakelock_held(i915);
-
if (!HAS_RUNTIME_PM(i915))
return -1;
@@ -140,7 +137,7 @@ track_intel_runtime_pm_wakeref(struct drm_i915_private
*i915)
return stack;
}
-static void cancel_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
+static void untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
depot_stack_handle_t stac
