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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

_________________________________________________________________________________________________________________
|	        |  Bad commit      |              Jesse's patch                             |   Dainel's patch             |
|IVB	        |  irq WARN on boot  | No irq warning, but a new one 'WARNING! power/level is deprecated; use power/control instead'   |
|HSW       |  irq WARN on boot  |              No irq warning                           |   black screen               |
|SNB        |  irq WARN on boot  |	          No irq warning                           |   No irq warning             |
|BDW       |  irq warn on boot   |              black screen                             |   black screen               |
|___________|___________________|____________________________________________________|____________________________|


Thanks
  --Sun, Yi

> -----Original Message-----
> From: Jesse Barnes [mailto:jbarnes@xxxxxxxxxxxxxxxx]
> Sent: Thursday, August 28, 2014 3:59 AM
> To: Daniel Vetter
> Cc: intel-gfx; Oliver Hartkopp; Sun, Yi
> Subject: Re:  [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@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.
> >
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux