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
