On Wed, Aug 27, 2014 at 9:59 PM, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote: > 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. I think that amount of testing is totally out of balance to keep a rather tiny warninig alive in the driver load code. The major use for the pm._irqs_disabled is to catch runtime pm bugs where code uses changes irq settings while the chip is off. That part will still be 100% effective with Jesse's patch. So I recommended that we instead merge Jesses patch instead of mine for -fixes. Jesse's patch has my r-b fwiw. -Daniel > > 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
