2014-07-07 18:53 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: > On Mon, 7 Jul 2014 18:48:47 -0300 > Paulo Zanoni <przanoni@xxxxxxxxx> wrote: > >> (documenting what we discussed on IRC) >> >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: >> > 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@xxxxxxxxx> > > -- > Jesse Barnes, Intel Open Source Technology Center -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
