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

[Paulo Zanoni]

2014-07-14 14:34 GMT-03:00 Daniel Vetter <daniel at 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 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>
>
> 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



-- 
Paulo Zanoni