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

[Daniel Vetter <daniel@ffwll.ch>]

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);
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch