[Intel-gfx] [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt

Fri Aug 29 09:08:20 CEST 2014

I have tested both Jesse and Daniel's patch.

On IVB both two will introduce a new warning.
On HSW Daniel's patch can cause black screen.
On BDW both two patch can cause black screen.

Thanks
  --Sun, Yi

> -----Original Message-----
> From: Jesse Barnes [mailto:jbarnes at virtuousgeek.org]
> Sent: Thursday, August 28, 2014 3:59 AM
> To: Daniel Vetter
> Cc: intel-gfx; Oliver Hartkopp; Sun, Yi
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/ilk: special case enabling of
> PCU_EVENT interrupt
> 
> Yi, can you get this one run through testing on multiple platforms?  We just
> want to make sure there's not some path we missed that's gonna spew a
> warning with this change.
> 
> Thanks,
> Jesse
> 
> On Tue, 26 Aug 2014 22:51:13 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> >
> > On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes at virtuousgeek.org>
> wrote:
> > > On Tue, 26 Aug 2014 09:23:54 +0200
> > > Daniel Vetter <daniel at 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 at 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.
> >
> > Oliver, can you please test the below diff?
> >
> > Thanks, Daniel
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c index f19dbff0e73b..915a60b48159
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct
> > drm_device *dev)
> >
> >  	intel_power_domains_init_hw(dev_priv);
> >
> > +	/*
> > +	 * We enable some interrupt sources in our postinstall hooks, so mark
> > +	 * interrupts as enabled _before_ actually enabling them to avoid
> > +	 * special cases in our ordering checks.
> > +	 */
> > +	dev_priv->pm._irqs_disabled = false;
> > +
> >  	ret = drm_irq_install(dev, dev->pdev->irq);
> >  	if (ret)
> >  		goto cleanup_gem_stolen;
> >
> > -	dev_priv->pm._irqs_disabled = false;
> > -
> >  	/* Important: The output setup functions called by modeset_init need
> >  	 * working irqs for e.g. gmbus and dp aux transfers. */
> >  	intel_modeset_init(dev);
> 
> 
> --
> Jesse Barnes, Intel Open Source Technology Center