Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 26 Aug 2014 22:51:13 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> 
> On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Tue, 26 Aug 2014 09:23:54 +0200
> > Daniel Vetter <daniel@ffwll.ch> 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@virtuousgeek.org>
> >>
> >> 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