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

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

[Daniel Vetter]

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 with
drm_irq_install. Quoting the discussion with Jesse why that's safe:

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

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

[Chris Wilson]

On Wed, Aug 27, 2014 at 10:11:34AM +0200, Daniel Vetter 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 with
> drm_irq_install. Quoting the discussion with Jesse why that's safe:
> 
> 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>

Well it doesn't break gm45 at least, which is my fear every time the
interrupt gets touched inside modeset_init.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Wed, Aug 27, 2014 at 10:11:34AM +0200, Daniel Vetter 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 with
> drm_irq_install. Quoting the discussion with Jesse why that's safe:
> 
> 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>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # gm45, ilk

gm45 because it has been one of my troublesome machines in the past wrt
to modeset init. ilk because it exhibits this bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jani Nikula]

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:

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

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

[Jani Nikula]

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

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

[Daniel Vetter]

On Thu, Sep 04, 2014 at 02:12:10PM +0300, Jani Nikula 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:
> 
> http://mid.gmane.org/D9F66AA509623343B6A9A3D4502D5A52112B0676@SHSMSX101.ccr.corp.intel.com

Shrug it off as an unstable test result. Both mine and Jesse's patch
really only change the logic we use to WARN about interrupt state. We
don't use pm._irqs_disabled for anything else at all.

Which means that black screen is at most a timing issue. Or the baseline
kernels don't perfectly match (the new warning in Jesse's patch is a bit
an indicator for that).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Jani Nikula]

On Thu, 04 Sep 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Sep 04, 2014 at 02:12:10PM +0300, Jani Nikula 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:
>> 
>> http://mid.gmane.org/D9F66AA509623343B6A9A3D4502D5A52112B0676@SHSMSX101.ccr.corp.intel.com
>
> Shrug it off as an unstable test result. Both mine and Jesse's patch
> really only change the logic we use to WARN about interrupt state. We
> don't use pm._irqs_disabled for anything else at all.

Okay, so this is a PITA to review, but at least
ironlake_enable_display_irq will behave differently during
drm_irq_install because of this patch.

Jani.

> Which means that black screen is at most a timing issue. Or the baseline
> kernels don't perfectly match (the new warning in Jesse's patch is a bit
> an indicator for that).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Daniel Vetter]

On Thu, Sep 04, 2014 at 04:42:36PM +0300, Jani Nikula wrote:
> On Thu, 04 Sep 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Sep 04, 2014 at 02:12:10PM +0300, Jani Nikula 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:
> >> 
> >> http://mid.gmane.org/D9F66AA509623343B6A9A3D4502D5A52112B0676@SHSMSX101.ccr.corp.intel.com
> >
> > Shrug it off as an unstable test result. Both mine and Jesse's patch
> > really only change the logic we use to WARN about interrupt state. We
> > don't use pm._irqs_disabled for anything else at all.
> 
> Okay, so this is a PITA to review, but at least
> ironlake_enable_display_irq will behave differently during
> drm_irq_install because of this patch.

Oops, I've somehow completely missed the early return in there. That means
we actually break the ironlake rps setup done in postinstall without any
of these patches. I've mixed this up with the pipestat check I've done
where I just WARN, but don't bail out.

tbh not sure why we bail out, at worst we'll get a few unclaimed register
warnings.

The only other place is if we get an interrupt right away, but that means
the preinstall hook has a bug somewhere. So the only place where behaviour
chances is still only ironlake, so still no explanation why hsw/bdw
suddenly start to fall over all together.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Jesse Barnes]

On Wed, 27 Aug 2014 19:43:57 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Aug 27, 2014 at 10:11:34AM +0200, Daniel Vetter 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 with
> > drm_irq_install. Quoting the discussion with Jesse why that's safe:
> > 
> > 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>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # gm45, ilk
> 
> gm45 because it has been one of my troublesome machines in the past wrt
> to modeset init. ilk because it exhibits this bug.
> -Chris
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jani Nikula]

On Thu, 04 Sep 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Sep 04, 2014 at 04:42:36PM +0300, Jani Nikula wrote:
>> On Thu, 04 Sep 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Thu, Sep 04, 2014 at 02:12:10PM +0300, Jani Nikula 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:
>> >> 
>> >> http://mid.gmane.org/D9F66AA509623343B6A9A3D4502D5A52112B0676@SHSMSX101.ccr.corp.intel.com
>> >
>> > Shrug it off as an unstable test result. Both mine and Jesse's patch
>> > really only change the logic we use to WARN about interrupt state. We
>> > don't use pm._irqs_disabled for anything else at all.
>> 
>> Okay, so this is a PITA to review, but at least
>> ironlake_enable_display_irq will behave differently during
>> drm_irq_install because of this patch.
>
> Oops, I've somehow completely missed the early return in there. That means
> we actually break the ironlake rps setup done in postinstall without any
> of these patches. I've mixed this up with the pipestat check I've done
> where I just WARN, but don't bail out.
>
> tbh not sure why we bail out, at worst we'll get a few unclaimed register
> warnings.
>
> The only other place is if we get an interrupt right away, but that means
> the preinstall hook has a bug somewhere. So the only place where behaviour
> chances is still only ironlake, so still no explanation why hsw/bdw
> suddenly start to fall over all together.

Pushed to drm-intel-fixes. Thanks for the patch, testing and review.

BR,
Jani.



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

-- 
Jani Nikula, Intel Open Source Technology Center