[PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Jesse Barnes]

This was always the case on our suspend path, but it was recently
exposed by the change to use our runtime IRQ disable routine rather than
the full DRM IRQ disable.  Keep the warning on the enable side, as that
really would indicate a bug.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1c1ec22..fe3b309 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(dev_priv->pm.irqs_disabled))
+	if (dev_priv->pm.irqs_disabled)
 		return;
 
 	if ((dev_priv->irq_mask & mask) != mask) {
-- 
1.8.3.2

[PATCH 2/4] drm/i915: add helper for checking whether IRQs are enabled

[Jesse Barnes]

Now that we use the runtime IRQ enable/disable functions in our suspend
path, we can simply check the pm._irqs_disabled flag everywhere.  So
rename it to catch the users, and add an inline for it to make the
checks clear everywhere.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/i915_gem.c      |  2 +-
 drivers/gpu/drm/i915/i915_irq.c      | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  9 ++++++++-
 drivers/gpu/drm/i915/intel_pm.c      |  6 +++---
 7 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 76c2572..f3c0482 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2076,7 +2076,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
 	seq_printf(m, "IRQs disabled: %s\n",
-		   yesno(dev_priv->pm.irqs_disabled));
+		   yesno(!intel_irqs_enabled(dev_priv)));
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0640071..d12e9b7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1298,7 +1298,7 @@ struct ilk_wm_values {
  */
 struct i915_runtime_pm {
 	bool suspended;
-	bool irqs_disabled;
+	bool _irqs_disabled;
 };
 
 enum intel_pipe_crc_source {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d857f58..2f1815e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1161,7 +1161,7 @@ static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 	unsigned long timeout_expire;
 	int ret;
 
-	WARN(dev_priv->pm.irqs_disabled, "IRQs disabled\n");
+	WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
 
 	if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
 		return 0;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fe3b309..bc953cc 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -136,7 +136,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(dev_priv->pm.irqs_disabled))
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
 	if ((dev_priv->irq_mask & mask) != 0) {
@@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pm.irqs_disabled)
+	if (!intel_irqs_enabled(dev_priv))
 		return;
 
 	if ((dev_priv->irq_mask & mask) != mask) {
@@ -173,7 +173,7 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(dev_priv->pm.irqs_disabled))
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
 	dev_priv->gt_irq_mask &= ~interrupt_mask;
@@ -206,7 +206,7 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(dev_priv->pm.irqs_disabled))
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
 	new_val = dev_priv->pm_irq_mask;
@@ -264,7 +264,7 @@ static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(dev_priv->pm.irqs_disabled))
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
 	new_val = dev_priv->pm_irq_mask;
@@ -420,7 +420,7 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(dev_priv->pm.irqs_disabled))
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
 	I915_WRITE(SDEIMR, sdeimr);
@@ -4478,7 +4478,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	dev->driver->irq_uninstall(dev);
-	dev_priv->pm.irqs_disabled = true;
+	dev_priv->pm._irqs_disabled = true;
 }
 
 /* Restore interrupts so we can recover from runtime PM. */
@@ -4486,7 +4486,7 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	dev_priv->pm.irqs_disabled = false;
+	dev_priv->pm._irqs_disabled = false;
 	dev->driver->irq_preinstall(dev);
 	dev->driver->irq_postinstall(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e8e711..d993b69 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7323,7 +7323,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	 * gen-specific and since we only disable LCPLL after we fully disable
 	 * the interrupts, the check below should be enough.
 	 */
-	WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
+	WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n");
 }
 
 static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab5962b..4912738 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -687,7 +687,14 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
 void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
 void i9xx_check_fifo_underruns(struct drm_device *dev);
-
+static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
+{
+	/*
+	 * We only use drm_irq_uninstall() at unload and VT switch, so
+	 * this is the only thing we need to check.
+	 */
+	return !dev_priv->pm._irqs_disabled;
+}
 
 /* intel_crt.c */
 void intel_crt_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0bf9b0c..88734cb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4879,7 +4879,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Interrupts should be disabled already to avoid re-arming. */
-	WARN_ON(dev->irq_enabled && !dev_priv->pm.irqs_disabled);
+	WARN_ON(intel_irqs_enabled(dev_priv));
 
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
@@ -4891,7 +4891,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Interrupts should be disabled already to avoid re-arming. */
-	WARN_ON(dev->irq_enabled && !dev_priv->pm.irqs_disabled);
+	WARN_ON(intel_irqs_enabled(dev_priv));
 
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_disable_drps(dev);
@@ -6802,5 +6802,5 @@ void intel_pm_setup(struct drm_device *dev)
 			  intel_gen6_powersave_work);
 
 	dev_priv->pm.suspended = false;
-	dev_priv->pm.irqs_disabled = false;
+	dev_priv->pm._irqs_disabled = false;
 }
-- 
1.8.3.2

Re: [PATCH 2/4] drm/i915: add helper for checking whether IRQs are enabled

[Paulo Zanoni]

2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Now that we use the runtime IRQ enable/disable functions in our suspend
> path, we can simply check the pm._irqs_disabled flag everywhere.  So
> rename it to catch the users, and add an inline for it to make the
> checks clear everywhere.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c      |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c      | 16 ++++++++--------
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_pm.c      |  6 +++---
>  7 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 76c2572..f3c0482 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2076,7 +2076,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
>
>         seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
>         seq_printf(m, "IRQs disabled: %s\n",
> -                  yesno(dev_priv->pm.irqs_disabled));
> +                  yesno(!intel_irqs_enabled(dev_priv)));
>
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0640071..d12e9b7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1298,7 +1298,7 @@ struct ilk_wm_values {
>   */
>  struct i915_runtime_pm {
>         bool suspended;
> -       bool irqs_disabled;
> +       bool _irqs_disabled;
>  };
>
>  enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d857f58..2f1815e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1161,7 +1161,7 @@ static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
>         unsigned long timeout_expire;
>         int ret;
>
> -       WARN(dev_priv->pm.irqs_disabled, "IRQs disabled\n");
> +       WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
>
>         if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
>                 return 0;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fe3b309..bc953cc 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -136,7 +136,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  {
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>                 return;
>
>         if ((dev_priv->irq_mask & mask) != 0) {
> @@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  {
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (dev_priv->pm.irqs_disabled)
> +       if (!intel_irqs_enabled(dev_priv))
>                 return;
>
>         if ((dev_priv->irq_mask & mask) != mask) {
> @@ -173,7 +173,7 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
>  {
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>                 return;
>
>         dev_priv->gt_irq_mask &= ~interrupt_mask;
> @@ -206,7 +206,7 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>                 return;
>
>         new_val = dev_priv->pm_irq_mask;
> @@ -264,7 +264,7 @@ static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
>
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>                 return;
>
>         new_val = dev_priv->pm_irq_mask;
> @@ -420,7 +420,7 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>                 return;
>
>         I915_WRITE(SDEIMR, sdeimr);
> @@ -4478,7 +4478,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         dev->driver->irq_uninstall(dev);
> -       dev_priv->pm.irqs_disabled = true;
> +       dev_priv->pm._irqs_disabled = true;
>  }
>
>  /* Restore interrupts so we can recover from runtime PM. */
> @@ -4486,7 +4486,7 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       dev_priv->pm.irqs_disabled = false;
> +       dev_priv->pm._irqs_disabled = false;
>         dev->driver->irq_preinstall(dev);
>         dev->driver->irq_postinstall(dev);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e8e711..d993b69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7323,7 +7323,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>          * gen-specific and since we only disable LCPLL after we fully disable
>          * the interrupts, the check below should be enough.
>          */
> -       WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
> +       WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n");
>  }
>
>  static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab5962b..4912738 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,7 +687,14 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
>  void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void i9xx_check_fifo_underruns(struct drm_device *dev);
> -
> +static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
> +{
> +       /*
> +        * We only use drm_irq_uninstall() at unload and VT switch, so
> +        * this is the only thing we need to check.
> +        */
> +       return !dev_priv->pm._irqs_disabled;
> +}
>
>  /* intel_crt.c */
>  void intel_crt_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0bf9b0c..88734cb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4879,7 +4879,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         /* Interrupts should be disabled already to avoid re-arming. */
> -       WARN_ON(dev->irq_enabled && !dev_priv->pm.irqs_disabled);
> +       WARN_ON(intel_irqs_enabled(dev_priv));
>
>         flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>
> @@ -4891,7 +4891,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         /* Interrupts should be disabled already to avoid re-arming. */
> -       WARN_ON(dev->irq_enabled && !dev_priv->pm.irqs_disabled);
> +       WARN_ON(intel_irqs_enabled(dev_priv));
>
>         if (IS_IRONLAKE_M(dev)) {
>                 ironlake_disable_drps(dev);
> @@ -6802,5 +6802,5 @@ void intel_pm_setup(struct drm_device *dev)
>                           intel_gen6_powersave_work);
>
>         dev_priv->pm.suspended = false;
> -       dev_priv->pm.irqs_disabled = false;
> +       dev_priv->pm._irqs_disabled = false;
>  }
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: add helper for checking whether IRQs are enabled

[Paulo Zanoni]

2014-07-14 12:06 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> Now that we use the runtime IRQ enable/disable functions in our suspend
>> path, we can simply check the pm._irqs_disabled flag everywhere.  So
>> rename it to catch the users, and add an inline for it to make the
>> checks clear everywhere.
>>
>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Looks like this patch brings some WARNs at init time, so I retract
that R-B tag :)

>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>>  drivers/gpu/drm/i915/i915_gem.c      |  2 +-
>>  drivers/gpu/drm/i915/i915_irq.c      | 16 ++++++++--------
>>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++++++-
>>  drivers/gpu/drm/i915/intel_pm.c      |  6 +++---
>>  7 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 76c2572..f3c0482 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2076,7 +2076,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
>>
>>         seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
>>         seq_printf(m, "IRQs disabled: %s\n",
>> -                  yesno(dev_priv->pm.irqs_disabled));
>> +                  yesno(!intel_irqs_enabled(dev_priv)));
>>
>>         return 0;
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 0640071..d12e9b7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1298,7 +1298,7 @@ struct ilk_wm_values {
>>   */
>>  struct i915_runtime_pm {
>>         bool suspended;
>> -       bool irqs_disabled;
>> +       bool _irqs_disabled;
>>  };
>>
>>  enum intel_pipe_crc_source {
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index d857f58..2f1815e 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1161,7 +1161,7 @@ static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
>>         unsigned long timeout_expire;
>>         int ret;
>>
>> -       WARN(dev_priv->pm.irqs_disabled, "IRQs disabled\n");
>> +       WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
>>
>>         if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
>>                 return 0;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index fe3b309..bc953cc 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -136,7 +136,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>>  {
>>         assert_spin_locked(&dev_priv->irq_lock);
>>
>> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
>> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>>                 return;
>>
>>         if ((dev_priv->irq_mask & mask) != 0) {
>> @@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>>  {
>>         assert_spin_locked(&dev_priv->irq_lock);
>>
>> -       if (dev_priv->pm.irqs_disabled)
>> +       if (!intel_irqs_enabled(dev_priv))
>>                 return;
>>
>>         if ((dev_priv->irq_mask & mask) != mask) {
>> @@ -173,7 +173,7 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
>>  {
>>         assert_spin_locked(&dev_priv->irq_lock);
>>
>> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
>> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>>                 return;
>>
>>         dev_priv->gt_irq_mask &= ~interrupt_mask;
>> @@ -206,7 +206,7 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>>
>>         assert_spin_locked(&dev_priv->irq_lock);
>>
>> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
>> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>>                 return;
>>
>>         new_val = dev_priv->pm_irq_mask;
>> @@ -264,7 +264,7 @@ static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
>>
>>         assert_spin_locked(&dev_priv->irq_lock);
>>
>> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
>> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>>                 return;
>>
>>         new_val = dev_priv->pm_irq_mask;
>> @@ -420,7 +420,7 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>>
>>         assert_spin_locked(&dev_priv->irq_lock);
>>
>> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
>> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>>                 return;
>>
>>         I915_WRITE(SDEIMR, sdeimr);
>> @@ -4478,7 +4478,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>
>>         dev->driver->irq_uninstall(dev);
>> -       dev_priv->pm.irqs_disabled = true;
>> +       dev_priv->pm._irqs_disabled = true;
>>  }
>>
>>  /* Restore interrupts so we can recover from runtime PM. */
>> @@ -4486,7 +4486,7 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> -       dev_priv->pm.irqs_disabled = false;
>> +       dev_priv->pm._irqs_disabled = false;
>>         dev->driver->irq_preinstall(dev);
>>         dev->driver->irq_postinstall(dev);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5e8e711..d993b69 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -7323,7 +7323,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>>          * gen-specific and since we only disable LCPLL after we fully disable
>>          * the interrupts, the check below should be enough.
>>          */
>> -       WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
>> +       WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n");
>>  }
>>
>>  static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index ab5962b..4912738 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -687,7 +687,14 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
>>  void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>>  void i9xx_check_fifo_underruns(struct drm_device *dev);
>> -
>> +static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
>> +{
>> +       /*
>> +        * We only use drm_irq_uninstall() at unload and VT switch, so
>> +        * this is the only thing we need to check.
>> +        */
>> +       return !dev_priv->pm._irqs_disabled;
>> +}
>>
>>  /* intel_crt.c */
>>  void intel_crt_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 0bf9b0c..88734cb 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4879,7 +4879,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>
>>         /* Interrupts should be disabled already to avoid re-arming. */
>> -       WARN_ON(dev->irq_enabled && !dev_priv->pm.irqs_disabled);
>> +       WARN_ON(intel_irqs_enabled(dev_priv));
>>
>>         flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>>
>> @@ -4891,7 +4891,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>
>>         /* Interrupts should be disabled already to avoid re-arming. */
>> -       WARN_ON(dev->irq_enabled && !dev_priv->pm.irqs_disabled);
>> +       WARN_ON(intel_irqs_enabled(dev_priv));
>>
>>         if (IS_IRONLAKE_M(dev)) {
>>                 ironlake_disable_drps(dev);
>> @@ -6802,5 +6802,5 @@ void intel_pm_setup(struct drm_device *dev)
>>                           intel_gen6_powersave_work);
>>
>>         dev_priv->pm.suspended = false;
>> -       dev_priv->pm.irqs_disabled = false;
>> +       dev_priv->pm._irqs_disabled = false;
>>  }
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: add helper for checking whether IRQs are enabled

[Jesse Barnes]

On Mon, 14 Jul 2014 12:19:54 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2014-07-14 12:06 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> Now that we use the runtime IRQ enable/disable functions in our suspend
> >> path, we can simply check the pm._irqs_disabled flag everywhere.  So
> >> rename it to catch the users, and add an inline for it to make the
> >> checks clear everywhere.
> >>
> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Looks like this patch brings some WARNs at init time, so I retract
> that R-B tag :)

The later patches fix those.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/4] drm/i915: drop runtime PM get/put pair around DP detect

[Jesse Barnes]

We're taking the right power well refs now, so this shouldn't be needed.

Reported-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 912e9c4..ed6f00c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3596,8 +3596,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	enum intel_display_power_domain power_domain;
 	struct edid *edid = NULL;
 
-	intel_runtime_pm_get(dev_priv);
-
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
@@ -3633,8 +3631,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 out:
 	intel_display_power_put(dev_priv, power_domain);
 
-	intel_runtime_pm_put(dev_priv);
-
 	return status;
 }
 
-- 
1.8.3.2

Re: [PATCH 3/4] drm/i915: drop runtime PM get/put pair around DP detect

[Paulo Zanoni]

2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> We're taking the right power well refs now, so this shouldn't be needed.
>
> Reported-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I submitted the exact same patch back in April, and got a review
comment: http://patchwork.freedesktop.org/patch/24645/

Then I sent a new version with the review comment applied:
http://patchwork.freedesktop.org/patch/24769/

It even has a R-B tag, it's just waiting to be merged :)

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 912e9c4..ed6f00c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3596,8 +3596,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>         enum intel_display_power_domain power_domain;
>         struct edid *edid = NULL;
>
> -       intel_runtime_pm_get(dev_priv);
> -
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> @@ -3633,8 +3631,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  out:
>         intel_display_power_put(dev_priv, power_domain);
>
> -       intel_runtime_pm_put(dev_priv);
> -
>         return status;
>  }
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 4/4] drm/i915: set pm._irqs_disabled at IRQ init time

