Re: [Intel-gfx] [PATCH 17/21] drm/i915: Track the wakeref used to initialise display power domains
Chris Wilson writes:
> On module load and unload, we grab the POWER_DOMAIN_INIT powerwells and
> transfer them to the runtime-pm code. We can use our wakeref tracking to
> verify that the wakeref is indeed passed from init to enable, and
> disable to fini; and across suspend.
>
> Signed-off-by: Chris Wilson
> Cc: Jani Nikula
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 3 +
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/intel_runtime_pm.c | 151 +---
> 3 files changed, 88 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index b7dcacf2a5d3..96717a23b32f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2699,6 +2699,9 @@ static int i915_runtime_pm_status(struct seq_file *m,
> void *unused)
> if (!HAS_RUNTIME_PM(dev_priv))
> seq_puts(m, "Runtime power management not supported\n");
>
> + seq_printf(m, "Runtime power management: %s\n",
> +enableddisabled(!dev_priv->power_domains.wakeref));
> +
> seq_printf(m, "GPU idle: %s (epoch %u)\n",
> yesno(!dev_priv->gt.awake), dev_priv->gt.epoch);
> seq_printf(m, "IRQs disabled: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 44c1b21febba..259b91b62ff2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -822,6 +822,8 @@ struct i915_power_domains {
> bool display_core_suspended;
> int power_well_count;
>
> + intel_wakeref_t wakeref;
> +
> struct mutex lock;
> int domain_use_count[POWER_DOMAIN_NUM];
> 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 fd2fc10dd1e4..8d38f3e7fad1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -4009,7 +4009,7 @@ static void intel_power_domains_verify_state(struct
> drm_i915_private *dev_priv);
>
> /**
> * intel_power_domains_init_hw - initialize hardware power domain state
> - * @dev_priv: i915 device instance
> + * @i915: i915 device instance
> * @resume: Called from resume code paths or not
> *
> * This function initializes the hardware power domain state and enables all
> @@ -4023,30 +4023,31 @@ static void intel_power_domains_verify_state(struct
> drm_i915_private *dev_priv);
> * intel_power_domains_enable()) and must be paired with
> * intel_power_domains_fini_hw().
> */
> -void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool
> resume)
> +void intel_power_domains_init_hw(struct drm_i915_private *i915, bool resume)
> {
> - struct i915_power_domains *power_domains = _priv->power_domains;
> + struct i915_power_domains *power_domains = >power_domains;
>
> power_domains->initializing = true;
>
> - if (IS_ICELAKE(dev_priv)) {
> - icl_display_core_init(dev_priv, resume);
> - } else if (IS_CANNONLAKE(dev_priv)) {
> - cnl_display_core_init(dev_priv, resume);
> - } else if (IS_GEN9_BC(dev_priv)) {
> - skl_display_core_init(dev_priv, resume);
> - } else if (IS_GEN9_LP(dev_priv)) {
> - bxt_display_core_init(dev_priv, resume);
> - } else if (IS_CHERRYVIEW(dev_priv)) {
> + if (IS_ICELAKE(i915)) {
> + icl_display_core_init(i915, resume);
> + } else if (IS_CANNONLAKE(i915)) {
> + cnl_display_core_init(i915, resume);
> + } else if (IS_GEN9_BC(i915)) {
> + skl_display_core_init(i915, resume);
> + } else if (IS_GEN9_LP(i915)) {
> + bxt_display_core_init(i915, resume);
> + } else if (IS_CHERRYVIEW(i915)) {
> mutex_lock(_domains->lock);
> - chv_phy_control_init(dev_priv);
> + chv_phy_control_init(i915);
> mutex_unlock(_domains->lock);
> - } else if (IS_VALLEYVIEW(dev_priv)) {
> + } else if (IS_VALLEYVIEW(i915)) {
> mutex_lock(_domains->lock);
> - vlv_cmnlane_wa(dev_priv);
> + vlv_cmnlane_wa(i915);
> mutex_unlock(_domains->lock);
> - } else if (IS_IVYBRIDGE(dev_priv) || INTEL_GEN(dev_priv) >= 7)
> - intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
> + } else if (IS_IVYBRIDGE(i915) || INTEL_GEN(i915) >= 7) {
> + intel_pch_reset_handshake(i915, !HAS_PCH_NOP(i915));
> + }
>
> /*
>* Keep all power wells enabled for any dependent HW access during
> @@ -4054,18 +4055,20 @@ void intel_power_domains_init_hw(struct
> drm_i915_private *dev_priv, bool resume)
>* resources powered until display HW readout is complete. We drop
>* this reference in intel_power_domains_enable().
>*/
> - intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +
Re: [Intel-gfx] [PATCH 17/21] drm/i915: Track the wakeref used to initialise display power domains
Quoting John Harrison (2019-01-10 23:15:31)
> On 1/10/2019 02:11, Chris Wilson wrote:
> > @@ -4054,18 +4055,20 @@ void intel_power_domains_init_hw(struct
> > drm_i915_private *dev_priv, bool resume)
> >* resources powered until display HW readout is complete. We drop
> >* this reference in intel_power_domains_enable().
> >*/
> > - intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > + power_domains->wakeref =
> > + intel_display_power_get(i915, POWER_DOMAIN_INIT);
> > +
> > /* Disable power support if the user asked so. */
> > if (!i915_modparams.disable_power_well)
> > - intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > - intel_power_domains_sync_hw(dev_priv);
> > + intel_display_power_get(i915, POWER_DOMAIN_INIT);
> Why not save this cookie away somewhere too, e.g.
> power_domains->wakeref_disable? That way you can also get rid of the
> put_unchecked calls below.
Just seemed like a bit more hassle for the case where rpm was
intentionally disabled by the user. The system is going to leak the
wakerefs, tracking seemed pointless, so I just threw my hands up in the
air and gave up.
> > + intel_power_domains_sync_hw(i915);
> >
> > power_domains->initializing = false;
> > }
> >
> > /**
> >* intel_power_domains_fini_hw - deinitialize hw power domain state
> > - * @dev_priv: i915 device instance
> > + * @i915: i915 device instance
> >*
> >* De-initializes the display power domain HW state. It also ensures that
> > the
> >* device stays powered up so that the driver can be reloaded.
> > @@ -4074,21 +4077,24 @@ void intel_power_domains_init_hw(struct
> > drm_i915_private *dev_priv, bool resume)
> >* intel_power_domains_disable()) and must be paired with
> >* intel_power_domains_init_hw().
> >*/
> > -void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
> > +void intel_power_domains_fini_hw(struct drm_i915_private *i915)
> > {
> > - /* Keep the power well enabled, but cancel its rpm wakeref. */
> > - intel_runtime_pm_put_unchecked(dev_priv);
> > + intel_wakeref_t wakeref __maybe_unused =
> > + fetch_and_zero(>power_domains.wakeref);
> Why the '__maybe_unused'? The call to put is unconditional. Or is it
> about keeping the compiler happy in the case where the wakeref tracking
> is disabled? Although I don't recall seeing any 'maybe's in the earlier
> patches.
Just about keeping the compiler happy when the compiler complained.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/21] drm/i915: Track the wakeref used to initialise display power domains
On 1/10/2019 02:11, Chris Wilson wrote:
On module load and unload, we grab the POWER_DOMAIN_INIT powerwells and
transfer them to the runtime-pm code. We can use our wakeref tracking to
verify that the wakeref is indeed passed from init to enable, and
disable to fini; and across suspend.
Signed-off-by: Chris Wilson
Cc: Jani Nikula
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/intel_runtime_pm.c | 151 +---
3 files changed, 88 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index b7dcacf2a5d3..96717a23b32f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2699,6 +2699,9 @@ static int i915_runtime_pm_status(struct seq_file *m,
void *unused)
if (!HAS_RUNTIME_PM(dev_priv))
seq_puts(m, "Runtime power management not supported\n");
+ seq_printf(m, "Runtime power management: %s\n",
+ enableddisabled(!dev_priv->power_domains.wakeref));
+
seq_printf(m, "GPU idle: %s (epoch %u)\n",
yesno(!dev_priv->gt.awake), dev_priv->gt.epoch);
seq_printf(m, "IRQs disabled: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44c1b21febba..259b91b62ff2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -822,6 +822,8 @@ struct i915_power_domains {
bool display_core_suspended;
int power_well_count;
+ intel_wakeref_t wakeref;
+
struct mutex lock;
int domain_use_count[POWER_DOMAIN_NUM];
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 fd2fc10dd1e4..8d38f3e7fad1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -4009,7 +4009,7 @@ static void intel_power_domains_verify_state(struct
drm_i915_private *dev_priv);
/**
* intel_power_domains_init_hw - initialize hardware power domain state
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
* @resume: Called from resume code paths or not
*
* This function initializes the hardware power domain state and enables all
@@ -4023,30 +4023,31 @@ static void intel_power_domains_verify_state(struct
drm_i915_private *dev_priv);
* intel_power_domains_enable()) and must be paired with
* intel_power_domains_fini_hw().
*/
-void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool
resume)
+void intel_power_domains_init_hw(struct drm_i915_private *i915, bool resume)
{
- struct i915_power_domains *power_domains = _priv->power_domains;
+ struct i915_power_domains *power_domains = >power_domains;
power_domains->initializing = true;
- if (IS_ICELAKE(dev_priv)) {
- icl_display_core_init(dev_priv, resume);
- } else if (IS_CANNONLAKE(dev_priv)) {
- cnl_display_core_init(dev_priv, resume);
- } else if (IS_GEN9_BC(dev_priv)) {
- skl_display_core_init(dev_priv, resume);
- } else if (IS_GEN9_LP(dev_priv)) {
- bxt_display_core_init(dev_priv, resume);
- } else if (IS_CHERRYVIEW(dev_priv)) {
+ if (IS_ICELAKE(i915)) {
+ icl_display_core_init(i915, resume);
+ } else if (IS_CANNONLAKE(i915)) {
+ cnl_display_core_init(i915, resume);
+ } else if (IS_GEN9_BC(i915)) {
+ skl_display_core_init(i915, resume);
+ } else if (IS_GEN9_LP(i915)) {
+ bxt_display_core_init(i915, resume);
+ } else if (IS_CHERRYVIEW(i915)) {
mutex_lock(_domains->lock);
- chv_phy_control_init(dev_priv);
+ chv_phy_control_init(i915);
mutex_unlock(_domains->lock);
- } else if (IS_VALLEYVIEW(dev_priv)) {
+ } else if (IS_VALLEYVIEW(i915)) {
mutex_lock(_domains->lock);
- vlv_cmnlane_wa(dev_priv);
+ vlv_cmnlane_wa(i915);
mutex_unlock(_domains->lock);
- } else if (IS_IVYBRIDGE(dev_priv) || INTEL_GEN(dev_priv) >= 7)
- intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
+ } else if (IS_IVYBRIDGE(i915) || INTEL_GEN(i915) >= 7) {
+ intel_pch_reset_handshake(i915, !HAS_PCH_NOP(i915));
+ }
/*
* Keep all power wells enabled for any dependent HW access during
@@ -4054,18 +4055,20 @@ void intel_power_domains_init_hw(struct
drm_i915_private *dev_priv, bool resume)
* resources powered until display HW readout is complete. We drop
* this reference in intel_power_domains_enable().
*/
- intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+ power_domains->wakeref =
+ intel_display_power_get(i915, POWER_DOMAIN_INIT);
+
/*
[Intel-gfx] [PATCH 17/21] drm/i915: Track the wakeref used to initialise display power domains
On module load and unload, we grab the POWER_DOMAIN_INIT powerwells and
transfer them to the runtime-pm code. We can use our wakeref tracking to
verify that the wakeref is indeed passed from init to enable, and
disable to fini; and across suspend.
Signed-off-by: Chris Wilson
Cc: Jani Nikula
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/intel_runtime_pm.c | 151 +---
3 files changed, 88 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index b7dcacf2a5d3..96717a23b32f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2699,6 +2699,9 @@ static int i915_runtime_pm_status(struct seq_file *m,
void *unused)
if (!HAS_RUNTIME_PM(dev_priv))
seq_puts(m, "Runtime power management not supported\n");
+ seq_printf(m, "Runtime power management: %s\n",
+ enableddisabled(!dev_priv->power_domains.wakeref));
+
seq_printf(m, "GPU idle: %s (epoch %u)\n",
yesno(!dev_priv->gt.awake), dev_priv->gt.epoch);
seq_printf(m, "IRQs disabled: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44c1b21febba..259b91b62ff2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -822,6 +822,8 @@ struct i915_power_domains {
bool display_core_suspended;
int power_well_count;
+ intel_wakeref_t wakeref;
+
struct mutex lock;
int domain_use_count[POWER_DOMAIN_NUM];
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 fd2fc10dd1e4..8d38f3e7fad1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -4009,7 +4009,7 @@ static void intel_power_domains_verify_state(struct
drm_i915_private *dev_priv);
/**
* intel_power_domains_init_hw - initialize hardware power domain state
- * @dev_priv: i915 device instance
+ * @i915: i915 device instance
* @resume: Called from resume code paths or not
*
* This function initializes the hardware power domain state and enables all
@@ -4023,30 +4023,31 @@ static void intel_power_domains_verify_state(struct
drm_i915_private *dev_priv);
* intel_power_domains_enable()) and must be paired with
* intel_power_domains_fini_hw().
*/
-void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool
resume)
+void intel_power_domains_init_hw(struct drm_i915_private *i915, bool resume)
{
- struct i915_power_domains *power_domains = _priv->power_domains;
+ struct i915_power_domains *power_domains = >power_domains;
power_domains->initializing = true;
- if (IS_ICELAKE(dev_priv)) {
- icl_display_core_init(dev_priv, resume);
- } else if (IS_CANNONLAKE(dev_priv)) {
- cnl_display_core_init(dev_priv, resume);
- } else if (IS_GEN9_BC(dev_priv)) {
- skl_display_core_init(dev_priv, resume);
- } else if (IS_GEN9_LP(dev_priv)) {
- bxt_display_core_init(dev_priv, resume);
- } else if (IS_CHERRYVIEW(dev_priv)) {
+ if (IS_ICELAKE(i915)) {
+ icl_display_core_init(i915, resume);
+ } else if (IS_CANNONLAKE(i915)) {
+ cnl_display_core_init(i915, resume);
+ } else if (IS_GEN9_BC(i915)) {
+ skl_display_core_init(i915, resume);
+ } else if (IS_GEN9_LP(i915)) {
+ bxt_display_core_init(i915, resume);
+ } else if (IS_CHERRYVIEW(i915)) {
mutex_lock(_domains->lock);
- chv_phy_control_init(dev_priv);
+ chv_phy_control_init(i915);
mutex_unlock(_domains->lock);
- } else if (IS_VALLEYVIEW(dev_priv)) {
+ } else if (IS_VALLEYVIEW(i915)) {
mutex_lock(_domains->lock);
- vlv_cmnlane_wa(dev_priv);
+ vlv_cmnlane_wa(i915);
mutex_unlock(_domains->lock);
- } else if (IS_IVYBRIDGE(dev_priv) || INTEL_GEN(dev_priv) >= 7)
- intel_pch_reset_handshake(dev_priv, !HAS_PCH_NOP(dev_priv));
+ } else if (IS_IVYBRIDGE(i915) || INTEL_GEN(i915) >= 7) {
+ intel_pch_reset_handshake(i915, !HAS_PCH_NOP(i915));
+ }
/*
* Keep all power wells enabled for any dependent HW access during
@@ -4054,18 +4055,20 @@ void intel_power_domains_init_hw(struct
drm_i915_private *dev_priv, bool resume)
* resources powered until display HW readout is complete. We drop
* this reference in intel_power_domains_enable().
*/
- intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+ power_domains->wakeref =
+ intel_display_power_get(i915, POWER_DOMAIN_INIT);
+
/* Disable power support if the user
