| Message ID | 1403281762-1927-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(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
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.
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
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
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
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
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) {
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(-)