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

[Paulo Zanoni]

2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes at virtuousgeek.org>:
> On Mon, 7 Jul 2014 18:48:47 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
>
>> (documenting what we discussed on IRC)
>>
>> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes at 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 at intel.com>

>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni