[Intel-gfx] [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt

[Daniel Vetter]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch