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

[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.

-- 
Jesse Barnes, Intel Open Source Technology Center