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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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@ffwll.ch> wrote:

> 
> On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Tue, 26 Aug 2014 09:23:54 +0200
> > Daniel Vetter <daniel@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@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