* [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 8:03 ` Chris Wilson
2019-05-09 6:19 ` [PATCH v2 02/11] drm/i915: Force printing wakeref tacking during pm_cleanup Imre Deak
` (13 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
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 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)
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee
2019-05-09 6:19 ` [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee Imre Deak
@ 2019-05-09 8:03 ` Chris Wilson
2019-05-09 9:58 ` Imre Deak
0 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2019-05-09 8:03 UTC (permalink / raw)
To: Imre Deak, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee
2019-05-09 8:03 ` Chris Wilson
@ 2019-05-09 9:58 ` Imre Deak
0 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 9:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 02/11] drm/i915: Force printing wakeref tacking during pm_cleanup
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
2019-05-09 6:19 ` [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 8:05 ` Chris Wilson
2019-05-09 6:19 ` [PATCH v2 03/11] drm/i915: Verify power domains state during suspend in all cases Imre Deak
` (12 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
Make sure we print and drop the wakeref tracking info during pm_cleanup
even if there are wakeref holders (either raw-wakeref or wakelock
holders). Dropping the wakeref tracking means that a late put on the ref
will WARN since the wakeref will be unknown, but that is rightly so,
since the put is late and we want to catch that case.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 75 ++++++++++++++++++-------
1 file changed, 54 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 84a18d8b942c..dc964c8608f1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -233,31 +233,60 @@ __print_intel_runtime_pm_wakeref(struct drm_printer *p,
}
static noinline void
-__intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
+__untrack_all_wakerefs(struct intel_runtime_pm_debug *debug,
+ struct intel_runtime_pm_debug *saved)
+{
+ *saved = *debug;
+
+ debug->owners = NULL;
+ debug->count = 0;
+ debug->last_release = __save_depot_stack();
+}
+
+static void
+dump_and_free_wakeref_tracking(struct intel_runtime_pm_debug *debug)
{
- struct i915_runtime_pm *rpm = &i915->runtime_pm;
- struct intel_runtime_pm_debug dbg = {};
struct drm_printer p;
- unsigned long flags;
- if (atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
- &rpm->debug.lock,
- flags)) {
- dbg = rpm->debug;
-
- rpm->debug.owners = NULL;
- rpm->debug.count = 0;
- rpm->debug.last_release = __save_depot_stack();
-
- spin_unlock_irqrestore(&rpm->debug.lock, flags);
- }
- if (!dbg.count)
+ if (!debug->count)
return;
p = drm_debug_printer("i915");
- __print_intel_runtime_pm_wakeref(&p, &dbg);
+ __print_intel_runtime_pm_wakeref(&p, debug);
- kfree(dbg.owners);
+ kfree(debug->owners);
+}
+
+static noinline void
+__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 = {};
+ unsigned long flags;
+
+ if (!atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
+ &rpm->debug.lock,
+ flags))
+ return;
+
+ __untrack_all_wakerefs(&rpm->debug, &dbg);
+ spin_unlock_irqrestore(&rpm->debug.lock, flags);
+
+ dump_and_free_wakeref_tracking(&dbg);
+}
+
+static noinline void
+untrack_all_intel_runtime_pm_wakerefs(struct drm_i915_private *i915)
+{
+ struct i915_runtime_pm *rpm = &i915->runtime_pm;
+ struct intel_runtime_pm_debug dbg = {};
+ unsigned long flags;
+
+ spin_lock_irqsave(&rpm->debug.lock, flags);
+ __untrack_all_wakerefs(&rpm->debug, &dbg);
+ spin_unlock_irqrestore(&rpm->debug.lock, flags);
+
+ dump_and_free_wakeref_tracking(&dbg);
}
void print_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
@@ -321,6 +350,11 @@ __intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
atomic_dec(&i915->runtime_pm.wakeref_count);
}
+static void
+untrack_all_intel_runtime_pm_wakerefs(struct drm_i915_private *i915)
+{
+}
+
#endif
static void
@@ -4838,15 +4872,14 @@ void intel_runtime_pm_disable(struct drm_i915_private *i915)
void intel_runtime_pm_cleanup(struct drm_i915_private *i915)
{
struct i915_runtime_pm *rpm = &i915->runtime_pm;
- int count;
+ int count = atomic_read(&rpm->wakeref_count);
- count = atomic_fetch_inc(&rpm->wakeref_count); /* balance untrack */
WARN(count,
"i915 raw-wakerefs=%d wakelocks=%d on cleanup\n",
intel_rpm_raw_wakeref_count(count),
intel_rpm_wakelock_count(count));
- intel_runtime_pm_release(i915, false);
+ untrack_all_intel_runtime_pm_wakerefs(i915);
}
void intel_runtime_pm_init_early(struct drm_i915_private *i915)
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 02/11] drm/i915: Force printing wakeref tacking during pm_cleanup
2019-05-09 6:19 ` [PATCH v2 02/11] drm/i915: Force printing wakeref tacking during pm_cleanup Imre Deak
@ 2019-05-09 8:05 ` Chris Wilson
0 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2019-05-09 8:05 UTC (permalink / raw)
To: Imre Deak, intel-gfx
Quoting Imre Deak (2019-05-09 07:19:45)
> Make sure we print and drop the wakeref tracking info during pm_cleanup
> even if there are wakeref holders (either raw-wakeref or wakelock
> holders). Dropping the wakeref tracking means that a late put on the ref
> will WARN since the wakeref will be unknown, but that is rightly so,
> since the put is late and we want to catch that case.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 75 ++++++++++++++++++-------
> 1 file changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 84a18d8b942c..dc964c8608f1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -233,31 +233,60 @@ __print_intel_runtime_pm_wakeref(struct drm_printer *p,
> }
>
> static noinline void
> -__intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
> +__untrack_all_wakerefs(struct intel_runtime_pm_debug *debug,
> + struct intel_runtime_pm_debug *saved)
> +{
> + *saved = *debug;
> +
> + debug->owners = NULL;
> + debug->count = 0;
> + debug->last_release = __save_depot_stack();
> +}
> +
> +static void
> +dump_and_free_wakeref_tracking(struct intel_runtime_pm_debug *debug)
> {
> - struct i915_runtime_pm *rpm = &i915->runtime_pm;
> - struct intel_runtime_pm_debug dbg = {};
> struct drm_printer p;
> - unsigned long flags;
>
> - if (atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
> - &rpm->debug.lock,
> - flags)) {
> - dbg = rpm->debug;
> -
> - rpm->debug.owners = NULL;
> - rpm->debug.count = 0;
> - rpm->debug.last_release = __save_depot_stack();
> -
> - spin_unlock_irqrestore(&rpm->debug.lock, flags);
> - }
> - if (!dbg.count)
> + if (!debug->count)
> return;
>
> p = drm_debug_printer("i915");
> - __print_intel_runtime_pm_wakeref(&p, &dbg);
> + __print_intel_runtime_pm_wakeref(&p, debug);
>
> - kfree(dbg.owners);
> + kfree(debug->owners);
> +}
> +
> +static noinline void
> +__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 = {};
> + unsigned long flags;
> +
> + if (!atomic_dec_and_lock_irqsave(&rpm->wakeref_count,
> + &rpm->debug.lock,
> + flags))
> + return;
> +
> + __untrack_all_wakerefs(&rpm->debug, &dbg);
> + spin_unlock_irqrestore(&rpm->debug.lock, flags);
> +
> + dump_and_free_wakeref_tracking(&dbg);
> +}
> +
> +static noinline void
> +untrack_all_intel_runtime_pm_wakerefs(struct drm_i915_private *i915)
> +{
> + struct i915_runtime_pm *rpm = &i915->runtime_pm;
> + struct intel_runtime_pm_debug dbg = {};
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rpm->debug.lock, flags);
> + __untrack_all_wakerefs(&rpm->debug, &dbg);
> + spin_unlock_irqrestore(&rpm->debug.lock, flags);
> +
> + dump_and_free_wakeref_tracking(&dbg);
> }
>
> void print_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
> @@ -321,6 +350,11 @@ __intel_wakeref_dec_and_check_tracking(struct drm_i915_private *i915)
> atomic_dec(&i915->runtime_pm.wakeref_count);
> }
>
> +static void
> +untrack_all_intel_runtime_pm_wakerefs(struct drm_i915_private *i915)
> +{
> +}
> +
> #endif
>
> static void
> @@ -4838,15 +4872,14 @@ void intel_runtime_pm_disable(struct drm_i915_private *i915)
> void intel_runtime_pm_cleanup(struct drm_i915_private *i915)
> {
> struct i915_runtime_pm *rpm = &i915->runtime_pm;
> - int count;
> + int count = atomic_read(&rpm->wakeref_count);
>
> - count = atomic_fetch_inc(&rpm->wakeref_count); /* balance untrack */
> WARN(count,
> "i915 raw-wakerefs=%d wakelocks=%d on cleanup\n",
> intel_rpm_raw_wakeref_count(count),
> intel_rpm_wakelock_count(count));
>
> - intel_runtime_pm_release(i915, false);
> + untrack_all_intel_runtime_pm_wakerefs(i915);
> }
That looks much better, thanks!
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 03/11] drm/i915: Verify power domains state during suspend in all cases
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
2019-05-09 6:19 ` [PATCH v2 01/11] drm/i915: Add support for tracking wakerefs w/o power-on guarantee Imre Deak
2019-05-09 6:19 ` [PATCH v2 02/11] drm/i915: Force printing wakeref tacking during pm_cleanup Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 8:05 ` Chris Wilson
2019-05-09 6:19 ` [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (11 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
There is no reason why we couldn't verify the power domains state during
suspend in all cases, so do that. I overlooked this when originally
adding the check.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_runtime_pm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index dc964c8608f1..89d6309bb1f7 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -4528,10 +4528,10 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
* Even if power well support was disabled we still want to disable
* power wells if power domains must be deinitialized for suspend.
*/
- if (!i915_modparams.disable_power_well) {
+ if (!i915_modparams.disable_power_well)
intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
- intel_power_domains_verify_state(i915);
- }
+
+ intel_power_domains_verify_state(i915);
if (INTEL_GEN(i915) >= 11)
icl_display_core_uninit(i915);
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (2 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 03/11] drm/i915: Verify power domains state during suspend in all cases Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 8:17 ` Chris Wilson
2019-05-09 6:19 ` [PATCH v2 05/11] drm/i915: Disable power asynchronously during DP AUX transfers Imre Deak
` (10 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
By disabling a power domain asynchronously we can restrict holding a
reference on that power domain to the actual code sequence that
requires the power to be on for the HW access it's doing, by also
avoiding unneeded on-off-on togglings of the power domain (since the
disabling happens with a delay).
One benefit is potential power saving due to the following two reasons:
1. The fact that we will now be holding the reference only for the
necessary duration by the end of the patchset. While simply not
delaying the disabling has the same benefit, it has the problem that
frequent on-off-on power switching has its own power cost (see the 2.
point below) and the debug trace for power well on/off events will
cause a lot of dmesg spam (see details about this further below).
2. Avoiding the power cost of freuqent on-off-on power switching. This
requires us to find the optimal disabling delay based on the measured
power cost of on->off and off->on switching of each power well vs.
the power of keeping the given power well on.
In this patchset I'm not providing this optimal delay for two
reasons:
a) I don't have the means yet to perform the measurement (with high
enough signal-to-noise ratio, or with the help of an energy
counter that takes switching into account). I'm currently looking
for a way to measure this.
b) Before reducing the disabling delay we need an alternative way for
debug tracing powerwell on/off events. Simply avoiding/throttling
the debug messages is not a solution, see further below.
Note that even in the case where we can't measure any considerable
power cost of frequent on-off switching of powerwells, it still would
make sense to do the disabling asynchronously (with 0 delay) to avoid
blocking on the disabling. On VLV I measured this disabling time
overhead to be 1ms on average with a worst case of 4ms.
In the case of the AUX power domains on ICL we would also need to keep
the sequence where we hold the power reference short, the way it would
be by the end of this patchset where we hold it only for the actual AUX
transfer. Anything else would make the locking we need for ICL TypeC
ports (whenever we hold a reference on any AUX power domain) rather
problematic, adding for instance unnecessary lockdep dependencies to
the required TypeC port lock.
I chose the disabling delay to be 100msec for now to avoid the unneeded
toggling (and so not to introduce dmesg spamming) in the DP MST sideband
signaling code. We could optimize this delay later, once we have the
means to measure the switching power cost (see above).
Note that simply removing/throttling the debug tracing for power well
on/off events is not a solution. We need to know the exact spots of
these events and cannot rely only on incorrect register accesses caught
(due to not holding a wakeref at the time of access). Incorrect
powerwell enabling/disabling could lead to other problems, for instance
we need to keep certain powerwells enabled for the duration of modesets
and AUX transfers.
v2:
- Clarify the commit log parts about power cost measurement and the
problem of simply removing/throttling debug tracing. (Chris)
- Optimize out local wakeref vars at intel_runtime_pm_put_raw() and
intel_display_power_put_async() call sites if
CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris)
- Rebased on v2 of the wakeref w/o power-on guarantee patch.
- Add missing docbook headers.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 5 +
drivers/gpu/drm/i915/intel_runtime_pm.c | 362 +++++++++++++++++++++++-
drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +
3 files changed, 368 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0257808734c..5801f5407589 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -834,6 +834,11 @@ struct i915_power_domains {
struct mutex lock;
int domain_use_count[POWER_DOMAIN_NUM];
+
+ struct delayed_work async_put_work;
+ intel_wakeref_t async_put_wakeref;
+ u64 async_put_domains[2];
+
struct i915_power_well *power_wells;
};
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 89d6309bb1f7..829d483d27e9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -60,6 +60,19 @@
* present for a given platform.
*/
+static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915);
+static void
+__intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref,
+ bool wakelock);
+
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+static void
+intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref);
+#else
+#define intel_runtime_pm_put_raw(i915, wref) \
+ __intel_runtime_pm_put(i915, -1, false)
+#endif
+
#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
#include <linux/sort.h>
@@ -1903,6 +1916,125 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
chv_set_pipe_power_well(dev_priv, power_well, false);
}
+static u64 __async_put_domains_mask(struct i915_power_domains *power_domains)
+{
+ return power_domains->async_put_domains[0] |
+ power_domains->async_put_domains[1];
+}
+
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+
+static bool
+assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
+{
+ return !WARN_ON(power_domains->async_put_domains[0] &
+ power_domains->async_put_domains[1]);
+}
+
+static bool
+__async_put_domains_state_ok(struct i915_power_domains *power_domains)
+{
+ enum intel_display_power_domain domain;
+ bool err = false;
+
+ err |= !assert_async_put_domain_masks_disjoint(power_domains);
+ err |= WARN_ON(!!power_domains->async_put_wakeref !=
+ !!__async_put_domains_mask(power_domains));
+
+ for_each_power_domain(domain, __async_put_domains_mask(power_domains))
+ err |= WARN_ON(power_domains->domain_use_count[domain] != 1);
+
+ return !err;
+}
+
+static void print_power_domains(struct i915_power_domains *power_domains,
+ const char *prefix, u64 mask)
+{
+ enum intel_display_power_domain domain;
+
+ DRM_DEBUG_DRIVER("%s (%lu):\n", prefix, hweight64(mask));
+ for_each_power_domain(domain, mask)
+ DRM_DEBUG_DRIVER("%s use_count %d\n",
+ intel_display_power_domain_str(domain),
+ power_domains->domain_use_count[domain]);
+}
+
+static void
+print_async_put_domains_state(struct i915_power_domains *power_domains)
+{
+ DRM_DEBUG_DRIVER("async_put_wakeref %u\n",
+ power_domains->async_put_wakeref);
+
+ print_power_domains(power_domains, "async_put_domains[0]",
+ power_domains->async_put_domains[0]);
+ print_power_domains(power_domains, "async_put_domains[1]",
+ power_domains->async_put_domains[1]);
+}
+
+static void
+verify_async_put_domains_state(struct i915_power_domains *power_domains)
+{
+ if (!__async_put_domains_state_ok(power_domains))
+ print_async_put_domains_state(power_domains);
+}
+
+#else
+
+static void
+assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
+{
+}
+
+static void
+verify_async_put_domains_state(struct i915_power_domains *power_domains)
+{
+}
+
+#endif /* CONFIG_DRM_I915_DEBUG_RUNTIME_PM */
+
+static u64 async_put_domains_mask(struct i915_power_domains *power_domains)
+{
+ assert_async_put_domain_masks_disjoint(power_domains);
+
+ return __async_put_domains_mask(power_domains);
+}
+
+static void
+async_put_domains_clear_domain(struct i915_power_domains *power_domains,
+ enum intel_display_power_domain domain)
+{
+ assert_async_put_domain_masks_disjoint(power_domains);
+
+ power_domains->async_put_domains[0] &= ~BIT_ULL(domain);
+ power_domains->async_put_domains[1] &= ~BIT_ULL(domain);
+}
+
+static bool
+intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
+ enum intel_display_power_domain domain)
+{
+ struct i915_power_domains *power_domains = &dev_priv->power_domains;
+ bool ret = false;
+
+ if (!(async_put_domains_mask(power_domains) & BIT_ULL(domain)))
+ goto out_verify;
+
+ async_put_domains_clear_domain(power_domains, domain);
+
+ ret = true;
+
+ if (async_put_domains_mask(power_domains))
+ goto out_verify;
+
+ cancel_delayed_work(&power_domains->async_put_work);
+ intel_runtime_pm_put_raw(dev_priv,
+ fetch_and_zero(&power_domains->async_put_wakeref));
+out_verify:
+ verify_async_put_domains_state(power_domains);
+
+ return ret;
+}
+
static void
__intel_display_power_get_domain(struct drm_i915_private *dev_priv,
enum intel_display_power_domain domain)
@@ -1910,6 +2042,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
struct i915_power_domains *power_domains = &dev_priv->power_domains;
struct i915_power_well *power_well;
+ if (intel_display_power_grab_async_put_ref(dev_priv, domain))
+ return;
+
for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
intel_power_well_get(dev_priv, power_well);
@@ -1935,9 +2070,7 @@ intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
intel_wakeref_t wakeref = intel_runtime_pm_get(dev_priv);
mutex_lock(&power_domains->lock);
-
__intel_display_power_get_domain(dev_priv, domain);
-
mutex_unlock(&power_domains->lock);
return wakeref;
@@ -1986,24 +2119,36 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
return wakeref;
}
-static void __intel_display_power_put(struct drm_i915_private *dev_priv,
- enum intel_display_power_domain domain)
+static void
+__intel_display_power_put_domain(struct drm_i915_private *dev_priv,
+ enum intel_display_power_domain domain)
{
struct i915_power_domains *power_domains;
struct i915_power_well *power_well;
+ const char *name = intel_display_power_domain_str(domain);
power_domains = &dev_priv->power_domains;
- mutex_lock(&power_domains->lock);
-
WARN(!power_domains->domain_use_count[domain],
"Use count on domain %s is already zero\n",
- intel_display_power_domain_str(domain));
+ name);
+ WARN(async_put_domains_mask(power_domains) & BIT_ULL(domain),
+ "Async disabling of domain %s is pending\n",
+ name);
+
power_domains->domain_use_count[domain]--;
for_each_power_domain_well_reverse(dev_priv, power_well, BIT_ULL(domain))
intel_power_well_put(dev_priv, power_well);
+}
+static void __intel_display_power_put(struct drm_i915_private *dev_priv,
+ enum intel_display_power_domain domain)
+{
+ struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+ mutex_lock(&power_domains->lock);
+ __intel_display_power_put_domain(dev_priv, domain);
mutex_unlock(&power_domains->lock);
}
@@ -2027,6 +2172,187 @@ void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
intel_runtime_pm_put_unchecked(dev_priv);
}
+static void
+queue_async_put_domains_work(struct i915_power_domains *power_domains,
+ intel_wakeref_t wakeref)
+{
+ WARN_ON(power_domains->async_put_wakeref);
+ power_domains->async_put_wakeref = wakeref;
+ WARN_ON(!queue_delayed_work(system_unbound_wq,
+ &power_domains->async_put_work,
+ msecs_to_jiffies(100)));
+}
+
+static void
+release_async_put_domains(struct i915_power_domains *power_domains, u64 mask)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(power_domains, struct drm_i915_private,
+ power_domains);
+ enum intel_display_power_domain domain;
+ intel_wakeref_t wakeref;
+
+ /*
+ * The caller must hold already raw wakeref, upgrade that to a proper
+ * wakeref to make the state checker happy about the HW access during
+ * power well disabling.
+ */
+ assert_rpm_raw_wakeref_held(dev_priv);
+ wakeref = intel_runtime_pm_get(dev_priv);
+
+ for_each_power_domain(domain, mask) {
+ /* Clear before put, so put's sanity check is happy. */
+ async_put_domains_clear_domain(power_domains, domain);
+ __intel_display_power_put_domain(dev_priv, domain);
+ }
+
+ intel_runtime_pm_put(dev_priv, wakeref);
+}
+
+static void
+intel_display_power_put_async_work(struct work_struct *work)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(work, struct drm_i915_private,
+ power_domains.async_put_work.work);
+ struct i915_power_domains *power_domains = &dev_priv->power_domains;
+ intel_wakeref_t new_work_wakeref = intel_runtime_pm_get_raw(dev_priv);
+ intel_wakeref_t old_work_wakeref = 0;
+
+ mutex_lock(&power_domains->lock);
+
+ /*
+ * Bail out if all the domain refs pending to be released were grabbed
+ * by subsequent gets or a flush_work.
+ */
+ old_work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
+ if (!old_work_wakeref)
+ goto out_verify;
+
+ release_async_put_domains(power_domains,
+ power_domains->async_put_domains[0]);
+
+ /* Requeue the work if more domains were async put meanwhile. */
+ if (power_domains->async_put_domains[1]) {
+ power_domains->async_put_domains[0] =
+ fetch_and_zero(&power_domains->async_put_domains[1]);
+ queue_async_put_domains_work(power_domains,
+ fetch_and_zero(&new_work_wakeref));
+ }
+
+out_verify:
+ verify_async_put_domains_state(power_domains);
+
+ mutex_unlock(&power_domains->lock);
+
+ if (old_work_wakeref)
+ intel_runtime_pm_put_raw(dev_priv, old_work_wakeref);
+ if (new_work_wakeref)
+ intel_runtime_pm_put_raw(dev_priv, new_work_wakeref);
+}
+/**
+ * intel_display_power_put_async - release a power domain reference asynchronously
+ * @i915: 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 schedules a work to power down the
+ * corresponding hardware block if this is the last reference.
+ */
+void __intel_display_power_put_async(struct drm_i915_private *i915,
+ enum intel_display_power_domain domain,
+ intel_wakeref_t wakeref)
+{
+ struct i915_power_domains *power_domains = &i915->power_domains;
+ intel_wakeref_t work_wakeref = intel_runtime_pm_get_raw(i915);
+
+ mutex_lock(&power_domains->lock);
+
+ if (power_domains->domain_use_count[domain] > 1) {
+ __intel_display_power_put_domain(i915, domain);
+
+ goto out_verify;
+ }
+
+ WARN_ON(power_domains->domain_use_count[domain] != 1);
+
+ /* Let a pending work requeue itself or queue a new one. */
+ if (power_domains->async_put_wakeref) {
+ power_domains->async_put_domains[1] |= BIT_ULL(domain);
+ } else {
+ power_domains->async_put_domains[0] |= BIT_ULL(domain);
+ queue_async_put_domains_work(power_domains,
+ fetch_and_zero(&work_wakeref));
+ }
+
+out_verify:
+ verify_async_put_domains_state(power_domains);
+
+ mutex_unlock(&power_domains->lock);
+
+ if (work_wakeref)
+ intel_runtime_pm_put_raw(i915, work_wakeref);
+
+ intel_runtime_pm_put(i915, wakeref);
+}
+
+/**
+ * intel_display_power_flush_work - flushes the async display power disabling work
+ * @i915: i915 device instance
+ *
+ * Flushes any pending work that was scheduled by a preceeding
+ * intel_display_power_put_async() call, completing the disabling of the
+ * corresponding power domains.
+ *
+ * Note that the work handler function may still be running after this
+ * function returns; to ensure that the work handler isn't running use
+ * intel_display_power_flush_work_sync() instead.
+ */
+void intel_display_power_flush_work(struct drm_i915_private *i915)
+{
+ struct i915_power_domains *power_domains = &i915->power_domains;
+ intel_wakeref_t work_wakeref;
+
+ mutex_lock(&power_domains->lock);
+
+ work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
+ if (!work_wakeref)
+ goto out_verify;
+
+ release_async_put_domains(power_domains,
+ async_put_domains_mask(power_domains));
+ cancel_delayed_work(&power_domains->async_put_work);
+
+out_verify:
+ verify_async_put_domains_state(power_domains);
+
+ mutex_unlock(&power_domains->lock);
+
+ if (work_wakeref)
+ intel_runtime_pm_put_raw(i915, work_wakeref);
+}
+
+/**
+ * intel_display_power_flush_work_sync - flushes and syncs the async display power disabling work
+ * @i915: i915 device instance
+ *
+ * Like intel_display_power_flush_work(), but also ensure that the work
+ * handler function is not running any more when this function returns.
+ */
+static void
+intel_display_power_flush_work_sync(struct drm_i915_private *dev_priv)
+{
+ struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+ intel_display_power_flush_work(dev_priv);
+ cancel_delayed_work_sync(&power_domains->async_put_work);
+
+ verify_async_put_domains_state(power_domains);
+
+ WARN_ON(power_domains->async_put_wakeref);
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
/**
* intel_display_power_put - release a power domain reference
@@ -3525,6 +3851,9 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
mutex_init(&power_domains->lock);
+ INIT_DELAYED_WORK(&power_domains->async_put_work,
+ intel_display_power_put_async_work);
+
/*
* The enabling order will be from lower to higher indexed wells,
* the disabling order is reversed.
@@ -4445,6 +4774,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *i915)
if (!i915_modparams.disable_power_well)
intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
+ intel_display_power_flush_work_sync(i915);
+
intel_power_domains_verify_state(i915);
/* Keep the power well enabled, but cancel its rpm wakeref. */
@@ -4520,6 +4851,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
if (!(i915->csr.allowed_dc_mask & DC_STATE_EN_DC9) &&
suspend_mode == I915_DRM_SUSPEND_IDLE &&
i915->csr.dmc_payload) {
+ intel_display_power_flush_work(i915);
intel_power_domains_verify_state(i915);
return;
}
@@ -4531,6 +4863,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
if (!i915_modparams.disable_power_well)
intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
+ intel_display_power_flush_work(i915);
intel_power_domains_verify_state(i915);
if (INTEL_GEN(i915) >= 11)
@@ -4609,6 +4942,8 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
mutex_lock(&power_domains->lock);
+ verify_async_put_domains_state(power_domains);
+
dump_domain_info = false;
for_each_power_well(i915, power_well) {
enum intel_display_power_domain domain;
@@ -4670,6 +5005,11 @@ static intel_wakeref_t __intel_runtime_pm_get(struct drm_i915_private *i915,
return track_intel_runtime_pm_wakeref(i915);
}
+static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915)
+{
+ return __intel_runtime_pm_get(i915, false);
+}
+
/**
* intel_runtime_pm_get - grab a runtime pm reference
* @i915: i915 device instance
@@ -4769,6 +5109,14 @@ static void __intel_runtime_pm_put(struct drm_i915_private *i915,
pm_runtime_put_autosuspend(kdev);
}
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+static void
+intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref)
+{
+ __intel_runtime_pm_put(i915, wref, false);
+}
+#endif
+
/**
* intel_runtime_pm_put_unchecked - release an unchecked runtime pm reference
* @i915: i915 device instance
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
index e30b38632bd2..a7a3d929546f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.h
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
@@ -59,13 +59,21 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
enum intel_display_power_domain domain);
void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
enum intel_display_power_domain domain);
+void __intel_display_power_put_async(struct drm_i915_private *i915,
+ enum intel_display_power_domain domain,
+ intel_wakeref_t wakeref);
+void intel_display_power_flush_work(struct drm_i915_private *i915);
#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
void intel_display_power_put(struct drm_i915_private *dev_priv,
enum intel_display_power_domain domain,
intel_wakeref_t wakeref);
+#define intel_display_power_put_async(i915, domain, wakeref) \
+ __intel_display_power_put_async(i915, domain, wakeref)
#else
#define intel_display_power_put(i915, domain, wakeref) \
intel_display_power_put_unchecked(i915, domain)
+#define intel_display_power_put_async(i915, domain, wakeref) \
+ __intel_display_power_put_async(i915, domain, -1)
#endif
void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
u8 req_slices);
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling
2019-05-09 6:19 ` [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
@ 2019-05-09 8:17 ` Chris Wilson
2019-05-09 10:46 ` Imre Deak
0 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2019-05-09 8:17 UTC (permalink / raw)
To: Imre Deak, intel-gfx
Quoting Imre Deak (2019-05-09 07:19:47)
> By disabling a power domain asynchronously we can restrict holding a
> reference on that power domain to the actual code sequence that
> requires the power to be on for the HW access it's doing, by also
> avoiding unneeded on-off-on togglings of the power domain (since the
> disabling happens with a delay).
>
> One benefit is potential power saving due to the following two reasons:
> 1. The fact that we will now be holding the reference only for the
> necessary duration by the end of the patchset. While simply not
> delaying the disabling has the same benefit, it has the problem that
> frequent on-off-on power switching has its own power cost (see the 2.
> point below) and the debug trace for power well on/off events will
> cause a lot of dmesg spam (see details about this further below).
> 2. Avoiding the power cost of freuqent on-off-on power switching. This
> requires us to find the optimal disabling delay based on the measured
> power cost of on->off and off->on switching of each power well vs.
> the power of keeping the given power well on.
>
> In this patchset I'm not providing this optimal delay for two
> reasons:
> a) I don't have the means yet to perform the measurement (with high
> enough signal-to-noise ratio, or with the help of an energy
> counter that takes switching into account). I'm currently looking
> for a way to measure this.
>
> b) Before reducing the disabling delay we need an alternative way for
> debug tracing powerwell on/off events. Simply avoiding/throttling
> the debug messages is not a solution, see further below.
>
> Note that even in the case where we can't measure any considerable
> power cost of frequent on-off switching of powerwells, it still would
> make sense to do the disabling asynchronously (with 0 delay) to avoid
> blocking on the disabling. On VLV I measured this disabling time
> overhead to be 1ms on average with a worst case of 4ms.
Good point. Again, that would be nice to show in a microbenchmark -- the
latency will still impact the wakeup path (I expect), the challenge is
identifying userspace path impacted and relating that to workloads. The
further challenge is that since this is power centric, we need to
measure the power vs latency / throughput of such workloads.
> In the case of the AUX power domains on ICL we would also need to keep
> the sequence where we hold the power reference short, the way it would
> be by the end of this patchset where we hold it only for the actual AUX
> transfer. Anything else would make the locking we need for ICL TypeC
> ports (whenever we hold a reference on any AUX power domain) rather
> problematic, adding for instance unnecessary lockdep dependencies to
> the required TypeC port lock.
>
> I chose the disabling delay to be 100msec for now to avoid the unneeded
> toggling (and so not to introduce dmesg spamming) in the DP MST sideband
> signaling code. We could optimize this delay later, once we have the
> means to measure the switching power cost (see above).
>
> Note that simply removing/throttling the debug tracing for power well
> on/off events is not a solution. We need to know the exact spots of
> these events and cannot rely only on incorrect register accesses caught
> (due to not holding a wakeref at the time of access). Incorrect
> powerwell enabling/disabling could lead to other problems, for instance
> we need to keep certain powerwells enabled for the duration of modesets
> and AUX transfers.
>
> v2:
> - Clarify the commit log parts about power cost measurement and the
> problem of simply removing/throttling debug tracing. (Chris)
> - Optimize out local wakeref vars at intel_runtime_pm_put_raw() and
> intel_display_power_put_async() call sites if
> CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris)
> - Rebased on v2 of the wakeref w/o power-on guarantee patch.
> - Add missing docbook headers.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +
> drivers/gpu/drm/i915/intel_runtime_pm.c | 362 +++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +
> 3 files changed, 368 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d0257808734c..5801f5407589 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -834,6 +834,11 @@ struct i915_power_domains {
>
> struct mutex lock;
> int domain_use_count[POWER_DOMAIN_NUM];
> +
> + struct delayed_work async_put_work;
> + intel_wakeref_t async_put_wakeref;
> + u64 async_put_domains[2];
Fwiw, in the forcewake code we used the term 'auto' for the extra
reference held by the timer for the delayed released.
There we used a timer per fw_domain, which are much fewer in number than
power_domains!, but that has the advantage of avoiding a clock
algorithm.
> +
> struct i915_power_well *power_wells;
Would stuffing a timer into each power well be simpler? Just wondering.
Also wondering how much alike we could make the async released and if we
can make a small library.
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 89d6309bb1f7..829d483d27e9 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -60,6 +60,19 @@
> * present for a given platform.
> */
>
> +static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915);
> +static void
> +__intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref,
> + bool wakelock);
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> +static void
> +intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref);
> +#else
> +#define intel_runtime_pm_put_raw(i915, wref) \
> + __intel_runtime_pm_put(i915, -1, false)
> +#endif
> +
> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>
> #include <linux/sort.h>
> @@ -1903,6 +1916,125 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> chv_set_pipe_power_well(dev_priv, power_well, false);
> }
>
> +static u64 __async_put_domains_mask(struct i915_power_domains *power_domains)
> +{
> + return power_domains->async_put_domains[0] |
> + power_domains->async_put_domains[1];
> +}
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> +
> +static bool
> +assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
> +{
> + return !WARN_ON(power_domains->async_put_domains[0] &
> + power_domains->async_put_domains[1]);
> +}
> +
> +static bool
> +__async_put_domains_state_ok(struct i915_power_domains *power_domains)
> +{
> + enum intel_display_power_domain domain;
> + bool err = false;
> +
> + err |= !assert_async_put_domain_masks_disjoint(power_domains);
> + err |= WARN_ON(!!power_domains->async_put_wakeref !=
> + !!__async_put_domains_mask(power_domains));
> +
> + for_each_power_domain(domain, __async_put_domains_mask(power_domains))
> + err |= WARN_ON(power_domains->domain_use_count[domain] != 1);
> +
> + return !err;
> +}
> +
> +static void print_power_domains(struct i915_power_domains *power_domains,
> + const char *prefix, u64 mask)
> +{
> + enum intel_display_power_domain domain;
> +
> + DRM_DEBUG_DRIVER("%s (%lu):\n", prefix, hweight64(mask));
> + for_each_power_domain(domain, mask)
> + DRM_DEBUG_DRIVER("%s use_count %d\n",
> + intel_display_power_domain_str(domain),
> + power_domains->domain_use_count[domain]);
> +}
> +
> +static void
> +print_async_put_domains_state(struct i915_power_domains *power_domains)
> +{
> + DRM_DEBUG_DRIVER("async_put_wakeref %u\n",
> + power_domains->async_put_wakeref);
> +
> + print_power_domains(power_domains, "async_put_domains[0]",
> + power_domains->async_put_domains[0]);
> + print_power_domains(power_domains, "async_put_domains[1]",
> + power_domains->async_put_domains[1]);
> +}
> +
> +static void
> +verify_async_put_domains_state(struct i915_power_domains *power_domains)
> +{
> + if (!__async_put_domains_state_ok(power_domains))
> + print_async_put_domains_state(power_domains);
> +}
> +
> +#else
> +
> +static void
> +assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
> +{
> +}
> +
> +static void
> +verify_async_put_domains_state(struct i915_power_domains *power_domains)
> +{
> +}
> +
> +#endif /* CONFIG_DRM_I915_DEBUG_RUNTIME_PM */
> +
> +static u64 async_put_domains_mask(struct i915_power_domains *power_domains)
> +{
> + assert_async_put_domain_masks_disjoint(power_domains);
> +
> + return __async_put_domains_mask(power_domains);
> +}
> +
> +static void
> +async_put_domains_clear_domain(struct i915_power_domains *power_domains,
> + enum intel_display_power_domain domain)
> +{
> + assert_async_put_domain_masks_disjoint(power_domains);
> +
> + power_domains->async_put_domains[0] &= ~BIT_ULL(domain);
> + power_domains->async_put_domains[1] &= ~BIT_ULL(domain);
> +}
> +
> +static bool
> +intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain)
> +{
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> + bool ret = false;
> +
> + if (!(async_put_domains_mask(power_domains) & BIT_ULL(domain)))
> + goto out_verify;
> +
> + async_put_domains_clear_domain(power_domains, domain);
> +
> + ret = true;
> +
> + if (async_put_domains_mask(power_domains))
> + goto out_verify;
> +
> + cancel_delayed_work(&power_domains->async_put_work);
> + intel_runtime_pm_put_raw(dev_priv,
> + fetch_and_zero(&power_domains->async_put_wakeref));
> +out_verify:
> + verify_async_put_domains_state(power_domains);
> +
> + return ret;
> +}
> +
> static void
> __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> enum intel_display_power_domain domain)
> @@ -1910,6 +2042,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> struct i915_power_domains *power_domains = &dev_priv->power_domains;
> struct i915_power_well *power_well;
>
> + if (intel_display_power_grab_async_put_ref(dev_priv, domain))
> + return;
> +
> for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
> intel_power_well_get(dev_priv, power_well);
>
> @@ -1935,9 +2070,7 @@ intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
> intel_wakeref_t wakeref = intel_runtime_pm_get(dev_priv);
>
> mutex_lock(&power_domains->lock);
> -
> __intel_display_power_get_domain(dev_priv, domain);
> -
> mutex_unlock(&power_domains->lock);
>
> return wakeref;
> @@ -1986,24 +2119,36 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> return wakeref;
> }
>
> -static void __intel_display_power_put(struct drm_i915_private *dev_priv,
> - enum intel_display_power_domain domain)
> +static void
> +__intel_display_power_put_domain(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain)
> {
> struct i915_power_domains *power_domains;
> struct i915_power_well *power_well;
> + const char *name = intel_display_power_domain_str(domain);
>
> power_domains = &dev_priv->power_domains;
>
> - mutex_lock(&power_domains->lock);
> -
> WARN(!power_domains->domain_use_count[domain],
> "Use count on domain %s is already zero\n",
> - intel_display_power_domain_str(domain));
> + name);
> + WARN(async_put_domains_mask(power_domains) & BIT_ULL(domain),
> + "Async disabling of domain %s is pending\n",
> + name);
> +
> power_domains->domain_use_count[domain]--;
>
> for_each_power_domain_well_reverse(dev_priv, power_well, BIT_ULL(domain))
> intel_power_well_put(dev_priv, power_well);
> +}
>
> +static void __intel_display_power_put(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain)
> +{
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +
> + mutex_lock(&power_domains->lock);
> + __intel_display_power_put_domain(dev_priv, domain);
> mutex_unlock(&power_domains->lock);
> }
>
> @@ -2027,6 +2172,187 @@ void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
> intel_runtime_pm_put_unchecked(dev_priv);
> }
>
> +static void
> +queue_async_put_domains_work(struct i915_power_domains *power_domains,
> + intel_wakeref_t wakeref)
> +{
> + WARN_ON(power_domains->async_put_wakeref);
> + power_domains->async_put_wakeref = wakeref;
> + WARN_ON(!queue_delayed_work(system_unbound_wq,
> + &power_domains->async_put_work,
> + msecs_to_jiffies(100)));
> +}
> +
> +static void
> +release_async_put_domains(struct i915_power_domains *power_domains, u64 mask)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(power_domains, struct drm_i915_private,
> + power_domains);
> + enum intel_display_power_domain domain;
> + intel_wakeref_t wakeref;
> +
> + /*
> + * The caller must hold already raw wakeref, upgrade that to a proper
> + * wakeref to make the state checker happy about the HW access during
> + * power well disabling.
> + */
> + assert_rpm_raw_wakeref_held(dev_priv);
> + wakeref = intel_runtime_pm_get(dev_priv);
> +
> + for_each_power_domain(domain, mask) {
> + /* Clear before put, so put's sanity check is happy. */
> + async_put_domains_clear_domain(power_domains, domain);
> + __intel_display_power_put_domain(dev_priv, domain);
> + }
> +
> + intel_runtime_pm_put(dev_priv, wakeref);
> +}
> +
> +static void
> +intel_display_power_put_async_work(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, struct drm_i915_private,
> + power_domains.async_put_work.work);
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> + intel_wakeref_t new_work_wakeref = intel_runtime_pm_get_raw(dev_priv);
> + intel_wakeref_t old_work_wakeref = 0;
> +
> + mutex_lock(&power_domains->lock);
> +
> + /*
> + * Bail out if all the domain refs pending to be released were grabbed
> + * by subsequent gets or a flush_work.
> + */
> + old_work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
> + if (!old_work_wakeref)
> + goto out_verify;
> +
> + release_async_put_domains(power_domains,
> + power_domains->async_put_domains[0]);
> +
> + /* Requeue the work if more domains were async put meanwhile. */
> + if (power_domains->async_put_domains[1]) {
> + power_domains->async_put_domains[0] =
> + fetch_and_zero(&power_domains->async_put_domains[1]);
> + queue_async_put_domains_work(power_domains,
> + fetch_and_zero(&new_work_wakeref));
> + }
> +
> +out_verify:
> + verify_async_put_domains_state(power_domains);
> +
> + mutex_unlock(&power_domains->lock);
> +
> + if (old_work_wakeref)
> + intel_runtime_pm_put_raw(dev_priv, old_work_wakeref);
> + if (new_work_wakeref)
> + intel_runtime_pm_put_raw(dev_priv, new_work_wakeref);
> +}
> +/**
> + * intel_display_power_put_async - release a power domain reference asynchronously
> + * @i915: 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 schedules a work to power down the
> + * corresponding hardware block if this is the last reference.
> + */
> +void __intel_display_power_put_async(struct drm_i915_private *i915,
> + enum intel_display_power_domain domain,
> + intel_wakeref_t wakeref)
> +{
> + struct i915_power_domains *power_domains = &i915->power_domains;
> + intel_wakeref_t work_wakeref = intel_runtime_pm_get_raw(i915);
> +
> + mutex_lock(&power_domains->lock);
> +
> + if (power_domains->domain_use_count[domain] > 1) {
> + __intel_display_power_put_domain(i915, domain);
> +
> + goto out_verify;
> + }
> +
> + WARN_ON(power_domains->domain_use_count[domain] != 1);
> +
> + /* Let a pending work requeue itself or queue a new one. */
> + if (power_domains->async_put_wakeref) {
> + power_domains->async_put_domains[1] |= BIT_ULL(domain);
> + } else {
> + power_domains->async_put_domains[0] |= BIT_ULL(domain);
> + queue_async_put_domains_work(power_domains,
> + fetch_and_zero(&work_wakeref));
> + }
> +
> +out_verify:
> + verify_async_put_domains_state(power_domains);
> +
> + mutex_unlock(&power_domains->lock);
> +
> + if (work_wakeref)
> + intel_runtime_pm_put_raw(i915, work_wakeref);
> +
> + intel_runtime_pm_put(i915, wakeref);
> +}
> +
> +/**
> + * intel_display_power_flush_work - flushes the async display power disabling work
> + * @i915: i915 device instance
> + *
> + * Flushes any pending work that was scheduled by a preceeding
> + * intel_display_power_put_async() call, completing the disabling of the
> + * corresponding power domains.
> + *
> + * Note that the work handler function may still be running after this
> + * function returns; to ensure that the work handler isn't running use
> + * intel_display_power_flush_work_sync() instead.
> + */
> +void intel_display_power_flush_work(struct drm_i915_private *i915)
> +{
> + struct i915_power_domains *power_domains = &i915->power_domains;
> + intel_wakeref_t work_wakeref;
> +
> + mutex_lock(&power_domains->lock);
> +
> + work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
> + if (!work_wakeref)
> + goto out_verify;
> +
> + release_async_put_domains(power_domains,
> + async_put_domains_mask(power_domains));
> + cancel_delayed_work(&power_domains->async_put_work);
> +
> +out_verify:
> + verify_async_put_domains_state(power_domains);
> +
> + mutex_unlock(&power_domains->lock);
> +
> + if (work_wakeref)
> + intel_runtime_pm_put_raw(i915, work_wakeref);
> +}
> +
> +/**
> + * intel_display_power_flush_work_sync - flushes and syncs the async display power disabling work
> + * @i915: i915 device instance
> + *
> + * Like intel_display_power_flush_work(), but also ensure that the work
> + * handler function is not running any more when this function returns.
> + */
> +static void
> +intel_display_power_flush_work_sync(struct drm_i915_private *dev_priv)
> +{
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +
> + intel_display_power_flush_work(dev_priv);
> + cancel_delayed_work_sync(&power_domains->async_put_work);
> +
> + verify_async_put_domains_state(power_domains);
> +
> + WARN_ON(power_domains->async_put_wakeref);
> +}
> +
> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> /**
> * intel_display_power_put - release a power domain reference
> @@ -3525,6 +3851,9 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>
> mutex_init(&power_domains->lock);
>
> + INIT_DELAYED_WORK(&power_domains->async_put_work,
> + intel_display_power_put_async_work);
> +
> /*
> * The enabling order will be from lower to higher indexed wells,
> * the disabling order is reversed.
> @@ -4445,6 +4774,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *i915)
> if (!i915_modparams.disable_power_well)
> intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
>
> + intel_display_power_flush_work_sync(i915);
> +
> intel_power_domains_verify_state(i915);
>
> /* Keep the power well enabled, but cancel its rpm wakeref. */
> @@ -4520,6 +4851,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
> if (!(i915->csr.allowed_dc_mask & DC_STATE_EN_DC9) &&
> suspend_mode == I915_DRM_SUSPEND_IDLE &&
> i915->csr.dmc_payload) {
> + intel_display_power_flush_work(i915);
> intel_power_domains_verify_state(i915);
> return;
> }
> @@ -4531,6 +4863,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
> if (!i915_modparams.disable_power_well)
> intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
>
> + intel_display_power_flush_work(i915);
> intel_power_domains_verify_state(i915);
>
> if (INTEL_GEN(i915) >= 11)
> @@ -4609,6 +4942,8 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
>
> mutex_lock(&power_domains->lock);
>
> + verify_async_put_domains_state(power_domains);
> +
> dump_domain_info = false;
> for_each_power_well(i915, power_well) {
> enum intel_display_power_domain domain;
> @@ -4670,6 +5005,11 @@ static intel_wakeref_t __intel_runtime_pm_get(struct drm_i915_private *i915,
> return track_intel_runtime_pm_wakeref(i915);
> }
>
> +static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915)
> +{
> + return __intel_runtime_pm_get(i915, false);
> +}
> +
> /**
> * intel_runtime_pm_get - grab a runtime pm reference
> * @i915: i915 device instance
> @@ -4769,6 +5109,14 @@ static void __intel_runtime_pm_put(struct drm_i915_private *i915,
> pm_runtime_put_autosuspend(kdev);
> }
>
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> +static void
> +intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref)
> +{
> + __intel_runtime_pm_put(i915, wref, false);
> +}
> +#endif
> +
> /**
> * intel_runtime_pm_put_unchecked - release an unchecked runtime pm reference
> * @i915: i915 device instance
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index e30b38632bd2..a7a3d929546f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -59,13 +59,21 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> enum intel_display_power_domain domain);
> void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
> enum intel_display_power_domain domain);
> +void __intel_display_power_put_async(struct drm_i915_private *i915,
> + enum intel_display_power_domain domain,
> + intel_wakeref_t wakeref);
> +void intel_display_power_flush_work(struct drm_i915_private *i915);
> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> void intel_display_power_put(struct drm_i915_private *dev_priv,
> enum intel_display_power_domain domain,
> intel_wakeref_t wakeref);
> +#define intel_display_power_put_async(i915, domain, wakeref) \
> + __intel_display_power_put_async(i915, domain, wakeref)
> #else
> #define intel_display_power_put(i915, domain, wakeref) \
> intel_display_power_put_unchecked(i915, domain)
> +#define intel_display_power_put_async(i915, domain, wakeref) \
> + __intel_display_power_put_async(i915, domain, -1)
> #endif
> void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> u8 req_slices);
Seems like it serves a purpose and does it well, with the only question
on how we can share similar code,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling
2019-05-09 8:17 ` Chris Wilson
@ 2019-05-09 10:46 ` Imre Deak
0 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 10:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, May 09, 2019 at 09:17:54AM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2019-05-09 07:19:47)
> > By disabling a power domain asynchronously we can restrict holding a
> > reference on that power domain to the actual code sequence that
> > requires the power to be on for the HW access it's doing, by also
> > avoiding unneeded on-off-on togglings of the power domain (since the
> > disabling happens with a delay).
> >
> > One benefit is potential power saving due to the following two reasons:
> > 1. The fact that we will now be holding the reference only for the
> > necessary duration by the end of the patchset. While simply not
> > delaying the disabling has the same benefit, it has the problem that
> > frequent on-off-on power switching has its own power cost (see the 2.
> > point below) and the debug trace for power well on/off events will
> > cause a lot of dmesg spam (see details about this further below).
> > 2. Avoiding the power cost of freuqent on-off-on power switching. This
> > requires us to find the optimal disabling delay based on the measured
> > power cost of on->off and off->on switching of each power well vs.
> > the power of keeping the given power well on.
> >
> > In this patchset I'm not providing this optimal delay for two
> > reasons:
> > a) I don't have the means yet to perform the measurement (with high
> > enough signal-to-noise ratio, or with the help of an energy
> > counter that takes switching into account). I'm currently looking
> > for a way to measure this.
> >
> > b) Before reducing the disabling delay we need an alternative way for
> > debug tracing powerwell on/off events. Simply avoiding/throttling
> > the debug messages is not a solution, see further below.
> >
> > Note that even in the case where we can't measure any considerable
> > power cost of frequent on-off switching of powerwells, it still would
> > make sense to do the disabling asynchronously (with 0 delay) to avoid
> > blocking on the disabling. On VLV I measured this disabling time
> > overhead to be 1ms on average with a worst case of 4ms.
>
> Good point. Again, that would be nice to show in a microbenchmark -- the
> latency will still impact the wakeup path (I expect),
I thought intel_display_power_grab_async_put_ref() would alleviate that.
> the challenge is
> identifying userspace path impacted and relating that to workloads. The
> further challenge is that since this is power centric, we need to
> measure the power vs latency / throughput of such workloads.
Ok. I can only promise to do these as a follow-up. We'd need most of all
some way for the power measurement, I asked about it from Art et al, but
ideas are welcome. The time measurement is easier, I will put together
some report about it across all platforms.
>
> > In the case of the AUX power domains on ICL we would also need to keep
> > the sequence where we hold the power reference short, the way it would
> > be by the end of this patchset where we hold it only for the actual AUX
> > transfer. Anything else would make the locking we need for ICL TypeC
> > ports (whenever we hold a reference on any AUX power domain) rather
> > problematic, adding for instance unnecessary lockdep dependencies to
> > the required TypeC port lock.
> >
> > I chose the disabling delay to be 100msec for now to avoid the unneeded
> > toggling (and so not to introduce dmesg spamming) in the DP MST sideband
> > signaling code. We could optimize this delay later, once we have the
> > means to measure the switching power cost (see above).
> >
> > Note that simply removing/throttling the debug tracing for power well
> > on/off events is not a solution. We need to know the exact spots of
> > these events and cannot rely only on incorrect register accesses caught
> > (due to not holding a wakeref at the time of access). Incorrect
> > powerwell enabling/disabling could lead to other problems, for instance
> > we need to keep certain powerwells enabled for the duration of modesets
> > and AUX transfers.
> >
> > v2:
> > - Clarify the commit log parts about power cost measurement and the
> > problem of simply removing/throttling debug tracing. (Chris)
> > - Optimize out local wakeref vars at intel_runtime_pm_put_raw() and
> > intel_display_power_put_async() call sites if
> > CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris)
> > - Rebased on v2 of the wakeref w/o power-on guarantee patch.
> > - Add missing docbook headers.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 5 +
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 362 +++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +
> > 3 files changed, 368 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d0257808734c..5801f5407589 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -834,6 +834,11 @@ struct i915_power_domains {
> >
> > struct mutex lock;
> > int domain_use_count[POWER_DOMAIN_NUM];
> > +
> > + struct delayed_work async_put_work;
> > + intel_wakeref_t async_put_wakeref;
> > + u64 async_put_domains[2];
>
> Fwiw, in the forcewake code we used the term 'auto' for the extra
> reference held by the timer for the delayed released.
Yep, I remember now that API and I just realized now in what it differs
from this one: it will put the ref automatically after use:) The power
domain users have to do it manually.. So not sure which term is better
here, I can also go with s/async_put/auto_put/, but then do it for the
whole API for consistency.
>
> There we used a timer per fw_domain, which are much fewer in number than
> power_domains!, but that has the advantage of avoiding a clock
> algorithm.
Yep, 8 vs. 40 something, and more seem to be coming:/
>
> > +
> > struct i915_power_well *power_wells;
>
> Would stuffing a timer into each power well be simpler?
I considered it, it would be a bit simpler, but we'd still need most of
the logic (grab the ref from a pending async put, flush logic). We could
do away with the requeuing and async_put_domains[0/1] selection, but
that's not too much imo.
I chose this way, since I didn't like the idea of multiple work items
in-flight (and then those contending for the mutex).
> Just wondering. Also wondering how much alike we could make the async
> released and if we can make a small library.
One difference is that we need a mutex and so a work due to VLV/CHV,
whereas the forcewake code can do only with a timer. But maybe VLV/CHV
could be special cased. Sharing things more would be a good thing in any
case.
>
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 89d6309bb1f7..829d483d27e9 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -60,6 +60,19 @@
> > * present for a given platform.
> > */
> >
> > +static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915);
> > +static void
> > +__intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref,
> > + bool wakelock);
> > +
> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > +static void
> > +intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref);
> > +#else
> > +#define intel_runtime_pm_put_raw(i915, wref) \
> > + __intel_runtime_pm_put(i915, -1, false)
> > +#endif
> > +
> > #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> >
> > #include <linux/sort.h>
> > @@ -1903,6 +1916,125 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> > chv_set_pipe_power_well(dev_priv, power_well, false);
> > }
> >
> > +static u64 __async_put_domains_mask(struct i915_power_domains *power_domains)
> > +{
> > + return power_domains->async_put_domains[0] |
> > + power_domains->async_put_domains[1];
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > +
> > +static bool
> > +assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
> > +{
> > + return !WARN_ON(power_domains->async_put_domains[0] &
> > + power_domains->async_put_domains[1]);
> > +}
> > +
> > +static bool
> > +__async_put_domains_state_ok(struct i915_power_domains *power_domains)
> > +{
> > + enum intel_display_power_domain domain;
> > + bool err = false;
> > +
> > + err |= !assert_async_put_domain_masks_disjoint(power_domains);
> > + err |= WARN_ON(!!power_domains->async_put_wakeref !=
> > + !!__async_put_domains_mask(power_domains));
> > +
> > + for_each_power_domain(domain, __async_put_domains_mask(power_domains))
> > + err |= WARN_ON(power_domains->domain_use_count[domain] != 1);
> > +
> > + return !err;
> > +}
> > +
> > +static void print_power_domains(struct i915_power_domains *power_domains,
> > + const char *prefix, u64 mask)
> > +{
> > + enum intel_display_power_domain domain;
> > +
> > + DRM_DEBUG_DRIVER("%s (%lu):\n", prefix, hweight64(mask));
> > + for_each_power_domain(domain, mask)
> > + DRM_DEBUG_DRIVER("%s use_count %d\n",
> > + intel_display_power_domain_str(domain),
> > + power_domains->domain_use_count[domain]);
> > +}
> > +
> > +static void
> > +print_async_put_domains_state(struct i915_power_domains *power_domains)
> > +{
> > + DRM_DEBUG_DRIVER("async_put_wakeref %u\n",
> > + power_domains->async_put_wakeref);
> > +
> > + print_power_domains(power_domains, "async_put_domains[0]",
> > + power_domains->async_put_domains[0]);
> > + print_power_domains(power_domains, "async_put_domains[1]",
> > + power_domains->async_put_domains[1]);
> > +}
> > +
> > +static void
> > +verify_async_put_domains_state(struct i915_power_domains *power_domains)
> > +{
> > + if (!__async_put_domains_state_ok(power_domains))
> > + print_async_put_domains_state(power_domains);
> > +}
> > +
> > +#else
> > +
> > +static void
> > +assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
> > +{
> > +}
> > +
> > +static void
> > +verify_async_put_domains_state(struct i915_power_domains *power_domains)
> > +{
> > +}
> > +
> > +#endif /* CONFIG_DRM_I915_DEBUG_RUNTIME_PM */
> > +
> > +static u64 async_put_domains_mask(struct i915_power_domains *power_domains)
> > +{
> > + assert_async_put_domain_masks_disjoint(power_domains);
> > +
> > + return __async_put_domains_mask(power_domains);
> > +}
> > +
> > +static void
> > +async_put_domains_clear_domain(struct i915_power_domains *power_domains,
> > + enum intel_display_power_domain domain)
> > +{
> > + assert_async_put_domain_masks_disjoint(power_domains);
> > +
> > + power_domains->async_put_domains[0] &= ~BIT_ULL(domain);
> > + power_domains->async_put_domains[1] &= ~BIT_ULL(domain);
> > +}
> > +
> > +static bool
> > +intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
> > + enum intel_display_power_domain domain)
> > +{
> > + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > + bool ret = false;
> > +
> > + if (!(async_put_domains_mask(power_domains) & BIT_ULL(domain)))
> > + goto out_verify;
> > +
> > + async_put_domains_clear_domain(power_domains, domain);
> > +
> > + ret = true;
> > +
> > + if (async_put_domains_mask(power_domains))
> > + goto out_verify;
> > +
> > + cancel_delayed_work(&power_domains->async_put_work);
> > + intel_runtime_pm_put_raw(dev_priv,
> > + fetch_and_zero(&power_domains->async_put_wakeref));
> > +out_verify:
> > + verify_async_put_domains_state(power_domains);
> > +
> > + return ret;
> > +}
> > +
> > static void
> > __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> > enum intel_display_power_domain domain)
> > @@ -1910,6 +2042,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> > struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > struct i915_power_well *power_well;
> >
> > + if (intel_display_power_grab_async_put_ref(dev_priv, domain))
> > + return;
> > +
> > for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
> > intel_power_well_get(dev_priv, power_well);
> >
> > @@ -1935,9 +2070,7 @@ intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
> > intel_wakeref_t wakeref = intel_runtime_pm_get(dev_priv);
> >
> > mutex_lock(&power_domains->lock);
> > -
> > __intel_display_power_get_domain(dev_priv, domain);
> > -
> > mutex_unlock(&power_domains->lock);
> >
> > return wakeref;
> > @@ -1986,24 +2119,36 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> > return wakeref;
> > }
> >
> > -static void __intel_display_power_put(struct drm_i915_private *dev_priv,
> > - enum intel_display_power_domain domain)
> > +static void
> > +__intel_display_power_put_domain(struct drm_i915_private *dev_priv,
> > + enum intel_display_power_domain domain)
> > {
> > struct i915_power_domains *power_domains;
> > struct i915_power_well *power_well;
> > + const char *name = intel_display_power_domain_str(domain);
> >
> > power_domains = &dev_priv->power_domains;
> >
> > - mutex_lock(&power_domains->lock);
> > -
> > WARN(!power_domains->domain_use_count[domain],
> > "Use count on domain %s is already zero\n",
> > - intel_display_power_domain_str(domain));
> > + name);
> > + WARN(async_put_domains_mask(power_domains) & BIT_ULL(domain),
> > + "Async disabling of domain %s is pending\n",
> > + name);
> > +
> > power_domains->domain_use_count[domain]--;
> >
> > for_each_power_domain_well_reverse(dev_priv, power_well, BIT_ULL(domain))
> > intel_power_well_put(dev_priv, power_well);
> > +}
> >
> > +static void __intel_display_power_put(struct drm_i915_private *dev_priv,
> > + enum intel_display_power_domain domain)
> > +{
> > + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +
> > + mutex_lock(&power_domains->lock);
> > + __intel_display_power_put_domain(dev_priv, domain);
> > mutex_unlock(&power_domains->lock);
> > }
> >
> > @@ -2027,6 +2172,187 @@ void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
> > intel_runtime_pm_put_unchecked(dev_priv);
> > }
> >
> > +static void
> > +queue_async_put_domains_work(struct i915_power_domains *power_domains,
> > + intel_wakeref_t wakeref)
> > +{
> > + WARN_ON(power_domains->async_put_wakeref);
> > + power_domains->async_put_wakeref = wakeref;
> > + WARN_ON(!queue_delayed_work(system_unbound_wq,
> > + &power_domains->async_put_work,
> > + msecs_to_jiffies(100)));
> > +}
> > +
> > +static void
> > +release_async_put_domains(struct i915_power_domains *power_domains, u64 mask)
> > +{
> > + struct drm_i915_private *dev_priv =
> > + container_of(power_domains, struct drm_i915_private,
> > + power_domains);
> > + enum intel_display_power_domain domain;
> > + intel_wakeref_t wakeref;
> > +
> > + /*
> > + * The caller must hold already raw wakeref, upgrade that to a proper
> > + * wakeref to make the state checker happy about the HW access during
> > + * power well disabling.
> > + */
> > + assert_rpm_raw_wakeref_held(dev_priv);
> > + wakeref = intel_runtime_pm_get(dev_priv);
> > +
> > + for_each_power_domain(domain, mask) {
> > + /* Clear before put, so put's sanity check is happy. */
> > + async_put_domains_clear_domain(power_domains, domain);
> > + __intel_display_power_put_domain(dev_priv, domain);
> > + }
> > +
> > + intel_runtime_pm_put(dev_priv, wakeref);
> > +}
> > +
> > +static void
> > +intel_display_power_put_async_work(struct work_struct *work)
> > +{
> > + struct drm_i915_private *dev_priv =
> > + container_of(work, struct drm_i915_private,
> > + power_domains.async_put_work.work);
> > + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > + intel_wakeref_t new_work_wakeref = intel_runtime_pm_get_raw(dev_priv);
> > + intel_wakeref_t old_work_wakeref = 0;
> > +
> > + mutex_lock(&power_domains->lock);
> > +
> > + /*
> > + * Bail out if all the domain refs pending to be released were grabbed
> > + * by subsequent gets or a flush_work.
> > + */
> > + old_work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
> > + if (!old_work_wakeref)
> > + goto out_verify;
> > +
> > + release_async_put_domains(power_domains,
> > + power_domains->async_put_domains[0]);
> > +
> > + /* Requeue the work if more domains were async put meanwhile. */
> > + if (power_domains->async_put_domains[1]) {
> > + power_domains->async_put_domains[0] =
> > + fetch_and_zero(&power_domains->async_put_domains[1]);
> > + queue_async_put_domains_work(power_domains,
> > + fetch_and_zero(&new_work_wakeref));
> > + }
> > +
> > +out_verify:
> > + verify_async_put_domains_state(power_domains);
> > +
> > + mutex_unlock(&power_domains->lock);
> > +
> > + if (old_work_wakeref)
> > + intel_runtime_pm_put_raw(dev_priv, old_work_wakeref);
> > + if (new_work_wakeref)
> > + intel_runtime_pm_put_raw(dev_priv, new_work_wakeref);
> > +}
> > +/**
> > + * intel_display_power_put_async - release a power domain reference asynchronously
> > + * @i915: 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 schedules a work to power down the
> > + * corresponding hardware block if this is the last reference.
> > + */
> > +void __intel_display_power_put_async(struct drm_i915_private *i915,
> > + enum intel_display_power_domain domain,
> > + intel_wakeref_t wakeref)
> > +{
> > + struct i915_power_domains *power_domains = &i915->power_domains;
> > + intel_wakeref_t work_wakeref = intel_runtime_pm_get_raw(i915);
> > +
> > + mutex_lock(&power_domains->lock);
> > +
> > + if (power_domains->domain_use_count[domain] > 1) {
> > + __intel_display_power_put_domain(i915, domain);
> > +
> > + goto out_verify;
> > + }
> > +
> > + WARN_ON(power_domains->domain_use_count[domain] != 1);
> > +
> > + /* Let a pending work requeue itself or queue a new one. */
> > + if (power_domains->async_put_wakeref) {
> > + power_domains->async_put_domains[1] |= BIT_ULL(domain);
> > + } else {
> > + power_domains->async_put_domains[0] |= BIT_ULL(domain);
> > + queue_async_put_domains_work(power_domains,
> > + fetch_and_zero(&work_wakeref));
> > + }
> > +
> > +out_verify:
> > + verify_async_put_domains_state(power_domains);
> > +
> > + mutex_unlock(&power_domains->lock);
> > +
> > + if (work_wakeref)
> > + intel_runtime_pm_put_raw(i915, work_wakeref);
> > +
> > + intel_runtime_pm_put(i915, wakeref);
> > +}
> > +
> > +/**
> > + * intel_display_power_flush_work - flushes the async display power disabling work
> > + * @i915: i915 device instance
> > + *
> > + * Flushes any pending work that was scheduled by a preceeding
> > + * intel_display_power_put_async() call, completing the disabling of the
> > + * corresponding power domains.
> > + *
> > + * Note that the work handler function may still be running after this
> > + * function returns; to ensure that the work handler isn't running use
> > + * intel_display_power_flush_work_sync() instead.
> > + */
> > +void intel_display_power_flush_work(struct drm_i915_private *i915)
> > +{
> > + struct i915_power_domains *power_domains = &i915->power_domains;
> > + intel_wakeref_t work_wakeref;
> > +
> > + mutex_lock(&power_domains->lock);
> > +
> > + work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
> > + if (!work_wakeref)
> > + goto out_verify;
> > +
> > + release_async_put_domains(power_domains,
> > + async_put_domains_mask(power_domains));
> > + cancel_delayed_work(&power_domains->async_put_work);
> > +
> > +out_verify:
> > + verify_async_put_domains_state(power_domains);
> > +
> > + mutex_unlock(&power_domains->lock);
> > +
> > + if (work_wakeref)
> > + intel_runtime_pm_put_raw(i915, work_wakeref);
> > +}
> > +
> > +/**
> > + * intel_display_power_flush_work_sync - flushes and syncs the async display power disabling work
> > + * @i915: i915 device instance
> > + *
> > + * Like intel_display_power_flush_work(), but also ensure that the work
> > + * handler function is not running any more when this function returns.
> > + */
> > +static void
> > +intel_display_power_flush_work_sync(struct drm_i915_private *dev_priv)
> > +{
> > + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +
> > + intel_display_power_flush_work(dev_priv);
> > + cancel_delayed_work_sync(&power_domains->async_put_work);
> > +
> > + verify_async_put_domains_state(power_domains);
> > +
> > + WARN_ON(power_domains->async_put_wakeref);
> > +}
> > +
> > #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > /**
> > * intel_display_power_put - release a power domain reference
> > @@ -3525,6 +3851,9 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> >
> > mutex_init(&power_domains->lock);
> >
> > + INIT_DELAYED_WORK(&power_domains->async_put_work,
> > + intel_display_power_put_async_work);
> > +
> > /*
> > * The enabling order will be from lower to higher indexed wells,
> > * the disabling order is reversed.
> > @@ -4445,6 +4774,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *i915)
> > if (!i915_modparams.disable_power_well)
> > intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
> >
> > + intel_display_power_flush_work_sync(i915);
> > +
> > intel_power_domains_verify_state(i915);
> >
> > /* Keep the power well enabled, but cancel its rpm wakeref. */
> > @@ -4520,6 +4851,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
> > if (!(i915->csr.allowed_dc_mask & DC_STATE_EN_DC9) &&
> > suspend_mode == I915_DRM_SUSPEND_IDLE &&
> > i915->csr.dmc_payload) {
> > + intel_display_power_flush_work(i915);
> > intel_power_domains_verify_state(i915);
> > return;
> > }
> > @@ -4531,6 +4863,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
> > if (!i915_modparams.disable_power_well)
> > intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
> >
> > + intel_display_power_flush_work(i915);
> > intel_power_domains_verify_state(i915);
> >
> > if (INTEL_GEN(i915) >= 11)
> > @@ -4609,6 +4942,8 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
> >
> > mutex_lock(&power_domains->lock);
> >
> > + verify_async_put_domains_state(power_domains);
> > +
> > dump_domain_info = false;
> > for_each_power_well(i915, power_well) {
> > enum intel_display_power_domain domain;
> > @@ -4670,6 +5005,11 @@ static intel_wakeref_t __intel_runtime_pm_get(struct drm_i915_private *i915,
> > return track_intel_runtime_pm_wakeref(i915);
> > }
> >
> > +static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915)
> > +{
> > + return __intel_runtime_pm_get(i915, false);
> > +}
> > +
> > /**
> > * intel_runtime_pm_get - grab a runtime pm reference
> > * @i915: i915 device instance
> > @@ -4769,6 +5109,14 @@ static void __intel_runtime_pm_put(struct drm_i915_private *i915,
> > pm_runtime_put_autosuspend(kdev);
> > }
> >
> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > +static void
> > +intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref)
> > +{
> > + __intel_runtime_pm_put(i915, wref, false);
> > +}
> > +#endif
> > +
> > /**
> > * intel_runtime_pm_put_unchecked - release an unchecked runtime pm reference
> > * @i915: i915 device instance
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index e30b38632bd2..a7a3d929546f 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -59,13 +59,21 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> > enum intel_display_power_domain domain);
> > void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
> > enum intel_display_power_domain domain);
> > +void __intel_display_power_put_async(struct drm_i915_private *i915,
> > + enum intel_display_power_domain domain,
> > + intel_wakeref_t wakeref);
> > +void intel_display_power_flush_work(struct drm_i915_private *i915);
> > #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> > void intel_display_power_put(struct drm_i915_private *dev_priv,
> > enum intel_display_power_domain domain,
> > intel_wakeref_t wakeref);
> > +#define intel_display_power_put_async(i915, domain, wakeref) \
> > + __intel_display_power_put_async(i915, domain, wakeref)
> > #else
> > #define intel_display_power_put(i915, domain, wakeref) \
> > intel_display_power_put_unchecked(i915, domain)
> > +#define intel_display_power_put_async(i915, domain, wakeref) \
> > + __intel_display_power_put_async(i915, domain, -1)
> > #endif
> > void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> > u8 req_slices);
>
> Seems like it serves a purpose and does it well, with the only question
> on how we can share similar code,
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks!
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 05/11] drm/i915: Disable power asynchronously during DP AUX transfers
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (3 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 6:19 ` [PATCH v2 06/11] drm/i915: WARN for eDP encoders in intel_dp_detect_dpcd() Imre Deak
` (9 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
In a follow-up patch we will restrict holding the reference on the AUX
power domain to the AUX transfer function. To avoid the unnecessary
on-off-on power togglings drop the reference asynchronously.
There is no reason we couldn't do this in general and also put the
reference asynchronously in pps_unlock(); but that's a separate change
that can be done as a follow-up.
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 53cc4afea256..700ceacb82e6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1221,7 +1221,10 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
to_i915(intel_dig_port->base.base.dev);
i915_reg_t ch_ctl, ch_data[5];
u32 aux_clock_divider;
- intel_wakeref_t wakeref;
+ enum intel_display_power_domain aux_domain =
+ intel_aux_power_domain(intel_dig_port);
+ intel_wakeref_t aux_wakeref;
+ intel_wakeref_t pps_wakeref;
int i, ret, recv_bytes;
int try, clock = 0;
u32 status;
@@ -1231,7 +1234,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
for (i = 0; i < ARRAY_SIZE(ch_data); i++)
ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
- wakeref = pps_lock(intel_dp);
+ aux_wakeref = intel_display_power_get(dev_priv, aux_domain);
+ pps_wakeref = pps_lock(intel_dp);
/*
* We will be called with VDD already enabled for dpcd/edid/oui reads.
@@ -1377,7 +1381,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
if (vdd)
edp_panel_vdd_off(intel_dp, false);
- pps_unlock(intel_dp, wakeref);
+ pps_unlock(intel_dp, pps_wakeref);
+ intel_display_power_put_async(dev_priv, aux_domain, aux_wakeref);
return ret;
}
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 06/11] drm/i915: WARN for eDP encoders in intel_dp_detect_dpcd()
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (4 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 05/11] drm/i915: Disable power asynchronously during DP AUX transfers Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 6:19 ` [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect() Imre Deak
` (8 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
We are not calling this function for eDP, so add an early assert about
this for clarity.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 700ceacb82e6..1e011998e9d5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4844,15 +4844,15 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
u8 *dpcd = intel_dp->dpcd;
u8 type;
+ if (WARN_ON(intel_dp_is_edp(intel_dp)))
+ return connector_status_connected;
+
if (lspcon->active)
lspcon_resume(lspcon);
if (!intel_dp_get_dpcd(intel_dp))
return connector_status_disconnected;
- if (intel_dp_is_edp(intel_dp))
- return connector_status_connected;
-
/* if there's no downstream port, we're done */
if (!drm_dp_is_branch(dpcd))
return connector_status_connected;
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (5 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 06/11] drm/i915: WARN for eDP encoders in intel_dp_detect_dpcd() Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 15:57 ` Ville Syrjälä
2019-05-09 6:19 ` [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse() Imre Deak
` (7 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
We don't need the AUX power for the whole duration of the detect, only
when we're doing AUX transfers. The AUX transfer function takes its own
reference on the AUX power domain already. The two places during detect
which access display core registers (not specific to a
pipe/port/transcoder) only need the power domain that is required for
that access. That power domain is equivalent to the device global power
domain on most platforms (enabled whenever we hold a runtime PM
reference) except on CHV/VLV where it's equivalent to the display power
well.
Add a new power domain that reflects the above, and use this at the two
spots accessing registers. With that we can avoid taking the AUX
reference for the whole duration of the detect function.
Put the domains asynchronously to avoid the unneeded on-off-on toggling.
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_display.h | 1 +
drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++--------
drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++++
3 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 1b6f5a71184d..7f3fafdfbd5f 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -220,6 +220,7 @@ enum aux_ch {
#define aux_ch_name(a) ((a) + 'A')
enum intel_display_power_domain {
+ POWER_DOMAIN_DISPLAY_CORE,
POWER_DOMAIN_PIPE_A,
POWER_DOMAIN_PIPE_B,
POWER_DOMAIN_PIPE_C,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1e011998e9d5..24cca12a9a3e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -216,15 +216,21 @@ static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+ intel_wakeref_t wakeref;
u32 lane_info;
if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
return 4;
+ wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
+
lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
DP_LANE_ASSIGNMENT_SHIFT(tc_port);
+ intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
+ wakeref);
+
switch (lane_info) {
default:
MISSING_CASE(lane_info);
@@ -5294,7 +5300,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
*
* Return %true if port is connected, %false otherwise.
*/
-bool intel_digital_port_connected(struct intel_encoder *encoder)
+static bool __intel_digital_port_connected(struct intel_encoder *encoder)
{
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -5324,6 +5330,20 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
return false;
}
+bool intel_digital_port_connected(struct intel_encoder *encoder)
+{
+ struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+ intel_wakeref_t wakeref;
+ bool res;
+
+ wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
+ res = __intel_digital_port_connected(encoder);
+ intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
+ wakeref);
+
+ return res;
+}
+
static struct edid *
intel_dp_get_edid(struct intel_dp *intel_dp)
{
@@ -5377,16 +5397,11 @@ intel_dp_detect(struct drm_connector *connector,
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct intel_encoder *encoder = &dig_port->base;
enum drm_connector_status status;
- enum intel_display_power_domain aux_domain =
- intel_aux_power_domain(dig_port);
- intel_wakeref_t wakeref;
DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
connector->base.id, connector->name);
WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
- wakeref = intel_display_power_get(dev_priv, aux_domain);
-
/* Can't disconnect eDP */
if (intel_dp_is_edp(intel_dp))
status = edp_detect(intel_dp);
@@ -5450,10 +5465,8 @@ intel_dp_detect(struct drm_connector *connector,
int ret;
ret = intel_dp_retrain_link(encoder, ctx);
- if (ret) {
- intel_display_power_put(dev_priv, aux_domain, wakeref);
+ if (ret)
return ret;
- }
}
/*
@@ -5475,7 +5488,6 @@ intel_dp_detect(struct drm_connector *connector,
if (status != connector_status_connected && !intel_dp->is_mst)
intel_dp_unset_edid(intel_dp);
- intel_display_power_put(dev_priv, aux_domain, wakeref);
return status;
}
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 829d483d27e9..618dd42f1b2d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -406,6 +406,8 @@ const char *
intel_display_power_domain_str(enum intel_display_power_domain domain)
{
switch (domain) {
+ case POWER_DOMAIN_DISPLAY_CORE:
+ return "DISPLAY_CORE";
case POWER_DOMAIN_PIPE_A:
return "PIPE_A";
case POWER_DOMAIN_PIPE_B:
@@ -2383,6 +2385,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
BIT_ULL(POWER_DOMAIN_INIT))
#define VLV_DISPLAY_POWER_DOMAINS ( \
+ BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
BIT_ULL(POWER_DOMAIN_PIPE_A) | \
BIT_ULL(POWER_DOMAIN_PIPE_B) | \
BIT_ULL(POWER_DOMAIN_PIPE_A_PANEL_FITTER) | \
@@ -2429,6 +2432,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
BIT_ULL(POWER_DOMAIN_INIT))
#define CHV_DISPLAY_POWER_DOMAINS ( \
+ BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
BIT_ULL(POWER_DOMAIN_PIPE_A) | \
BIT_ULL(POWER_DOMAIN_PIPE_B) | \
BIT_ULL(POWER_DOMAIN_PIPE_C) | \
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()
2019-05-09 6:19 ` [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect() Imre Deak
@ 2019-05-09 15:57 ` Ville Syrjälä
2019-05-09 16:10 ` Imre Deak
0 siblings, 1 reply; 26+ messages in thread
From: Ville Syrjälä @ 2019-05-09 15:57 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Thu, May 09, 2019 at 09:19:50AM +0300, Imre Deak wrote:
> We don't need the AUX power for the whole duration of the detect, only
> when we're doing AUX transfers. The AUX transfer function takes its own
> reference on the AUX power domain already. The two places during detect
> which access display core registers (not specific to a
> pipe/port/transcoder) only need the power domain that is required for
> that access. That power domain is equivalent to the device global power
> domain on most platforms (enabled whenever we hold a runtime PM
> reference) except on CHV/VLV where it's equivalent to the display power
> well.
>
> Add a new power domain that reflects the above, and use this at the two
> spots accessing registers. With that we can avoid taking the AUX
> reference for the whole duration of the detect function.
>
> Put the domains asynchronously to avoid the unneeded on-off-on toggling.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.h | 1 +
> drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++--------
> drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++++
> 3 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 1b6f5a71184d..7f3fafdfbd5f 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -220,6 +220,7 @@ enum aux_ch {
> #define aux_ch_name(a) ((a) + 'A')
>
> enum intel_display_power_domain {
> + POWER_DOMAIN_DISPLAY_CORE,
> POWER_DOMAIN_PIPE_A,
> POWER_DOMAIN_PIPE_B,
> POWER_DOMAIN_PIPE_C,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1e011998e9d5..24cca12a9a3e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -216,15 +216,21 @@ static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> + intel_wakeref_t wakeref;
> u32 lane_info;
>
> if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> return 4;
>
> + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> +
> lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> DP_LANE_ASSIGNMENT_SHIFT(tc_port);
>
> + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> + wakeref);
> +
Future idea: maybe we want a with_something() {} for this sort of thing?
> switch (lane_info) {
> default:
> MISSING_CASE(lane_info);
Unrelated but this thing here looks like a hand rolled hweight(). And
for some reason it's using decimal for bitmasks.
> @@ -5294,7 +5300,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
> *
> * Return %true if port is connected, %false otherwise.
> */
> -bool intel_digital_port_connected(struct intel_encoder *encoder)
> +static bool __intel_digital_port_connected(struct intel_encoder *encoder)
> {
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>
> @@ -5324,6 +5330,20 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
> return false;
> }
>
> +bool intel_digital_port_connected(struct intel_encoder *encoder)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + intel_wakeref_t wakeref;
> + bool res;
bool is_connected perhaps?
> +
> + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> + res = __intel_digital_port_connected(encoder);
> + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> + wakeref);
> +
> + return res;
> +}
> +
> static struct edid *
> intel_dp_get_edid(struct intel_dp *intel_dp)
> {
> @@ -5377,16 +5397,11 @@ intel_dp_detect(struct drm_connector *connector,
> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> struct intel_encoder *encoder = &dig_port->base;
> enum drm_connector_status status;
> - enum intel_display_power_domain aux_domain =
> - intel_aux_power_domain(dig_port);
> - intel_wakeref_t wakeref;
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> connector->base.id, connector->name);
> WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>
> - wakeref = intel_display_power_get(dev_priv, aux_domain);
> -
> /* Can't disconnect eDP */
> if (intel_dp_is_edp(intel_dp))
> status = edp_detect(intel_dp);
> @@ -5450,10 +5465,8 @@ intel_dp_detect(struct drm_connector *connector,
> int ret;
>
> ret = intel_dp_retrain_link(encoder, ctx);
> - if (ret) {
> - intel_display_power_put(dev_priv, aux_domain, wakeref);
> + if (ret)
> return ret;
> - }
> }
>
> /*
> @@ -5475,7 +5488,6 @@ intel_dp_detect(struct drm_connector *connector,
> if (status != connector_status_connected && !intel_dp->is_mst)
> intel_dp_unset_edid(intel_dp);
>
> - intel_display_power_put(dev_priv, aux_domain, wakeref);
> return status;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 829d483d27e9..618dd42f1b2d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -406,6 +406,8 @@ const char *
> intel_display_power_domain_str(enum intel_display_power_domain domain)
> {
> switch (domain) {
> + case POWER_DOMAIN_DISPLAY_CORE:
> + return "DISPLAY_CORE";
> case POWER_DOMAIN_PIPE_A:
> return "PIPE_A";
> case POWER_DOMAIN_PIPE_B:
> @@ -2383,6 +2385,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> BIT_ULL(POWER_DOMAIN_INIT))
>
> #define VLV_DISPLAY_POWER_DOMAINS ( \
> + BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
> BIT_ULL(POWER_DOMAIN_PIPE_A) | \
> BIT_ULL(POWER_DOMAIN_PIPE_B) | \
> BIT_ULL(POWER_DOMAIN_PIPE_A_PANEL_FITTER) | \
> @@ -2429,6 +2432,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> BIT_ULL(POWER_DOMAIN_INIT))
>
> #define CHV_DISPLAY_POWER_DOMAINS ( \
> + BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
> BIT_ULL(POWER_DOMAIN_PIPE_A) | \
> BIT_ULL(POWER_DOMAIN_PIPE_B) | \
> BIT_ULL(POWER_DOMAIN_PIPE_C) | \
> --
> 2.17.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()
2019-05-09 15:57 ` Ville Syrjälä
@ 2019-05-09 16:10 ` Imre Deak
0 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 16:10 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Thu, May 09, 2019 at 06:57:56PM +0300, Ville Syrjälä wrote:
> On Thu, May 09, 2019 at 09:19:50AM +0300, Imre Deak wrote:
> > We don't need the AUX power for the whole duration of the detect, only
> > when we're doing AUX transfers. The AUX transfer function takes its own
> > reference on the AUX power domain already. The two places during detect
> > which access display core registers (not specific to a
> > pipe/port/transcoder) only need the power domain that is required for
> > that access. That power domain is equivalent to the device global power
> > domain on most platforms (enabled whenever we hold a runtime PM
> > reference) except on CHV/VLV where it's equivalent to the display power
> > well.
> >
> > Add a new power domain that reflects the above, and use this at the two
> > spots accessing registers. With that we can avoid taking the AUX
> > reference for the whole duration of the detect function.
> >
> > Put the domains asynchronously to avoid the unneeded on-off-on toggling.
> >
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.h | 1 +
> > drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++--------
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++++
> > 3 files changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 1b6f5a71184d..7f3fafdfbd5f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -220,6 +220,7 @@ enum aux_ch {
> > #define aux_ch_name(a) ((a) + 'A')
> >
> > enum intel_display_power_domain {
> > + POWER_DOMAIN_DISPLAY_CORE,
> > POWER_DOMAIN_PIPE_A,
> > POWER_DOMAIN_PIPE_B,
> > POWER_DOMAIN_PIPE_C,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 1e011998e9d5..24cca12a9a3e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -216,15 +216,21 @@ static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
> > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > + intel_wakeref_t wakeref;
> > u32 lane_info;
> >
> > if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> > return 4;
> >
> > + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > +
> > lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> > DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> > DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> >
> > + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > + wakeref);
> > +
>
> Future idea: maybe we want a with_something() {} for this sort of thing?
Yep, good idea, can add that in this patch.
>
> > switch (lane_info) {
> > default:
> > MISSING_CASE(lane_info);
>
> Unrelated but this thing here looks like a hand rolled hweight(). And
> for some reason it's using decimal for bitmasks.
Yep, that's strange. I can follow up fixing that if noone is
interested.
>
> > @@ -5294,7 +5300,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
> > *
> > * Return %true if port is connected, %false otherwise.
> > */
> > -bool intel_digital_port_connected(struct intel_encoder *encoder)
> > +static bool __intel_digital_port_connected(struct intel_encoder *encoder)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >
> > @@ -5324,6 +5330,20 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
> > return false;
> > }
> >
> > +bool intel_digital_port_connected(struct intel_encoder *encoder)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + intel_wakeref_t wakeref;
> > + bool res;
>
> bool is_connected perhaps?
Ok.
>
> > +
> > + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > + res = __intel_digital_port_connected(encoder);
> > + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > + wakeref);
> > +
> > + return res;
> > +}
> > +
> > static struct edid *
> > intel_dp_get_edid(struct intel_dp *intel_dp)
> > {
> > @@ -5377,16 +5397,11 @@ intel_dp_detect(struct drm_connector *connector,
> > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > struct intel_encoder *encoder = &dig_port->base;
> > enum drm_connector_status status;
> > - enum intel_display_power_domain aux_domain =
> > - intel_aux_power_domain(dig_port);
> > - intel_wakeref_t wakeref;
> >
> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > connector->base.id, connector->name);
> > WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> >
> > - wakeref = intel_display_power_get(dev_priv, aux_domain);
> > -
> > /* Can't disconnect eDP */
> > if (intel_dp_is_edp(intel_dp))
> > status = edp_detect(intel_dp);
> > @@ -5450,10 +5465,8 @@ intel_dp_detect(struct drm_connector *connector,
> > int ret;
> >
> > ret = intel_dp_retrain_link(encoder, ctx);
> > - if (ret) {
> > - intel_display_power_put(dev_priv, aux_domain, wakeref);
> > + if (ret)
> > return ret;
> > - }
> > }
> >
> > /*
> > @@ -5475,7 +5488,6 @@ intel_dp_detect(struct drm_connector *connector,
> > if (status != connector_status_connected && !intel_dp->is_mst)
> > intel_dp_unset_edid(intel_dp);
> >
> > - intel_display_power_put(dev_priv, aux_domain, wakeref);
> > return status;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 829d483d27e9..618dd42f1b2d 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -406,6 +406,8 @@ const char *
> > intel_display_power_domain_str(enum intel_display_power_domain domain)
> > {
> > switch (domain) {
> > + case POWER_DOMAIN_DISPLAY_CORE:
> > + return "DISPLAY_CORE";
> > case POWER_DOMAIN_PIPE_A:
> > return "PIPE_A";
> > case POWER_DOMAIN_PIPE_B:
> > @@ -2383,6 +2385,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > BIT_ULL(POWER_DOMAIN_INIT))
> >
> > #define VLV_DISPLAY_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_A) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_B) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_A_PANEL_FITTER) | \
> > @@ -2429,6 +2432,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > BIT_ULL(POWER_DOMAIN_INIT))
> >
> > #define CHV_DISPLAY_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_A) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_B) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_C) | \
> > --
> > 2.17.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (6 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect() Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 15:50 ` Ville Syrjälä
2019-05-09 6:19 ` [PATCH v2 09/11] drm/i915: Replace use of PLLS power domain with DISPLAY_CORE domain Imre Deak
` (6 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
The power get/put was added in
commit 1c767b339b3938b19076ffdc9d70aa1e4235a45b
Author: Imre Deak <imre.deak@intel.com>
Date: Mon Aug 18 14:42:42 2014 +0300
drm/i915: take display port power domain in DP HPD handle
to account for the HW access in ibx_digital_port_connected(). This
latter call was in turn removed in
commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Date: Wed Mar 30 18:05:23 2016 +0530
drm/i915: Cleaning up intel_dp_hpd_pulse
after which we didn't actually need the power reference.
One way we are accessing the HW during HPD pulse handling is via DP AUX
transfers, but the transfer function takes its own reference, so doesn't
need the reference in intel_dp_hpd_pulse().
The other spot is in the PSR code doing register access, for that we can
use the DISPLAY_CORE power domain in a similar way done in the previous
patch.
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 20 ++++----------------
drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
2 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 24cca12a9a3e..de881bfea011 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6308,9 +6308,6 @@ enum irqreturn
intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
{
struct intel_dp *intel_dp = &intel_dig_port->dp;
- struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
- enum irqreturn ret = IRQ_NONE;
- intel_wakeref_t wakeref;
if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
/*
@@ -6333,9 +6330,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
return IRQ_NONE;
}
- wakeref = intel_display_power_get(dev_priv,
- intel_aux_power_domain(intel_dig_port));
-
if (intel_dp->is_mst) {
if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
/*
@@ -6347,7 +6341,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
intel_dp->is_mst = false;
drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
intel_dp->is_mst);
- goto put_power;
+
+ return IRQ_NONE;
}
}
@@ -6357,17 +6352,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
handled = intel_dp_short_pulse(intel_dp);
if (!handled)
- goto put_power;
+ return IRQ_NONE;
}
- ret = IRQ_HANDLED;
-
-put_power:
- intel_display_power_put(dev_priv,
- intel_aux_power_domain(intel_dig_port),
- wakeref);
-
- return ret;
+ return IRQ_HANDLED;
}
/* check the VBT to see whether the eDP is on another port */
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 963663ba0edf..856a39c7ee77 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1251,10 +1251,13 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
const u8 errors = DP_PSR_RFB_STORAGE_ERROR |
DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
DP_PSR_LINK_CRC_ERROR;
+ intel_wakeref_t wakeref;
if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
return;
+ wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
+
mutex_lock(&psr->lock);
if (!psr->enabled || psr->dp != intel_dp)
@@ -1294,6 +1297,9 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
exit:
mutex_unlock(&psr->lock);
+
+ intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
+ wakeref);
}
bool intel_psr_enabled(struct intel_dp *intel_dp)
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()
2019-05-09 6:19 ` [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse() Imre Deak
@ 2019-05-09 15:50 ` Ville Syrjälä
2019-05-09 16:06 ` Imre Deak
0 siblings, 1 reply; 26+ messages in thread
From: Ville Syrjälä @ 2019-05-09 15:50 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Thu, May 09, 2019 at 09:19:51AM +0300, Imre Deak wrote:
> The power get/put was added in
>
> commit 1c767b339b3938b19076ffdc9d70aa1e4235a45b
> Author: Imre Deak <imre.deak@intel.com>
> Date: Mon Aug 18 14:42:42 2014 +0300
>
> drm/i915: take display port power domain in DP HPD handle
>
> to account for the HW access in ibx_digital_port_connected(). This
> latter call was in turn removed in
>
> commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
> Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> Date: Wed Mar 30 18:05:23 2016 +0530
>
> drm/i915: Cleaning up intel_dp_hpd_pulse
>
> after which we didn't actually need the power reference.
>
> One way we are accessing the HW during HPD pulse handling is via DP AUX
> transfers, but the transfer function takes its own reference, so doesn't
> need the reference in intel_dp_hpd_pulse().
>
> The other spot is in the PSR code doing register access, for that we can
> use the DISPLAY_CORE power domain in a similar way done in the previous
> patch.
I don't think the PSR code should touch the hardware unless the crtc is
active. So not sure it really needs anything.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 20 ++++----------------
> drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
> 2 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 24cca12a9a3e..de881bfea011 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6308,9 +6308,6 @@ enum irqreturn
> intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> {
> struct intel_dp *intel_dp = &intel_dig_port->dp;
> - struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> - enum irqreturn ret = IRQ_NONE;
> - intel_wakeref_t wakeref;
>
> if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> /*
> @@ -6333,9 +6330,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> return IRQ_NONE;
> }
>
> - wakeref = intel_display_power_get(dev_priv,
> - intel_aux_power_domain(intel_dig_port));
> -
> if (intel_dp->is_mst) {
> if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> /*
> @@ -6347,7 +6341,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> intel_dp->is_mst = false;
> drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> intel_dp->is_mst);
> - goto put_power;
> +
> + return IRQ_NONE;
> }
> }
>
> @@ -6357,17 +6352,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> handled = intel_dp_short_pulse(intel_dp);
>
> if (!handled)
> - goto put_power;
> + return IRQ_NONE;
> }
>
> - ret = IRQ_HANDLED;
> -
> -put_power:
> - intel_display_power_put(dev_priv,
> - intel_aux_power_domain(intel_dig_port),
> - wakeref);
> -
> - return ret;
> + return IRQ_HANDLED;
> }
>
> /* check the VBT to see whether the eDP is on another port */
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 963663ba0edf..856a39c7ee77 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1251,10 +1251,13 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> const u8 errors = DP_PSR_RFB_STORAGE_ERROR |
> DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> DP_PSR_LINK_CRC_ERROR;
> + intel_wakeref_t wakeref;
>
> if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> return;
>
> + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> +
> mutex_lock(&psr->lock);
>
> if (!psr->enabled || psr->dp != intel_dp)
> @@ -1294,6 +1297,9 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
> exit:
> mutex_unlock(&psr->lock);
> +
> + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> + wakeref);
> }
>
> bool intel_psr_enabled(struct intel_dp *intel_dp)
> --
> 2.17.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()
2019-05-09 15:50 ` Ville Syrjälä
@ 2019-05-09 16:06 ` Imre Deak
0 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 16:06 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Thu, May 09, 2019 at 06:50:42PM +0300, Ville Syrjälä wrote:
> On Thu, May 09, 2019 at 09:19:51AM +0300, Imre Deak wrote:
> > The power get/put was added in
> >
> > commit 1c767b339b3938b19076ffdc9d70aa1e4235a45b
> > Author: Imre Deak <imre.deak@intel.com>
> > Date: Mon Aug 18 14:42:42 2014 +0300
> >
> > drm/i915: take display port power domain in DP HPD handle
> >
> > to account for the HW access in ibx_digital_port_connected(). This
> > latter call was in turn removed in
> >
> > commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
> > Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > Date: Wed Mar 30 18:05:23 2016 +0530
> >
> > drm/i915: Cleaning up intel_dp_hpd_pulse
> >
> > after which we didn't actually need the power reference.
> >
> > One way we are accessing the HW during HPD pulse handling is via DP AUX
> > transfers, but the transfer function takes its own reference, so doesn't
> > need the reference in intel_dp_hpd_pulse().
> >
> > The other spot is in the PSR code doing register access, for that we can
> > use the DISPLAY_CORE power domain in a similar way done in the previous
> > patch.
>
> I don't think the PSR code should touch the hardware unless the crtc is
> active. So not sure it really needs anything.
Ah, right, thanks for spotting it. I noticed the access in
intel_psr_short_pulse()->intel_psr_disable_locked()
only by code inspection, didn't think of checking the state in which it
can be called (even though the fn name should've been indicative). So
yes PSR is enabled at that point and the modeset should have already all
the power references needed for that access. I will remove the get/put
from intel_psr_short_pulse().
>
> >
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 20 ++++----------------
> > drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
> > 2 files changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 24cca12a9a3e..de881bfea011 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6308,9 +6308,6 @@ enum irqreturn
> > intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > {
> > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > - struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > - enum irqreturn ret = IRQ_NONE;
> > - intel_wakeref_t wakeref;
> >
> > if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> > /*
> > @@ -6333,9 +6330,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > return IRQ_NONE;
> > }
> >
> > - wakeref = intel_display_power_get(dev_priv,
> > - intel_aux_power_domain(intel_dig_port));
> > -
> > if (intel_dp->is_mst) {
> > if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> > /*
> > @@ -6347,7 +6341,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > intel_dp->is_mst = false;
> > drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > intel_dp->is_mst);
> > - goto put_power;
> > +
> > + return IRQ_NONE;
> > }
> > }
> >
> > @@ -6357,17 +6352,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > handled = intel_dp_short_pulse(intel_dp);
> >
> > if (!handled)
> > - goto put_power;
> > + return IRQ_NONE;
> > }
> >
> > - ret = IRQ_HANDLED;
> > -
> > -put_power:
> > - intel_display_power_put(dev_priv,
> > - intel_aux_power_domain(intel_dig_port),
> > - wakeref);
> > -
> > - return ret;
> > + return IRQ_HANDLED;
> > }
> >
> > /* check the VBT to see whether the eDP is on another port */
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 963663ba0edf..856a39c7ee77 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -1251,10 +1251,13 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> > const u8 errors = DP_PSR_RFB_STORAGE_ERROR |
> > DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> > DP_PSR_LINK_CRC_ERROR;
> > + intel_wakeref_t wakeref;
> >
> > if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> > return;
> >
> > + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > +
> > mutex_lock(&psr->lock);
> >
> > if (!psr->enabled || psr->dp != intel_dp)
> > @@ -1294,6 +1297,9 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
> > exit:
> > mutex_unlock(&psr->lock);
> > +
> > + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > + wakeref);
> > }
> >
> > bool intel_psr_enabled(struct intel_dp *intel_dp)
> > --
> > 2.17.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 09/11] drm/i915: Replace use of PLLS power domain with DISPLAY_CORE domain
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (7 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse() Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 6:19 ` [PATCH v2 10/11] drm/i915: Avoid taking the PPS lock for non-eDP/VLV/CHV Imre Deak
` (5 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
There isn't a separate power domain specific to PLLs. When programming
them we require the same power domain to be enabled which is needed when
accessing other display core parts (not specific to any
pipe/port/transcoder). This corresponds to the DISPLAY_CORE domain added
previously in this patchset, so use that instead to save bits in the
power domain mask.
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_display.h | 1 -
drivers/gpu/drm/i915/intel_dpll_mgr.c | 36 ++++++++++++-------------
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 --
4 files changed, 19 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a351c8e219ba..e08af706b72f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6363,7 +6363,7 @@ static u64 get_crtc_power_domains(struct drm_crtc *crtc,
mask |= BIT_ULL(POWER_DOMAIN_AUDIO);
if (crtc_state->shared_dpll)
- mask |= BIT_ULL(POWER_DOMAIN_PLLS);
+ mask |= BIT_ULL(POWER_DOMAIN_DISPLAY_CORE);
return mask;
}
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 7f3fafdfbd5f..41f2aa966abc 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -251,7 +251,6 @@ enum intel_display_power_domain {
POWER_DOMAIN_PORT_OTHER,
POWER_DOMAIN_VGA,
POWER_DOMAIN_AUDIO,
- POWER_DOMAIN_PLLS,
POWER_DOMAIN_AUX_A,
POWER_DOMAIN_AUX_B,
POWER_DOMAIN_AUX_C,
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index bf5e2541c35e..897d93537414 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -351,7 +351,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
u32 val;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -360,7 +360,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
hw_state->fp0 = I915_READ(PCH_FP0(id));
hw_state->fp1 = I915_READ(PCH_FP1(id));
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return val & DPLL_VCO_ENABLE;
}
@@ -519,14 +519,14 @@ static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv,
u32 val;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
val = I915_READ(WRPLL_CTL(id));
hw_state->wrpll = val;
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return val & WRPLL_PLL_ENABLE;
}
@@ -539,14 +539,14 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv,
u32 val;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
val = I915_READ(SPLL_CTL);
hw_state->spll = val;
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return val & SPLL_PLL_ENABLE;
}
@@ -1004,7 +1004,7 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
bool ret;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -1025,7 +1025,7 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
ret = true;
out:
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return ret;
}
@@ -1041,7 +1041,7 @@ static bool skl_ddi_dpll0_get_hw_state(struct drm_i915_private *dev_priv,
bool ret;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -1058,7 +1058,7 @@ static bool skl_ddi_dpll0_get_hw_state(struct drm_i915_private *dev_priv,
ret = true;
out:
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return ret;
}
@@ -1602,7 +1602,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
bxt_port_to_phy_channel(dev_priv, port, &phy, &ch);
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -1660,7 +1660,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
ret = true;
out:
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return ret;
}
@@ -2087,7 +2087,7 @@ static bool cnl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
bool ret;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -2107,7 +2107,7 @@ static bool cnl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
ret = true;
out:
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return ret;
}
@@ -2862,7 +2862,7 @@ static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
u32 val;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -2909,7 +2909,7 @@ static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
ret = true;
out:
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return ret;
}
@@ -2924,7 +2924,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
u32 val;
wakeref = intel_display_power_get_if_enabled(dev_priv,
- POWER_DOMAIN_PLLS);
+ POWER_DOMAIN_DISPLAY_CORE);
if (!wakeref)
return false;
@@ -2937,7 +2937,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
ret = true;
out:
- intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+ intel_display_power_put(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref);
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 618dd42f1b2d..a03e1c3d63d2 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -468,8 +468,6 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
return "VGA";
case POWER_DOMAIN_AUDIO:
return "AUDIO";
- case POWER_DOMAIN_PLLS:
- return "PLLS";
case POWER_DOMAIN_AUX_A:
return "AUX_A";
case POWER_DOMAIN_AUX_B:
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 10/11] drm/i915: Avoid taking the PPS lock for non-eDP/VLV/CHV
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (8 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 09/11] drm/i915: Replace use of PLLS power domain with DISPLAY_CORE domain Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 6:19 ` [PATCH v2 11/11] drm/i915: Assert that TypeC ports are not used for eDP Imre Deak
` (4 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
On ICL we have to make sure that we enable the AUX power domain in a
controlled way (corresponding to the port's actual TypeC mode). Since
the PPS lock - which takes an AUX power ref - is only needed on
eDP on all platforms and eDP/DP on VLV/CHV avoid taking it in all
other cases.
v2:
- Clarify commit log about the condition for taking the PPS lock.
(Ville)
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index de881bfea011..ddb1135b2f54 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6265,6 +6265,10 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
intel_dp->reset_link_params = true;
+ if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
+ !intel_dp_is_edp(intel_dp))
+ return;
+
with_pps_lock(intel_dp, wakeref) {
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
intel_dp->active_pipe = vlv_active_pipe(intel_dp);
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 11/11] drm/i915: Assert that TypeC ports are not used for eDP
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (9 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 10/11] drm/i915: Avoid taking the PPS lock for non-eDP/VLV/CHV Imre Deak
@ 2019-05-09 6:19 ` Imre Deak
2019-05-09 8:42 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for asynchronous display power disabling (rev3) Patchwork
` (3 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Imre Deak @ 2019-05-09 6:19 UTC (permalink / raw)
To: intel-gfx
Add an assert that we don't use TypeC ports for eDP. That may in theory
be possible on TypeC legacy ports, but I'm not sure if that's a
practical scenario, so let's deal with that only if there's a use case.
Adding support for that wouldn't be too difficult, since TypeC mode
switching is not possible on TypeC legacy ports.
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ddb1135b2f54..2e9c421aea60 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -7212,10 +7212,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
intel_dp->DP = I915_READ(intel_dp->output_reg);
intel_dp->attached_connector = intel_connector;
- if (intel_dp_is_port_edp(dev_priv, port))
+ if (intel_dp_is_port_edp(dev_priv, port)) {
+ /*
+ * Currently we don't support eDP on TypeC ports, although in
+ * theory it could work on TypeC legacy ports.
+ */
+ WARN_ON(intel_port_is_tc(dev_priv, port));
type = DRM_MODE_CONNECTOR_eDP;
- else
+ } else {
type = DRM_MODE_CONNECTOR_DisplayPort;
+ }
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
intel_dp->active_pipe = vlv_active_pipe(intel_dp);
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for asynchronous display power disabling (rev3)
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (10 preceding siblings ...)
2019-05-09 6:19 ` [PATCH v2 11/11] drm/i915: Assert that TypeC ports are not used for eDP Imre Deak
@ 2019-05-09 8:42 ` Patchwork
2019-05-09 9:09 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2019-05-09 8:42 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Add support for asynchronous display power disabling (rev3)
URL : https://patchwork.freedesktop.org/series/60242/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
09fdc96a8a2a drm/i915: Add support for tracking wakerefs w/o power-on guarantee
-:140: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#140: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:141:
+static void untrack_intel_runtime_pm_wakeref(struct drm_i915_private *i915,
depot_stack_handle_t stack)
total: 0 errors, 0 warnings, 1 checks, 323 lines checked
63c71d9263cd drm/i915: Force printing wakeref tacking during pm_cleanup
4ee90d1f5b2e drm/i915: Verify power domains state during suspend in all cases
2dc02abcf7d1 drm/i915: Add support for asynchronous display power disabling
-:387: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#387: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:2253:
+}
+/**
-:438: WARNING:TYPO_SPELLING: 'preceeding' may be misspelled - perhaps 'preceding'?
#438: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:2304:
+ * Flushes any pending work that was scheduled by a preceeding
total: 0 errors, 1 warnings, 1 checks, 486 lines checked
07f466831fe0 drm/i915: Disable power asynchronously during DP AUX transfers
517a70e54263 drm/i915: WARN for eDP encoders in intel_dp_detect_dpcd()
7e0177cf1f6b drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()
e4c479e1ad7e drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()
-:12: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 1c767b339b39 ("drm/i915: take display port power domain in DP HPD handler")'
#12:
commit 1c767b339b3938b19076ffdc9d70aa1e4235a45b
-:21: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")'
#21:
commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
total: 2 errors, 0 warnings, 0 checks, 68 lines checked
d781ea869815 drm/i915: Replace use of PLLS power domain with DISPLAY_CORE domain
093b2421b98b drm/i915: Avoid taking the PPS lock for non-eDP/VLV/CHV
3d4a316b7dc0 drm/i915: Assert that TypeC ports are not used for eDP
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* ✓ Fi.CI.BAT: success for drm/i915: Add support for asynchronous display power disabling (rev3)
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (11 preceding siblings ...)
2019-05-09 8:42 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for asynchronous display power disabling (rev3) Patchwork
@ 2019-05-09 9:09 ` Patchwork
2019-05-09 10:21 ` ✓ Fi.CI.IGT: " Patchwork
2019-05-09 16:25 ` [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Ville Syrjälä
14 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2019-05-09 9:09 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Add support for asynchronous display power disabling (rev3)
URL : https://patchwork.freedesktop.org/series/60242/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6068 -> Patchwork_12990
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/
Known issues
------------
Here are the changes found in Patchwork_12990 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@core_auth@basic-auth:
- fi-apl-guc: [PASS][1] -> [INCOMPLETE][2] ([fdo#103927])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/fi-apl-guc/igt@core_auth@basic-auth.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/fi-apl-guc/igt@core_auth@basic-auth.html
* igt@i915_module_load@reload:
- fi-blb-e6850: [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/fi-blb-e6850/igt@i915_module_load@reload.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/fi-blb-e6850/igt@i915_module_load@reload.html
#### Possible fixes ####
* igt@i915_module_load@reload-no-display:
- fi-skl-6770hq: [DMESG-WARN][5] ([fdo#105541]) -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/fi-skl-6770hq/igt@i915_module_load@reload-no-display.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/fi-skl-6770hq/igt@i915_module_load@reload-no-display.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
Participating hosts (51 -> 43)
------------------------------
Additional (2): fi-hsw-peppy fi-skl-lmem
Missing (10): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y fi-byt-clapper fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_6068 -> Patchwork_12990
CI_DRM_6068: 310f6f83473d076a67abb2d35348507b10f56557 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4973: 3e3ff0e48989abd25fce4916e85e8fef20a3c63a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12990: 3d4a316b7dc0acd19c26a2300ac1574e45085ff3 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
3d4a316b7dc0 drm/i915: Assert that TypeC ports are not used for eDP
093b2421b98b drm/i915: Avoid taking the PPS lock for non-eDP/VLV/CHV
d781ea869815 drm/i915: Replace use of PLLS power domain with DISPLAY_CORE domain
e4c479e1ad7e drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()
7e0177cf1f6b drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()
517a70e54263 drm/i915: WARN for eDP encoders in intel_dp_detect_dpcd()
07f466831fe0 drm/i915: Disable power asynchronously during DP AUX transfers
2dc02abcf7d1 drm/i915: Add support for asynchronous display power disabling
4ee90d1f5b2e drm/i915: Verify power domains state during suspend in all cases
63c71d9263cd drm/i915: Force printing wakeref tacking during pm_cleanup
09fdc96a8a2a drm/i915: Add support for tracking wakerefs w/o power-on guarantee
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* ✓ Fi.CI.IGT: success for drm/i915: Add support for asynchronous display power disabling (rev3)
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (12 preceding siblings ...)
2019-05-09 9:09 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-05-09 10:21 ` Patchwork
2019-05-09 16:25 ` [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Ville Syrjälä
14 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2019-05-09 10:21 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Add support for asynchronous display power disabling (rev3)
URL : https://patchwork.freedesktop.org/series/60242/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6068_full -> Patchwork_12990_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_12990_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_isolation@bcs0-s3:
- shard-skl: [PASS][1] -> [INCOMPLETE][2] ([fdo#104108] / [fdo#107773])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-skl6/igt@gem_ctx_isolation@bcs0-s3.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-skl2/igt@gem_ctx_isolation@bcs0-s3.html
* igt@i915_pm_rpm@sysfs-read:
- shard-skl: [PASS][3] -> [INCOMPLETE][4] ([fdo#107807]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-skl4/igt@i915_pm_rpm@sysfs-read.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-skl6/igt@i915_pm_rpm@sysfs-read.html
* igt@i915_pm_rpm@system-suspend:
- shard-skl: [PASS][5] -> [INCOMPLETE][6] ([fdo#104108] / [fdo#107807])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-skl9/igt@i915_pm_rpm@system-suspend.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-skl4/igt@i915_pm_rpm@system-suspend.html
* igt@i915_suspend@fence-restore-untiled:
- shard-apl: [PASS][7] -> [DMESG-WARN][8] ([fdo#108566]) +5 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-apl2/igt@i915_suspend@fence-restore-untiled.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-apl3/igt@i915_suspend@fence-restore-untiled.html
* igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
- shard-iclb: [PASS][9] -> [FAIL][10] ([fdo#103167]) +4 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
* igt@kms_frontbuffer_tracking@fbc-suspend:
- shard-kbl: [PASS][11] -> [DMESG-WARN][12] ([fdo#108566]) +2 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-kbl5/igt@kms_frontbuffer_tracking@fbc-suspend.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-suspend.html
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
- shard-skl: [PASS][13] -> [FAIL][14] ([fdo#108145]) +1 similar issue
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
* igt@kms_psr2_su@frontbuffer:
- shard-iclb: [PASS][15] -> [SKIP][16] ([fdo#109642])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb7/igt@kms_psr2_su@frontbuffer.html
* igt@kms_psr@no_drrs:
- shard-iclb: [PASS][17] -> [FAIL][18] ([fdo#108341])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb5/igt@kms_psr@no_drrs.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb1/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_dpms:
- shard-iclb: [PASS][19] -> [SKIP][20] ([fdo#109441]) +2 similar issues
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb2/igt@kms_psr@psr2_dpms.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb4/igt@kms_psr@psr2_dpms.html
* igt@kms_setmode@basic:
- shard-apl: [PASS][21] -> [FAIL][22] ([fdo#99912])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-apl1/igt@kms_setmode@basic.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-apl5/igt@kms_setmode@basic.html
- shard-hsw: [PASS][23] -> [FAIL][24] ([fdo#99912])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-hsw6/igt@kms_setmode@basic.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-hsw2/igt@kms_setmode@basic.html
#### Possible fixes ####
* igt@kms_flip@2x-flip-vs-suspend:
- shard-hsw: [INCOMPLETE][25] ([fdo#103540]) -> [PASS][26]
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-hsw2/igt@kms_flip@2x-flip-vs-suspend.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-hsw5/igt@kms_flip@2x-flip-vs-suspend.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-skl: [FAIL][27] ([fdo#105363]) -> [PASS][28]
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-skl10/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
* igt@kms_flip@flip-vs-suspend:
- shard-kbl: [DMESG-WARN][29] ([fdo#108566]) -> [PASS][30]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-kbl5/igt@kms_flip@flip-vs-suspend.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-kbl3/igt@kms_flip@flip-vs-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite:
- shard-iclb: [FAIL][31] ([fdo#103167]) -> [PASS][32] +3 similar issues
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite.html
* igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
- shard-glk: [SKIP][33] ([fdo#109271]) -> [PASS][34] +3 similar issues
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-glk1/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-glk2/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
- shard-skl: [INCOMPLETE][35] ([fdo#104108]) -> [PASS][36]
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-skl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-skl10/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
* igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format:
- shard-glk: [SKIP][37] ([fdo#109271] / [fdo#109278]) -> [PASS][38] +6 similar issues
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-glk8/igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-glk9/igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format.html
* igt@kms_psr@psr2_basic:
- shard-iclb: [SKIP][39] ([fdo#109441]) -> [PASS][40] +3 similar issues
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb5/igt@kms_psr@psr2_basic.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb2/igt@kms_psr@psr2_basic.html
* igt@kms_setmode@basic:
- shard-kbl: [FAIL][41] ([fdo#99912]) -> [PASS][42]
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-kbl3/igt@kms_setmode@basic.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-kbl2/igt@kms_setmode@basic.html
* igt@kms_sysfs_edid_timing:
- shard-iclb: [FAIL][43] ([fdo#100047]) -> [PASS][44]
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-iclb3/igt@kms_sysfs_edid_timing.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-iclb1/igt@kms_sysfs_edid_timing.html
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-kbl: [INCOMPLETE][45] ([fdo#103665]) -> [PASS][46]
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-kbl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
- shard-apl: [DMESG-WARN][47] ([fdo#108566]) -> [PASS][48] +2 similar issues
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-apl4/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-apl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
* igt@kms_vblank@pipe-b-ts-continuation-suspend:
- shard-snb: [DMESG-WARN][49] ([fdo#102365]) -> [PASS][50]
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6068/shard-snb5/igt@kms_vblank@pipe-b-ts-continuation-suspend.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/shard-snb5/igt@kms_vblank@pipe-b-ts-continuation-suspend.html
[fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
[fdo#102365]: https://bugs.freedesktop.org/show_bug.cgi?id=102365
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
[fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_6068 -> Patchwork_12990
CI_DRM_6068: 310f6f83473d076a67abb2d35348507b10f56557 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4973: 3e3ff0e48989abd25fce4916e85e8fef20a3c63a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12990: 3d4a316b7dc0acd19c26a2300ac1574e45085ff3 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12990/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling
2019-05-09 6:19 [PATCH v2 00/11] drm/i915: Add support for asynchronous display power disabling Imre Deak
` (13 preceding siblings ...)
2019-05-09 10:21 ` ✓ Fi.CI.IGT: " Patchwork
@ 2019-05-09 16:25 ` Ville Syrjälä
14 siblings, 0 replies; 26+ messages in thread
From: Ville Syrjälä @ 2019-05-09 16:25 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Thu, May 09, 2019 at 09:19:43AM +0300, Imre Deak wrote:
> This patchset is v2 of [1]. The main change is the rework of patch 1
> based on Chris' feedback.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
>
> [1] https://patchwork.freedesktop.org/series/60242/
>
> Imre Deak (11):
> drm/i915: Add support for tracking wakerefs w/o power-on guarantee
> drm/i915: Force printing wakeref tacking during pm_cleanup
> drm/i915: Verify power domains state during suspend in all cases
> drm/i915: Add support for asynchronous display power disabling
> drm/i915: Disable power asynchronously during DP AUX transfers
> drm/i915: WARN for eDP encoders in intel_dp_detect_dpcd()
> drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()
> drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()
> drm/i915: Replace use of PLLS power domain with DISPLAY_CORE domain
> drm/i915: Avoid taking the PPS lock for non-eDP/VLV/CHV
> drm/i915: Assert that TypeC ports are not used for eDP
I trust Chris scrutinized the wakeref stuff sufficiently so
I didn't pay all that much attention to those patches.
Patches 5-11 lgtm.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> drivers/gpu/drm/i915/i915_drv.h | 5 +
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_display.h | 2 +-
> drivers/gpu/drm/i915/intel_dp.c | 83 ++--
> drivers/gpu/drm/i915/intel_dpll_mgr.c | 36 +-
> drivers/gpu/drm/i915/intel_drv.h | 52 ++-
> drivers/gpu/drm/i915/intel_psr.c | 6 +
> drivers/gpu/drm/i915/intel_runtime_pm.c | 595 +++++++++++++++++++++---
> drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +
> 9 files changed, 662 insertions(+), 127 deletions(-)
>
> --
> 2.17.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread