diff mbox

drm/i915: freeze display before the interrupts and GT

Message ID 1405629826-3455-1-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni July 17, 2014, 8:43 p.m. UTC
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Since we started using intel_runtime_pm_disable_interrupts() at normal
(non-runtime) suspend/resume, we had to remove a WARN from
ironlake_disable_display_irq to avoid a case where we were doing the
correct thing and the WARN was not really needed. The problem is that
the WARN was useful in other cases, and its removal can hide some bugs
that we would catch automatically.

To be able to add back the WARN, we have to call intel_crtc_control()
before interrupts are disabled, which is what this patch currently
does.

Also notice that Ville's patch from the Watermarks series "drm/i915:
Leave interrupts enabled while disabling crtcs during suspend" also
did a change that's equivalent to the one we're doing on this patch,
with the exception that its original patch, when applied to the
current tree, procduces a WARN.

Related commits:

commit daa390e5ee45cc051d6bf37b296901f2f92b002d
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
    drm/i915: don't warn if IRQs are disabled when shutting down display IRQs

commit e11aa362308f5de467ce355a2a2471321b15a35c
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
    drm/i915: use runtime irq suspend/resume in freeze/thaw

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 11 +++++------
 drivers/gpu/drm/i915/i915_irq.c |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

No need to revert anything :)

Comments

Jesse Barnes July 29, 2014, 6:13 p.m. UTC | #1
On Thu, 17 Jul 2014 17:43:46 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Since we started using intel_runtime_pm_disable_interrupts() at normal
> (non-runtime) suspend/resume, we had to remove a WARN from
> ironlake_disable_display_irq to avoid a case where we were doing the
> correct thing and the WARN was not really needed. The problem is that
> the WARN was useful in other cases, and its removal can hide some bugs
> that we would catch automatically.
> 
> To be able to add back the WARN, we have to call intel_crtc_control()
> before interrupts are disabled, which is what this patch currently
> does.
> 
> Also notice that Ville's patch from the Watermarks series "drm/i915:
> Leave interrupts enabled while disabling crtcs during suspend" also
> did a change that's equivalent to the one we're doing on this patch,
> with the exception that its original patch, when applied to the
> current tree, procduces a WARN.
> 
> Related commits:
> 
> commit daa390e5ee45cc051d6bf37b296901f2f92b002d
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>     drm/i915: don't warn if IRQs are disabled when shutting down display IRQs
> 
> commit e11aa362308f5de467ce355a2a2471321b15a35c
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>     drm/i915: use runtime irq suspend/resume in freeze/thaw
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 11 +++++------
>  drivers/gpu/drm/i915/i915_irq.c |  2 +-
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> No need to revert anything :)
> 
> 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);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 20c777c..af231ba 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 (!intel_irqs_enabled(dev_priv))
> +	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>  		return;
>  
>  	if ((dev_priv->irq_mask & mask) != mask) {

So with this added, we get the WARN back but don't need/want to revert
any other patches?

Did you check boot, suspend/resume, and runtime PM for warnings with
this change applied?  If so, looks fine:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Daniel Vetter July 29, 2014, 6:59 p.m. UTC | #2
On Tue, Jul 29, 2014 at 11:13:03AM -0700, Jesse Barnes wrote:
> On Thu, 17 Jul 2014 17:43:46 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Since we started using intel_runtime_pm_disable_interrupts() at normal
> > (non-runtime) suspend/resume, we had to remove a WARN from
> > ironlake_disable_display_irq to avoid a case where we were doing the
> > correct thing and the WARN was not really needed. The problem is that
> > the WARN was useful in other cases, and its removal can hide some bugs
> > that we would catch automatically.
> > 
> > To be able to add back the WARN, we have to call intel_crtc_control()
> > before interrupts are disabled, which is what this patch currently
> > does.
> > 
> > Also notice that Ville's patch from the Watermarks series "drm/i915:
> > Leave interrupts enabled while disabling crtcs during suspend" also
> > did a change that's equivalent to the one we're doing on this patch,
> > with the exception that its original patch, when applied to the
> > current tree, procduces a WARN.
> > 
> > Related commits:
> > 
> > commit daa390e5ee45cc051d6bf37b296901f2f92b002d
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> >     drm/i915: don't warn if IRQs are disabled when shutting down display IRQs
> > 
> > commit e11aa362308f5de467ce355a2a2471321b15a35c
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> >     drm/i915: use runtime irq suspend/resume in freeze/thaw
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 11 +++++------
> >  drivers/gpu/drm/i915/i915_irq.c |  2 +-
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > No need to revert anything :)
> > 
> > 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);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 20c777c..af231ba 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 (!intel_irqs_enabled(dev_priv))
> > +	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> >  		return;
> >  
> >  	if ((dev_priv->irq_mask & mask) != mask) {
> 
> So with this added, we get the WARN back but don't need/want to revert
> any other patches?
> 
> Did you check boot, suspend/resume, and runtime PM for warnings with
> this change applied?  If so, looks fine:
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

There was a bit of conflict with one of Dave's fixes for the dp mst work,
but I think that was just partial of this patch here. But please
double-check.

Queued for -next, thanks for the patch.
-Daniel
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

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);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 20c777c..af231ba 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 (!intel_irqs_enabled(dev_priv))
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
 	if ((dev_priv->irq_mask & mask) != mask) {