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

[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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch