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

Submitted by Imre Deak on May 9, 2019, 6:19 a.m.

Details

Message ID 20190509061954.10379-2-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Add support for asynchronous display power disabling" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak May 9, 2019, 6:19 a.m.
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(-)

Patch hide | download patch | download mbox

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 stack)
 {
 	struct i915_runtime_pm *rpm = &i915->runtime_pm;
@@ -236,14 +233,13 @@  __print_intel_runtime_pm_wakeref(struct drm_printer *p,
 }
 
 static noinline void
-untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+__intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
 {
 	struct i915_runtime_pm *rpm = &i915->runtime_pm;
 	struct intel_runtime_pm_debug dbg = {};
 	struct drm_printer p;
 	unsigned long flags;
 
-	assert_rpm_wakelock_held(i915);
 	if (atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
 					&rpm->debug.lock,
 					flags)) {
@@ -311,19 +307,51 @@  static void init_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
 static depot_stack_handle_t
 track_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
 {
-	atomic_inc(&i915->runtime_pm.wakeref_count);
-	assert_rpm_wakelock_held(i915);
 	return -1;
 }
 
-static void untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915)
+static void untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
+					     intel_wakeref_t wref)
+{
+}
+
+static void
+__intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
 {
-	assert_rpm_wakelock_held(i915);
 	atomic_dec(&i915->runtime_pm.wakeref_count);
 }
 
 #endif
 
+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);
+}
+
 bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
 					 enum i915_power_well_id power_well_id);
 
@@ -1946,13 +1974,17 @@  static void __intel_display_power_put(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_display_power_put - release a power domain reference
+ * intel_display_power_put_unchecked - release an unchecked power domain reference
  * @dev_priv: i915 device instance
  * @domain: power domain to reference
  *
  * This function drops the power domain reference obtained by
  * intel_display_power_get() and might power down the corresponding hardware
  * block right away if this is the last reference.
+ *
+ * This function exists only for historical reasons and should be avoided in
+ * new code, as the correctness of its use cannot be checked. Always use
+ * intel_display_power_put() instead.
  */
 void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
 				       enum intel_display_power_domain domain)
@@ -1962,6 +1994,16 @@  void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+/**
+ * intel_display_power_put - release a power domain reference
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ * @wakeref: wakeref acquired for the reference that is being released
+ *
+ * This function drops the power domain reference obtained by
+ * intel_display_power_get() and might power down the corresponding hardware
+ * block right away if this is the last reference.
+ */
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain,
 			     intel_wakeref_t wakeref)
@@ -4579,19 +4621,8 @@  static void intel_power_domains_verify_state(struct drm_i915_private *i915)
 
 #endif
 
-/**
- * intel_runtime_pm_get - grab a runtime pm reference
- * @i915: i915 device instance
- *
- * This function grabs a device-level runtime pm reference (mostly used for GEM
- * code to ensure the GTT or GT is on) and ensures that it is powered up.
- *
- * Any runtime pm reference obtained by this function must have a symmetric
- * call to intel_runtime_pm_put() to release the reference again.
- *
- * Returns: the wakeref cookie to pass to intel_runtime_pm_put()
- */
-intel_wakeref_t intel_runtime_pm_get(struct drm_i915_private *i915)
+static intel_wakeref_t __intel_runtime_pm_get(struct drm_i915_private *i915,
+					      bool wakelock)
 {
 	struct pci_dev *pdev = i915->drm.pdev;
 	struct device *kdev = &pdev->dev;
@@ -4600,9 +4631,28 @@  intel_wakeref_t intel_runtime_pm_get(struct drm_i915_private *i915)
 	ret = pm_runtime_get_sync(kdev);
 	WARN_ONCE(ret < 0, "pm_runtime_get_sync() failed: %d\n", ret);
 
+	intel_runtime_pm_acquire(i915, wakelock);
+
 	return track_intel_runtime_pm_wakeref(i915);
 }
 
+/**
+ * intel_runtime_pm_get - grab a runtime pm reference
+ * @i915: i915 device instance
+ *
+ * This function grabs a device-level runtime pm reference (mostly used for GEM
+ * code to ensure the GTT or GT is on) and ensures that it is powered up.
+ *
+ * Any runtime pm reference obtained by this function must have a symmetric
+ * call to intel_runtime_pm_put() to release the reference again.
+ *
+ * Returns: the wakeref cookie to pass to intel_runtime_pm_put()
+ */
+intel_wakeref_t intel_runtime_pm_get(struct drm_i915_private *i915)
+{
+	return __intel_runtime_pm_get(i915, true);
+}
+
 /**
  * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in use
  * @i915: i915 device instance
@@ -4633,6 +4683,8 @@  intel_wakeref_t intel_runtime_pm_get_if_in_use(struct drm_i915_private *i915)
 			return 0;
 	}
 
+	intel_runtime_pm_acquire(i915, true);
+
 	return track_intel_runtime_pm_wakeref(i915);
 }
 
@@ -4663,33 +4715,56 @@  intel_wakeref_t intel_runtime_pm_get_noresume(struct drm_i915_private *i915)
 	assert_rpm_wakelock_held(i915);
 	pm_runtime_get_noresume(kdev);
 
+	intel_runtime_pm_acquire(i915, true);
+
 	return track_intel_runtime_pm_wakeref(i915);
 }
 
+static void __intel_runtime_pm_put(struct drm_i915_private *i915,
+				   intel_wakeref_t wref,
+				   bool wakelock)
+{
+	struct pci_dev *pdev = i915->drm.pdev;
+	struct device *kdev = &pdev->dev;
+
+	untrack_intel_runtime_pm_wakeref(i915, wref);
+
+	intel_runtime_pm_release(i915, wakelock);
+
+	pm_runtime_mark_last_busy(kdev);
+	pm_runtime_put_autosuspend(kdev);
+}
+
 /**
- * intel_runtime_pm_put - release a runtime pm reference
+ * intel_runtime_pm_put_unchecked - release an unchecked runtime pm reference
  * @i915: i915 device instance
  *
  * This function drops the device-level runtime pm reference obtained by
  * intel_runtime_pm_get() and might power down the corresponding
  * hardware block right away if this is the last reference.
+ *
+ * This function exists only for historical reasons and should be avoided in
+ * new code, as the correctness of its use cannot be checked. Always use
+ * intel_runtime_pm_put() instead.
  */
 void intel_runtime_pm_put_unchecked(struct drm_i915_private *i915)
 {
-	struct pci_dev *pdev = i915->drm.pdev;
-	struct device *kdev = &pdev->dev;
-
-	untrack_intel_runtime_pm_wakeref(i915);
-
-	pm_runtime_mark_last_busy(kdev);
-	pm_runtime_put_autosuspend(kdev);
+	__intel_runtime_pm_put(i915, -1, true);
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+/**
+ * intel_runtime_pm_put - release a runtime pm reference
+ * @i915: i915 device instance
+ * @wref: wakeref acquired for the reference that is being released
+ *
+ * This function drops the device-level runtime pm reference obtained by
+ * intel_runtime_pm_get() and might power down the corresponding
+ * hardware block right away if this is the last reference.
+ */
 void intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref)
 {
-	cancel_intel_runtime_pm_wakeref(i915, wref);
-	intel_runtime_pm_put_unchecked(i915);
+	__intel_runtime_pm_put(i915, wref, true);
 }
 #endif
 
@@ -4767,10 +4842,11 @@  void intel_runtime_pm_cleanup(struct drm_i915_private *i915)
 
 	count = atomic_fetch_inc(&rpm->wakeref_count); /* balance untrack */
 	WARN(count,
-	     "i915->runtime_pm.wakeref_count=%d on cleanup\n",
-	     count);
+	     "i915 raw-wakerefs=%d wakelocks=%d on cleanup\n",
+	     intel_rpm_raw_wakeref_count(count),
+	     intel_rpm_wakelock_count(count));
 
-	untrack_intel_runtime_pm_wakeref(i915);
+	intel_runtime_pm_release(i915, false);
 }
 
 void intel_runtime_pm_init_early(struct drm_i915_private *i915)

Comments

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
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@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.

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@chris-wilson.co.uk>
> -Chris