On Tue, 26 Aug 2014 22:51:13 +0200 Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote: > > On Tue, 26 Aug 2014 09:23:54 +0200 > > Daniel Vetter <daniel@xxxxxxxx> wrote: > > > >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > >> > a backtrace and keep the rest of the IRQ tracking code happy. > >> > > >> > Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> > >> > >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c > >> right above the drm_irq_install call? In > >> intel_runtime_pm_restore_interrupts we also set it to false before we call > >> the various hooks. > > > > I didn't check on all the paths whether that was safe, we have a lot of > > warnings. > > > > And really this init time thing is a special case, so it made sense to > > me. > > Well I fully agree that your patch is the safe option and much easier to > review, too. > > But driver load/resume are the most fragile paths we have in our codebase > - you look at them and it falls apart. And we have absolutely no handle on > these issues on a fundamental level at all (compared to other areas where > we reworked the code or added enough tests to pretty much kill entire > classes of regressions). The only half-assed thing we can do is try to not > have too much complexity (so that you can still understand it, we're > probably over that already) and lock down the ordering and other > constraints with piles of really loud WARN_ON asserts. > > Your patch both removes WARN_ONs from these codepaths and adds special > cases, so falls a bit short on those metrics. And if I'm not mistaken > (like I've said the code is too complex by now to really understand) the > below change should get us there, too. So I want to see whether that > wouldn't work before going with your patch. Yes, it might work, but if you look through the history, we set this field carefully; first to true in the irq_init code, then to false only after the irq_install completes. So I think your fragility arguments apply to this change too. -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