[Jesse Barnes]

Before we've installed the handler, we can set this and avoid confusing
init code that then thinks IRQs are enabled and spews complaints
everywhere.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc953cc..86638b9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4373,6 +4373,9 @@ void intel_irq_init(struct drm_device *dev)
 
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
 
+	/* Haven't installed the IRQ handler yet */
+	dev_priv->pm._irqs_disabled = true;
+
 	if (IS_GEN2(dev)) {
 		dev->max_vblank_count = 0;
 		dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
-- 
1.8.3.2

Re: [PATCH 4/4] drm/i915: set pm._irqs_disabled at IRQ init time

[Paulo Zanoni]

2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Before we've installed the handler, we can set this and avoid confusing
> init code that then thinks IRQs are enabled and spews complaints
> everywhere.

But then at some point the DRM layer will call our IRQ init callbacks,
which will initalize the interrupts but leave irqs_disabled as true,
which will also confuse some code somewhere at some point. And it will
only be set to false after we {runtime,}-suspend/resume.

This is why I had kept the runtime PM code only used by the runtime PM
stuff. Recently we tried to reuse the runtime PM interrupt code at
other contexts, got regressions and now we're fixing the regressions
using duct tape... Maybe the best approach would be to revert some
patches...

>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc953cc..86638b9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4373,6 +4373,9 @@ void intel_irq_init(struct drm_device *dev)
>
>         pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>
> +       /* Haven't installed the IRQ handler yet */
> +       dev_priv->pm._irqs_disabled = true;
> +
>         if (IS_GEN2(dev)) {
>                 dev->max_vblank_count = 0;
>                 dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/4] drm/i915: set pm._irqs_disabled at IRQ init time

[Daniel Vetter]

On Mon, Jul 14, 2014 at 12:23:11PM -0300, Paulo Zanoni wrote:
> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > Before we've installed the handler, we can set this and avoid confusing
> > init code that then thinks IRQs are enabled and spews complaints
> > everywhere.
> 
> But then at some point the DRM layer will call our IRQ init callbacks,
> which will initalize the interrupts but leave irqs_disabled as true,
> which will also confuse some code somewhere at some point. And it will
> only be set to false after we {runtime,}-suspend/resume.

The drm irq stuff is _strictly_ a helper library, at least for modesetting
drivers. Which means it will never call our callbacks on its own.

> This is why I had kept the runtime PM code only used by the runtime PM
> stuff. Recently we tried to reuse the runtime PM interrupt code at
> other contexts, got regressions and now we're fixing the regressions
> using duct tape... Maybe the best approach would be to revert some
> patches...

Those patches where for soix, so feature work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: set pm._irqs_disabled at IRQ init time

[Paulo Zanoni]

2014-07-14 14:26 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Jul 14, 2014 at 12:23:11PM -0300, Paulo Zanoni wrote:
>> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> > Before we've installed the handler, we can set this and avoid confusing
>> > init code that then thinks IRQs are enabled and spews complaints
>> > everywhere.
>>
>> But then at some point the DRM layer will call our IRQ init callbacks,
>> which will initalize the interrupts but leave irqs_disabled as true,
>> which will also confuse some code somewhere at some point. And it will
>> only be set to false after we {runtime,}-suspend/resume.
>
> The drm irq stuff is _strictly_ a helper library, at least for modesetting
> drivers. Which means it will never call our callbacks on its own.

I was talking about drm_irq_install(), which is called by i915_driver_load().

>
>> This is why I had kept the runtime PM code only used by the runtime PM
>> stuff. Recently we tried to reuse the runtime PM interrupt code at
>> other contexts, got regressions and now we're fixing the regressions
>> using duct tape... Maybe the best approach would be to revert some
>> patches...
>
> Those patches where for soix, so feature work.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 4/4] drm/i915: set pm._irqs_disabled at IRQ init time

[Jesse Barnes]

On Mon, 14 Jul 2014 14:47:07 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2014-07-14 14:26 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Jul 14, 2014 at 12:23:11PM -0300, Paulo Zanoni wrote:
> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> > Before we've installed the handler, we can set this and avoid confusing
> >> > init code that then thinks IRQs are enabled and spews complaints
> >> > everywhere.
> >>
> >> But then at some point the DRM layer will call our IRQ init callbacks,
> >> which will initalize the interrupts but leave irqs_disabled as true,
> >> which will also confuse some code somewhere at some point. And it will
> >> only be set to false after we {runtime,}-suspend/resume.
> >
> > The drm irq stuff is _strictly_ a helper library, at least for modesetting
> > drivers. Which means it will never call our callbacks on its own.
> 
> I was talking about drm_irq_install(), which is called by i915_driver_load().

Since this set fixes the failures I've found, I'd rather see it merged
than to regress everyone's power consumption.

I think it'll probably take us awhile to figure out how we want to do
this stuff.  Paulo, maybe you can create a task with some of the things
you'd like to see done?

With these patches, we go away from ever using the drm irq bits except
for at load and unload.  We use the pm irq fields for tracking our own
stuff, which means some special init handling and then calls at
suspend/resume, so I don't think it's too complex as-is, but if you
want to do something better, that's ok too.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: clear pm._irqs_disabled field after installing IRQs

[Jesse Barnes]

After this point, we'll modify it with the runtime routines.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e583a1..f9b3f63 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1340,6 +1340,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_gem_stolen;
 
+	dev_priv->pm._irqs_disabled = false;
+
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
 	intel_modeset_init(dev);
-- 
1.8.3.2

[PATCH] drm/i915: mark IRQs as disabled on unload

[Jesse Barnes]

To avoid more spew with the new warnings.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d993b69..7288d1d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12778,6 +12778,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	 */
 	drm_irq_uninstall(dev);
 	cancel_work_sync(&dev_priv->hotplug_work);
+	dev_priv->pm._irqs_disabled = true;
+
 	/*
 	 * Due to the hpd irq storm handling the hotplug work can re-arm the
 	 * poll handlers. Hence disable polling after hpd handling is shut down.
-- 
1.8.3.2

Re: [PATCH] drm/i915: mark IRQs as disabled on unload

[Daniel Vetter]

On Fri, Jun 20, 2014 at 11:57:33AM -0700, Jesse Barnes wrote:
> To avoid more spew with the new warnings.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Ok, I've pulled this all into dinq. I guess we'll see how it all works
out.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d993b69..7288d1d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12778,6 +12778,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	 */
>  	drm_irq_uninstall(dev);
>  	cancel_work_sync(&dev_priv->hotplug_work);
> +	dev_priv->pm._irqs_disabled = true;
> +
>  	/*
>  	 * Due to the hpd irq storm handling the hotplug work can re-arm the
>  	 * poll handlers. Hence disable polling after hpd handling is shut down.
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Paulo Zanoni]

(documenting what we discussed on IRC)

2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> This was always the case on our suspend path, but it was recently
> exposed by the change to use our runtime IRQ disable routine rather than
> the full DRM IRQ disable.  Keep the warning on the enable side, as that
> really would indicate a bug.

While I understand the patch and think it's a reasonable thing to do,
I feel the need to spend some time persuading you in replacing it with
something that doesn't involve removing WARNs from our driver. While
the driver is runtime suspended, no one should really be manipulating
IRQs, even if they're disabling stuff that is already disabled: this
reflects there's probably a problem somewhere. These WARNs are
extremely helpful for the runtime PM feature because almost nobody
actually uses runtime PM to notice any bugs with it, so the WARNs can
make QA report bugs and bisect things for us.

Also, it seems S3 suspend is currently a little disaster on our
driver. Your 6 patches just solve some of the WARNs, not all of them.
And last week I even solved another WARN on the S3 path. I just did
some investigation, and the current bad commits are:
8abdc17941c71b37311bb93876ac83dce58160c8,
e11aa362308f5de467ce355a2a2471321b15a35c and
85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
commits, S3 doesn't give me any WARNs.

Instead of the change you proposed, can't we think of another solution
that would maybe address all the 3 regressions we have? Since we're
always submitting patches to change the order we do things at S3
suspend/resume, shouldn't we add something like "dev_priv->suspending"
that could be used to avoid the strict ordering that is required
during runtime?


>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1c1ec22..fe3b309 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  {
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(dev_priv->pm.irqs_disabled))
> +       if (dev_priv->pm.irqs_disabled)
>                 return;
>
>         if ((dev_priv->irq_mask & mask) != mask) {
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Jesse Barnes]

On Mon, 7 Jul 2014 18:48:47 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> (documenting what we discussed on IRC)
> 
> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > This was always the case on our suspend path, but it was recently
> > exposed by the change to use our runtime IRQ disable routine rather than
> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
> > really would indicate a bug.
> 
> While I understand the patch and think it's a reasonable thing to do,
> I feel the need to spend some time persuading you in replacing it with
> something that doesn't involve removing WARNs from our driver. While
> the driver is runtime suspended, no one should really be manipulating
> IRQs, even if they're disabling stuff that is already disabled: this
> reflects there's probably a problem somewhere. These WARNs are
> extremely helpful for the runtime PM feature because almost nobody
> actually uses runtime PM to notice any bugs with it, so the WARNs can
> make QA report bugs and bisect things for us.
> 
> Also, it seems S3 suspend is currently a little disaster on our
> driver. Your 6 patches just solve some of the WARNs, not all of them.
> And last week I even solved another WARN on the S3 path. I just did
> some investigation, and the current bad commits are:
> 8abdc17941c71b37311bb93876ac83dce58160c8,
> e11aa362308f5de467ce355a2a2471321b15a35c and
> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
> commits, S3 doesn't give me any WARNs.
> 
> Instead of the change you proposed, can't we think of another solution
> that would maybe address all the 3 regressions we have? Since we're
> always submitting patches to change the order we do things at S3
> suspend/resume, shouldn't we add something like "dev_priv->suspending"
> that could be used to avoid the strict ordering that is required
> during runtime?

Hm I was running with those changes and didn't see additional warnings,
so I'll have to look again.

I agree we want sensible warnings in place, and maybe removing this one
is too permissive, but I'd like to avoid a suspending flag if we can.

Maybe we just need to bundle up our assertions and check all the
relevant state after runtime suspend or S3 like we do for mode set
state in various places.  That would let us keep our warnings, but also
save us from having them spread out in code paths that get used in
many different contexts.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Paulo Zanoni]

2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Mon, 7 Jul 2014 18:48:47 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> (documenting what we discussed on IRC)
>>
>> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> > This was always the case on our suspend path, but it was recently
>> > exposed by the change to use our runtime IRQ disable routine rather than
>> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
>> > really would indicate a bug.
>>
>> While I understand the patch and think it's a reasonable thing to do,
>> I feel the need to spend some time persuading you in replacing it with
>> something that doesn't involve removing WARNs from our driver. While
>> the driver is runtime suspended, no one should really be manipulating
>> IRQs, even if they're disabling stuff that is already disabled: this
>> reflects there's probably a problem somewhere. These WARNs are
>> extremely helpful for the runtime PM feature because almost nobody
>> actually uses runtime PM to notice any bugs with it, so the WARNs can
>> make QA report bugs and bisect things for us.
>>
>> Also, it seems S3 suspend is currently a little disaster on our
>> driver. Your 6 patches just solve some of the WARNs, not all of them.
>> And last week I even solved another WARN on the S3 path. I just did
>> some investigation, and the current bad commits are:
>> 8abdc17941c71b37311bb93876ac83dce58160c8,
>> e11aa362308f5de467ce355a2a2471321b15a35c and
>> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
>> commits, S3 doesn't give me any WARNs.
>>
>> Instead of the change you proposed, can't we think of another solution
>> that would maybe address all the 3 regressions we have? Since we're
>> always submitting patches to change the order we do things at S3
>> suspend/resume, shouldn't we add something like "dev_priv->suspending"
>> that could be used to avoid the strict ordering that is required
>> during runtime?
>
> Hm I was running with those changes and didn't see additional warnings,
> so I'll have to look again.
>
> I agree we want sensible warnings in place, and maybe removing this one
> is too permissive, but I'd like to avoid a suspending flag if we can.
>
> Maybe we just need to bundle up our assertions and check all the
> relevant state after runtime suspend or S3 like we do for mode set
> state in various places.  That would let us keep our warnings, but also
> save us from having them spread out in code paths that get used in
> many different contexts.

I'm probably going to regret this, but since no one proposed a better
patch and the bug is still there:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Daniel Vetter]

On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote:
> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Mon, 7 Jul 2014 18:48:47 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> (documenting what we discussed on IRC)
> >>
> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> > This was always the case on our suspend path, but it was recently
> >> > exposed by the change to use our runtime IRQ disable routine rather than
> >> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
> >> > really would indicate a bug.
> >>
> >> While I understand the patch and think it's a reasonable thing to do,
> >> I feel the need to spend some time persuading you in replacing it with
> >> something that doesn't involve removing WARNs from our driver. While
> >> the driver is runtime suspended, no one should really be manipulating
> >> IRQs, even if they're disabling stuff that is already disabled: this
> >> reflects there's probably a problem somewhere. These WARNs are
> >> extremely helpful for the runtime PM feature because almost nobody
> >> actually uses runtime PM to notice any bugs with it, so the WARNs can
> >> make QA report bugs and bisect things for us.
> >>
> >> Also, it seems S3 suspend is currently a little disaster on our
> >> driver. Your 6 patches just solve some of the WARNs, not all of them.
> >> And last week I even solved another WARN on the S3 path. I just did
> >> some investigation, and the current bad commits are:
> >> 8abdc17941c71b37311bb93876ac83dce58160c8,
> >> e11aa362308f5de467ce355a2a2471321b15a35c and
> >> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
> >> commits, S3 doesn't give me any WARNs.
> >>
> >> Instead of the change you proposed, can't we think of another solution
> >> that would maybe address all the 3 regressions we have? Since we're
> >> always submitting patches to change the order we do things at S3
> >> suspend/resume, shouldn't we add something like "dev_priv->suspending"
> >> that could be used to avoid the strict ordering that is required
> >> during runtime?
> >
> > Hm I was running with those changes and didn't see additional warnings,
> > so I'll have to look again.
> >
> > I agree we want sensible warnings in place, and maybe removing this one
> > is too permissive, but I'd like to avoid a suspending flag if we can.
> >
> > Maybe we just need to bundle up our assertions and check all the
> > relevant state after runtime suspend or S3 like we do for mode set
> > state in various places.  That would let us keep our warnings, but also
> > save us from having them spread out in code paths that get used in
> > many different contexts.
> 
> I'm probably going to regret this, but since no one proposed a better
> patch and the bug is still there:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Not really eager to merge a patch soaking in preemptive regrets ...

I'll punt on this for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Paulo Zanoni]

2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote:
>> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> > On Mon, 7 Jul 2014 18:48:47 -0300
>> > Paulo Zanoni <przanoni@gmail.com> wrote:
>> >
>> >> (documenting what we discussed on IRC)
>> >>
>> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> >> > This was always the case on our suspend path, but it was recently
>> >> > exposed by the change to use our runtime IRQ disable routine rather than
>> >> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
>> >> > really would indicate a bug.
>> >>
>> >> While I understand the patch and think it's a reasonable thing to do,
>> >> I feel the need to spend some time persuading you in replacing it with
>> >> something that doesn't involve removing WARNs from our driver. While
>> >> the driver is runtime suspended, no one should really be manipulating
>> >> IRQs, even if they're disabling stuff that is already disabled: this
>> >> reflects there's probably a problem somewhere. These WARNs are
>> >> extremely helpful for the runtime PM feature because almost nobody
>> >> actually uses runtime PM to notice any bugs with it, so the WARNs can
>> >> make QA report bugs and bisect things for us.
>> >>
>> >> Also, it seems S3 suspend is currently a little disaster on our
>> >> driver. Your 6 patches just solve some of the WARNs, not all of them.
>> >> And last week I even solved another WARN on the S3 path. I just did
>> >> some investigation, and the current bad commits are:
>> >> 8abdc17941c71b37311bb93876ac83dce58160c8,
>> >> e11aa362308f5de467ce355a2a2471321b15a35c and
>> >> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
>> >> commits, S3 doesn't give me any WARNs.
>> >>
>> >> Instead of the change you proposed, can't we think of another solution
>> >> that would maybe address all the 3 regressions we have? Since we're
>> >> always submitting patches to change the order we do things at S3
>> >> suspend/resume, shouldn't we add something like "dev_priv->suspending"
>> >> that could be used to avoid the strict ordering that is required
>> >> during runtime?
>> >
>> > Hm I was running with those changes and didn't see additional warnings,
>> > so I'll have to look again.
>> >
>> > I agree we want sensible warnings in place, and maybe removing this one
>> > is too permissive, but I'd like to avoid a suspending flag if we can.
>> >
>> > Maybe we just need to bundle up our assertions and check all the
>> > relevant state after runtime suspend or S3 like we do for mode set
>> > state in various places.  That would let us keep our warnings, but also
>> > save us from having them spread out in code paths that get used in
>> > many different contexts.
>>
>> I'm probably going to regret this, but since no one proposed a better
>> patch and the bug is still there:
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Not really eager to merge a patch soaking in preemptive regrets ...
>
> I'll punt on this for now.

Possible solutions:

1 - Patch xxx_disable_vblank to do nothing in case dev_priv->pm.suspended
2 - Add a flag dev_priv->suspending and don't print those WARNs in
case it is set.
3 - Merge Jesse's original patch
4 - Something else?

I can implement any of the proposed solutions if needed...

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Daniel Vetter]

On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote:
> 2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote:
> >> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> > On Mon, 7 Jul 2014 18:48:47 -0300
> >> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >> >
> >> >> (documenting what we discussed on IRC)
> >> >>
> >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> >> > This was always the case on our suspend path, but it was recently
> >> >> > exposed by the change to use our runtime IRQ disable routine rather than
> >> >> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
> >> >> > really would indicate a bug.
> >> >>
> >> >> While I understand the patch and think it's a reasonable thing to do,
> >> >> I feel the need to spend some time persuading you in replacing it with
> >> >> something that doesn't involve removing WARNs from our driver. While
> >> >> the driver is runtime suspended, no one should really be manipulating
> >> >> IRQs, even if they're disabling stuff that is already disabled: this
> >> >> reflects there's probably a problem somewhere. These WARNs are
> >> >> extremely helpful for the runtime PM feature because almost nobody
> >> >> actually uses runtime PM to notice any bugs with it, so the WARNs can
> >> >> make QA report bugs and bisect things for us.
> >> >>
> >> >> Also, it seems S3 suspend is currently a little disaster on our
> >> >> driver. Your 6 patches just solve some of the WARNs, not all of them.
> >> >> And last week I even solved another WARN on the S3 path. I just did
> >> >> some investigation, and the current bad commits are:
> >> >> 8abdc17941c71b37311bb93876ac83dce58160c8,
> >> >> e11aa362308f5de467ce355a2a2471321b15a35c and
> >> >> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
> >> >> commits, S3 doesn't give me any WARNs.
> >> >>
> >> >> Instead of the change you proposed, can't we think of another solution
> >> >> that would maybe address all the 3 regressions we have? Since we're
> >> >> always submitting patches to change the order we do things at S3
> >> >> suspend/resume, shouldn't we add something like "dev_priv->suspending"
> >> >> that could be used to avoid the strict ordering that is required
> >> >> during runtime?
> >> >
> >> > Hm I was running with those changes and didn't see additional warnings,
> >> > so I'll have to look again.
> >> >
> >> > I agree we want sensible warnings in place, and maybe removing this one
> >> > is too permissive, but I'd like to avoid a suspending flag if we can.
> >> >
> >> > Maybe we just need to bundle up our assertions and check all the
> >> > relevant state after runtime suspend or S3 like we do for mode set
> >> > state in various places.  That would let us keep our warnings, but also
> >> > save us from having them spread out in code paths that get used in
> >> > many different contexts.
> >>
> >> I'm probably going to regret this, but since no one proposed a better
> >> patch and the bug is still there:
> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Not really eager to merge a patch soaking in preemptive regrets ...
> >
> > I'll punt on this for now.
> 
> Possible solutions:
> 
> 1 - Patch xxx_disable_vblank to do nothing in case dev_priv->pm.suspended
> 2 - Add a flag dev_priv->suspending and don't print those WARNs in
> case it is set.
> 3 - Merge Jesse's original patch
> 4 - Something else?
> 
> I can implement any of the proposed solutions if needed...

I've gone with 3 for now. It sounds like we need to work with this code a
bit more still until the best solution is clear. The patch at least
addresses the WARN.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Paulo Zanoni]

2014-07-17 5:06 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote:
>> 2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>> > On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote:
>> >> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> >> > On Mon, 7 Jul 2014 18:48:47 -0300
>> >> > Paulo Zanoni <przanoni@gmail.com> wrote:
>> >> >
>> >> >> (documenting what we discussed on IRC)
>> >> >>
>> >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> >> >> > This was always the case on our suspend path, but it was recently
>> >> >> > exposed by the change to use our runtime IRQ disable routine rather than
>> >> >> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
>> >> >> > really would indicate a bug.
>> >> >>
>> >> >> While I understand the patch and think it's a reasonable thing to do,
>> >> >> I feel the need to spend some time persuading you in replacing it with
>> >> >> something that doesn't involve removing WARNs from our driver. While
>> >> >> the driver is runtime suspended, no one should really be manipulating
>> >> >> IRQs, even if they're disabling stuff that is already disabled: this
>> >> >> reflects there's probably a problem somewhere. These WARNs are
>> >> >> extremely helpful for the runtime PM feature because almost nobody
>> >> >> actually uses runtime PM to notice any bugs with it, so the WARNs can
>> >> >> make QA report bugs and bisect things for us.
>> >> >>
>> >> >> Also, it seems S3 suspend is currently a little disaster on our
>> >> >> driver. Your 6 patches just solve some of the WARNs, not all of them.
>> >> >> And last week I even solved another WARN on the S3 path. I just did
>> >> >> some investigation, and the current bad commits are:
>> >> >> 8abdc17941c71b37311bb93876ac83dce58160c8,
>> >> >> e11aa362308f5de467ce355a2a2471321b15a35c and
>> >> >> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
>> >> >> commits, S3 doesn't give me any WARNs.
>> >> >>
>> >> >> Instead of the change you proposed, can't we think of another solution
>> >> >> that would maybe address all the 3 regressions we have? Since we're
>> >> >> always submitting patches to change the order we do things at S3
>> >> >> suspend/resume, shouldn't we add something like "dev_priv->suspending"
>> >> >> that could be used to avoid the strict ordering that is required
>> >> >> during runtime?
>> >> >
>> >> > Hm I was running with those changes and didn't see additional warnings,
>> >> > so I'll have to look again.
>> >> >
>> >> > I agree we want sensible warnings in place, and maybe removing this one
>> >> > is too permissive, but I'd like to avoid a suspending flag if we can.
>> >> >
>> >> > Maybe we just need to bundle up our assertions and check all the
>> >> > relevant state after runtime suspend or S3 like we do for mode set
>> >> > state in various places.  That would let us keep our warnings, but also
>> >> > save us from having them spread out in code paths that get used in
>> >> > many different contexts.
>> >>
>> >> I'm probably going to regret this, but since no one proposed a better
>> >> patch and the bug is still there:
>> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > Not really eager to merge a patch soaking in preemptive regrets ...
>> >
>> > I'll punt on this for now.
>>
>> Possible solutions:
>>
>> 1 - Patch xxx_disable_vblank to do nothing in case dev_priv->pm.suspended
>> 2 - Add a flag dev_priv->suspending and don't print those WARNs in
>> case it is set.
>> 3 - Merge Jesse's original patch
>> 4 - Something else?
>>
>> I can implement any of the proposed solutions if needed...
>
> I've gone with 3 for now. It sounds like we need to work with this code a
> bit more still until the best solution is clear. The patch at least
> addresses the WARN.

Ok, so after I wrote this email, I remembered Ville's "[10/24]
drm/i915: Leave interrupts enabled while disabling crtcs during
suspend", which is part of the watermarks series. Based on that, I
produced the following fix that would replace Jesse's current patch
series, fixing the bad WARN while keeping it in the places we want:

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3315358..63675bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -520,12 +520,6 @@ static int i915_drm_freeze(struct drm_device *dev)
                        return error;
                }

-               flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
-               intel_runtime_pm_disable_interrupts(dev);
-
-               intel_suspend_gt_powersave(dev);
-
                /*
                 * Disable CRTCs directly since we want to preserve sw state
                 * for _thaw. Also, power gate the CRTC power wells.
@@ -535,6 +529,11 @@ static int i915_drm_freeze(struct drm_device *dev)
                        intel_crtc_control(crtc, false);
                drm_modeset_unlock_all(dev);

+               flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+               intel_runtime_pm_disable_interrupts(dev);
+
+               intel_suspend_gt_powersave(dev);
+
                intel_modeset_suspend_hw(dev);
        }


What do you think?

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

[Daniel Vetter]

On Thu, Jul 17, 2014 at 3:42 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote:
>>> 2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>>> > On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote:
>>> >> 2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>>> >> > On Mon, 7 Jul 2014 18:48:47 -0300
>>> >> > Paulo Zanoni <przanoni@gmail.com> wrote:
>>> >> >
>>> >> >> (documenting what we discussed on IRC)
>>> >> >>
>>> >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
>>> >> >> > This was always the case on our suspend path, but it was recently
>>> >> >> > exposed by the change to use our runtime IRQ disable routine rather than
>>> >> >> > the full DRM IRQ disable.  Keep the warning on the enable side, as that
>>> >> >> > really would indicate a bug.
>>> >> >>
>>> >> >> While I understand the patch and think it's a reasonable thing to do,
>>> >> >> I feel the need to spend some time persuading you in replacing it with
>>> >> >> something that doesn't involve removing WARNs from our driver. While
>>> >> >> the driver is runtime suspended, no one should really be manipulating
>>> >> >> IRQs, even if they're disabling stuff that is already disabled: this
>>> >> >> reflects there's probably a problem somewhere. These WARNs are
>>> >> >> extremely helpful for the runtime PM feature because almost nobody
>>> >> >> actually uses runtime PM to notice any bugs with it, so the WARNs can
>>> >> >> make QA report bugs and bisect things for us.
>>> >> >>
>>> >> >> Also, it seems S3 suspend is currently a little disaster on our
>>> >> >> driver. Your 6 patches just solve some of the WARNs, not all of them.
>>> >> >> And last week I even solved another WARN on the S3 path. I just did
>>> >> >> some investigation, and the current bad commits are:
>>> >> >> 8abdc17941c71b37311bb93876ac83dce58160c8,
>>> >> >> e11aa362308f5de467ce355a2a2471321b15a35c and
>>> >> >> 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
>>> >> >> commits, S3 doesn't give me any WARNs.
>>> >> >>
>>> >> >> Instead of the change you proposed, can't we think of another solution
>>> >> >> that would maybe address all the 3 regressions we have? Since we're
>>> >> >> always submitting patches to change the order we do things at S3
>>> >> >> suspend/resume, shouldn't we add something like "dev_priv->suspending"
>>> >> >> that could be used to avoid the strict ordering that is required
>>> >> >> during runtime?
>>> >> >
>>> >> > Hm I was running with those changes and didn't see additional warnings,
>>> >> > so I'll have to look again.
>>> >> >
>>> >> > I agree we want sensible warnings in place, and maybe removing this one
>>> >> > is too permissive, but I'd like to avoid a suspending flag if we can.
>>> >> >
>>> >> > Maybe we just need to bundle up our assertions and check all the
>>> >> > relevant state after runtime suspend or S3 like we do for mode set
>>> >> > state in various places.  That would let us keep our warnings, but also
>>> >> > save us from having them spread out in code paths that get used in
>>> >> > many different contexts.
>>> >>
>>> >> I'm probably going to regret this, but since no one proposed a better
>>> >> patch and the bug is still there:
>>> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> >
>>> > Not really eager to merge a patch soaking in preemptive regrets ...
>>> >
>>> > I'll punt on this for now.
>>>
>>> Possible solutions:
>>>
>>> 1 - Patch xxx_disable_vblank to do nothing in case dev_priv->pm.suspended
>>> 2 - Add a flag dev_priv->suspending and don't print those WARNs in
>>> case it is set.
>>> 3 - Merge Jesse's original patch
>>> 4 - Something else?
>>>
>>> I can implement any of the proposed solutions if needed...
>>
>> I've gone with 3 for now. It sounds like we need to work with this code a
>> bit more still until the best solution is clear. The patch at least
>> addresses the WARN.
>
> Ok, so after I wrote this email, I remembered Ville's "[10/24]
> drm/i915: Leave interrupts enabled while disabling crtcs during
> suspend", which is part of the watermarks series. Based on that, I
> produced the following fix that would replace Jesse's current patch
> series, fixing the bad WARN while keeping it in the places we want:
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3315358..63675bf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -520,12 +520,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>                         return error;
>                 }
>
> -               flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> -               intel_runtime_pm_disable_interrupts(dev);
> -
> -               intel_suspend_gt_powersave(dev);
> -
>                 /*
>                  * Disable CRTCs directly since we want to preserve sw state
>                  * for _thaw. Also, power gate the CRTC power wells.
> @@ -535,6 +529,11 @@ static int i915_drm_freeze(struct drm_device *dev)
>                         intel_crtc_control(crtc, false);
>                 drm_modeset_unlock_all(dev);
>
> +               flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +               intel_runtime_pm_disable_interrupts(dev);
> +
> +               intel_suspend_gt_powersave(dev);
> +
>                 intel_modeset_suspend_hw(dev);
>         }
>
>
> What do you think?

Yeah, that means we can keep using the power domain code when shutting
down pipes, so more common code. And we need it anyway to make Ville
happy. Since I'm only going to freeze down dinq next week for -testing
I'll keep Jesse patches for now until he's back from vacation and can
review your patch. But please polish this one with commit message and
all.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch