On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote: > 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. Well we've done it in 4 commits or so, but currently we have: - Set irqs_disabled to true early in driver load to make sure checks that. That's done in irq_init, which is totally not the function that enables interrupts, only the function that initializes all the vtables and similar things. We actually have a fairly sane naming scheme nowadays (not fully consistent ofc): _init is sw setup, _enable/_hw_init is the actual hw setup. That is done in 95f25beddba2ec9510b249740bacc11eca70cf75 - Set irqs_disabled to false right after the irqs are actually enabled. This is done in ed2e6df18935beb3d63613c50103bf9757b2aa85 So my change should only move the flag change over the ->preinstall and ->postinstall hooks. I've done a little audit and didn't spot anything amiss. Furthermore the runtime pm setup already clears irqs_disabled _before_ calling these two hooks. So I think overall this is solid. And now I've also written the commit message for the patch if it tests out on Oliver's box ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
