| Message ID | 1405629826-3455-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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 --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) {