Re: [PATCH] drm/i915: Fix irq enable tracking in driver load

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 04 Sep 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 27 Aug 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> A bunch of warnings fire on some ->irq_postinstall hooks since those
>> can enable interrupts (e.g. rps interrupts). And then our ordering
>> self-checks fire and complain.
>>
>> To fix that set the tracking boolen before enabling the irqs witho
>> drm_irq_install. Quoting the discussion with Jesse why that's safe:
>
> Yi Sun's testing result needs to be addressed one way or another before
> merging this:

...before merging this *or* Jesse's patch, I mean.

>
> http://mid.gmane.org/D9F66AA509623343B6A9A3D4502D5A52112B0676@SHSMSX101.ccr.corp.intel.com
>
> BR,
> Jani.
>
>
>>
>> On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>> Yes, it might work, but if you look through the history, we set this
>>> field carefully; first to true in the irq_init code, then to false only
>>> after the irq_install completes.  So I think your fragility arguments
>>> apply to this change too.
>>
>> Well we've done it in 4 commits or so, but currently we have:
>>
>> - Set irqs_disabled to true early in driver load to make sure checks
>> that. That's done in irq_init, which is totally not the function that
>> enables interrupts, only the function that initializes all the vtables
>> and similar things. We actually have a fairly sane naming scheme
>> nowadays (not fully consistent ofc): _init is sw setup,
>> _enable/_hw_init is the actual hw setup. That is done in
>> 95f25beddba2ec9510b249740bacc11eca70cf75
>>
>> - Set irqs_disabled to false right after the irqs are actually
>> enabled. This is done in ed2e6df18935beb3d63613c50103bf9757b2aa85
>>
>> So my change should only move the flag change over the ->preinstall
>> and ->postinstall hooks. I've done a little audit and didn't spot
>> anything amiss. Furthermore the runtime pm setup already clears
>> irqs_disabled _before_ calling these two hooks.
>>
>> This regression has been introduced in
>>
>> commit ed2e6df18935beb3d63613c50103bf9757b2aa85
>> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Date:   Fri Jun 20 09:39:36 2014 -0700
>>
>>     drm/i915: clear pm._irqs_disabled field after installing IRQs
>>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/i915/i915_dma.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 3b24e3bb2106..498980661b2f 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);
>> -- 
>> 2.0.1
>>
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center