Re: [PATCH] drm/i915: Fix irq enable tracking in driver load

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 27 Aug 2014 19:43:57 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Aug 27, 2014 at 10:11:34AM +0200, Daniel Vetter wrote:
> > A bunch of warnings fire on some ->irq_postinstall hooks since those
> > can enable interrupts (e.g. rps interrupts). And then our ordering
> > self-checks fire and complain.
> > 
> > To fix that set the tracking boolen before enabling the irqs with
> > drm_irq_install. Quoting the discussion with Jesse why that's safe:
> > 
> > On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > 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.
> > 
> > This regression has been introduced in
> > 
> > commit ed2e6df18935beb3d63613c50103bf9757b2aa85
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Fri Jun 20 09:39:36 2014 -0700
> > 
> >     drm/i915: clear pm._irqs_disabled field after installing IRQs
> > 
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> > Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # gm45, ilk
> 
> gm45 because it has been one of my troublesome machines in the past wrt
> to modeset init. ilk because it exhibits this bug.
> -Chris
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center