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

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

[Jesse Barnes]

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>
---
 drivers/gpu/drm/i915/i915_irq.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d5445e7..ec1d9fe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -132,6 +132,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 
 /* For display hotplug interrupt */
 static void
+ironlake_enable_display_irq_nowarn(struct drm_i915_private *dev_priv, u32 mask)
+{
+	if ((dev_priv->irq_mask & mask) != 0) {
+		dev_priv->irq_mask &= ~mask;
+		I915_WRITE(DEIMR, dev_priv->irq_mask);
+		POSTING_READ(DEIMR);
+	}
+}
+
+static void
 ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
@@ -139,11 +149,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
 	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
-	if ((dev_priv->irq_mask & mask) != 0) {
-		dev_priv->irq_mask &= ~mask;
-		I915_WRITE(DEIMR, dev_priv->irq_mask);
-		POSTING_READ(DEIMR);
-	}
+	ironlake_enable_display_irq_nowarn(dev_priv, mask);
 }
 
 static void
@@ -3665,7 +3671,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 		 * setup is guaranteed to run in single-threaded context. But we
 		 * need it to make the assert_spin_locked happy. */
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-		ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT);
+		ironlake_enable_display_irq_nowarn(dev_priv, DE_PCU_EVENT);
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 	}
 
-- 
1.7.5.4

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

[Oliver Hartkopp]

On 26.08.2014 01:24, 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>

Reported-by: Oliver Hartkopp <socketcan@hartkopp.net>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

Yeah. That fixed it.

Thanks Jesse!

[    2.900147] Linux agpgart interface v0.103
[    2.900475] agpgart-intel 0000:00:00.0: Intel HD Graphics Chipset
[    2.900579] agpgart-intel 0000:00:00.0: detected gtt size: 2097152K total, 262144K mappable
[    2.901354] agpgart-intel 0000:00:00.0: detected 32768K stolen memory
[    2.901743] agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0xe0000000
[    2.901995] [drm] Initialized drm 1.1.0 20060810
[    2.904212] [drm] Memory usable by graphics device = 2048M
[    2.904295] [drm] Replacing VGA console driver
[    2.904961] Console: switching to colour dummy device 80x25
[    2.937950] i915 0000:00:02.0: irq 24 for MSI/MSI-X
[    2.937962] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[    2.937965] [drm] Driver supports precise vblank timestamp query.
[    2.938310] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem
[    3.030687] fbcon: inteldrmfb (fb0) is primary device
[    3.494467] tsc: Refined TSC clocksource calibration: 2792.999 MHz
[    4.176644] [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up
[    4.495814] Switched to clocksource tsc
[    4.529922] Console: switching to colour frame buffer device 240x67
[    4.536702] i915 0000:00:02.0: fb0: inteldrmfb frame buffer device
[    4.536769] i915 0000:00:02.0: registered panic notifier
[    4.548333] ACPI: Video Device [VID2] (multi-head: yes  rom: no  post: no)
[    4.900233] acpi device:45: registered as cooling_device0
[    4.924178] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:02/input/input4
[    4.924371] [drm] Initialized i915 1.6.0 20140725 for 0000:00:02.0 on minor 0


Btw. do you know what 

[drm:intel_dp_start_link_train] *ERROR* too many full retries, give up

is about?

Regards,
Oliver

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d5445e7..ec1d9fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -132,6 +132,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  
>  /* For display hotplug interrupt */
>  static void
> +ironlake_enable_display_irq_nowarn(struct drm_i915_private *dev_priv, u32 mask)
> +{
> +	if ((dev_priv->irq_mask & mask) != 0) {
> +		dev_priv->irq_mask &= ~mask;
> +		I915_WRITE(DEIMR, dev_priv->irq_mask);
> +		POSTING_READ(DEIMR);
> +	}
> +}
> +
> +static void
>  ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  {
>  	assert_spin_locked(&dev_priv->irq_lock);
> @@ -139,11 +149,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>  		return;
>  
> -	if ((dev_priv->irq_mask & mask) != 0) {
> -		dev_priv->irq_mask &= ~mask;
> -		I915_WRITE(DEIMR, dev_priv->irq_mask);
> -		POSTING_READ(DEIMR);
> -	}
> +	ironlake_enable_display_irq_nowarn(dev_priv, mask);
>  }
>  
>  static void
> @@ -3665,7 +3671,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  		 * setup is guaranteed to run in single-threaded context. But we
>  		 * need it to make the assert_spin_locked happy. */
>  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -		ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT);
> +		ironlake_enable_display_irq_nowarn(dev_priv, DE_PCU_EVENT);
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  	}
>  
> 

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

[Daniel Vetter]

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.

Also the commit message is a bit thin on the usual details like which
commits introduced this regression, so that maintainers know where to
apply this to.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d5445e7..ec1d9fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -132,6 +132,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  
>  /* For display hotplug interrupt */
>  static void
> +ironlake_enable_display_irq_nowarn(struct drm_i915_private *dev_priv, u32 mask)
> +{
> +	if ((dev_priv->irq_mask & mask) != 0) {
> +		dev_priv->irq_mask &= ~mask;
> +		I915_WRITE(DEIMR, dev_priv->irq_mask);
> +		POSTING_READ(DEIMR);
> +	}
> +}
> +
> +static void
>  ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  {
>  	assert_spin_locked(&dev_priv->irq_lock);
> @@ -139,11 +149,7 @@ ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
>  	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>  		return;
>  
> -	if ((dev_priv->irq_mask & mask) != 0) {
> -		dev_priv->irq_mask &= ~mask;
> -		I915_WRITE(DEIMR, dev_priv->irq_mask);
> -		POSTING_READ(DEIMR);
> -	}
> +	ironlake_enable_display_irq_nowarn(dev_priv, mask);
>  }
>  
>  static void
> @@ -3665,7 +3671,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  		 * setup is guaranteed to run in single-threaded context. But we
>  		 * need it to make the assert_spin_locked happy. */
>  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -		ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT);
> +		ironlake_enable_display_irq_nowarn(dev_priv, DE_PCU_EVENT);
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  	}
>  
> -- 
> 1.7.5.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jesse Barnes]

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.

> Also the commit message is a bit thin on the usual details like which
> commits introduced this regression, so that maintainers know where to
> apply this to.

I don't have the commit...  Oliver do you have it handy?

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Oliver Hartkopp]

On 26.08.2014 20:52, Jesse Barnes wrote:
> On Tue, 26 Aug 2014 09:23:54 +0200
> Daniel Vetter <daniel@ffwll.ch> 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.
>>>

>> Also the commit message is a bit thin on the usual details like which
>> commits introduced this regression, so that maintainers know where to
>> apply this to.
> 
> I don't have the commit...  Oliver do you have it handy?
> 

Hm - I really can not tell what has been done to introduce this regression.
I just saw the warning on my machine after upgrading to 3.17 ...

You can ask me about linux/net/can/ but not the drm stuff ;-)

Cheers,
Oliver

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

[Jesse Barnes]

On Tue, 26 Aug 2014 21:03:11 +0200
Oliver Hartkopp <socketcan@hartkopp.net> wrote:

> On 26.08.2014 20:52, Jesse Barnes wrote:
> > On Tue, 26 Aug 2014 09:23:54 +0200
> > Daniel Vetter <daniel@ffwll.ch> 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.
> >>>
> 
> >> Also the commit message is a bit thin on the usual details like which
> >> commits introduced this regression, so that maintainers know where to
> >> apply this to.
> > 
> > I don't have the commit...  Oliver do you have it handy?
> > 
> 
> Hm - I really can not tell what has been done to introduce this regression.
> I just saw the warning on my machine after upgrading to 3.17 ...
> 
> You can ask me about linux/net/can/ but not the drm stuff ;-)

I think it was this one Daniel, or the combination of patches around it:

commit 95f25beddba2ec9510b249740bacc11eca70cf75
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Jun 20 09:29:22 2014 -0700

    drm/i915: set pm._irqs_disabled at IRQ init time


-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Daniel Vetter]

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

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

[Jesse Barnes]

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.

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Daniel Vetter]

On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 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.
>
> 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.

So I think overall this is solid. And now I've also written the commit
message for the patch if it tests out on Oliver's box ;-)

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

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

[Oliver Hartkopp]

On 26.08.2014 22:51, Daniel Vetter wrote:
> 

> 
> Oliver, can you please test the below diff?
> 

> 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);
> 

Yes - that one works too! (see below)

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks,
Oliver


[    2.894351] Linux agpgart interface v0.103
[    2.894680] agpgart-intel 0000:00:00.0: Intel HD Graphics Chipset
[    2.894783] agpgart-intel 0000:00:00.0: detected gtt size: 2097152K total, 262144K mappable
[    2.895558] agpgart-intel 0000:00:00.0: detected 32768K stolen memory
[    2.895986] agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0xe0000000
[    2.896248] [drm] Initialized drm 1.1.0 20060810
[    2.898458] [drm] Memory usable by graphics device = 2048M
[    2.898541] [drm] Replacing VGA console driver
[    2.899207] Console: switching to colour dummy device 80x25
[    2.931603] i915 0000:00:02.0: irq 24 for MSI/MSI-X
[    2.931615] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[    2.931618] [drm] Driver supports precise vblank timestamp query.
[    2.931964] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem
[    3.023215] fbcon: inteldrmfb (fb0) is primary device
[    3.484082] tsc: Refined TSC clocksource calibration: 2792.999 MHz
[    4.170441] [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up
[    4.485295] Switched to clocksource tsc
[    4.533314] Console: switching to colour frame buffer device 240x67
[    4.550357] i915 0000:00:02.0: fb0: inteldrmfb frame buffer device
[    4.550446] i915 0000:00:02.0: registered panic notifier
[    4.561636] ACPI: Video Device [VID2] (multi-head: yes  rom: no  post: no)
[    4.913748] acpi device:45: registered as cooling_device0
[    4.941732] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:02/input/input4
[    4.941822] [drm] Initialized i915 1.6.0 20140725 for 0000:00:02.0 on minor 0

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

[Jesse Barnes]

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

Re: [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@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@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



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

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

[Jesse Barnes]

On Wed, 27 Aug 2014 23:33:05 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Aug 27, 2014 at 9:59 PM, Jesse Barnes <jbarnes@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.

Either is fine with me.  Maybe you could cc the PRTS and see if it
gives you results!

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Sun, Yi]

I have tested both Jesse and Daniel's patch.

On IVB both two will introduce a new warning.
On HSW Daniel's patch can cause black screen.
On BDW both two patch can cause black screen.

_________________________________________________________________________________________________________________
|	        |  Bad commit      |              Jesse's patch                             |   Dainel's patch             |
|IVB	        |  irq WARN on boot  | No irq warning, but a new one 'WARNING! power/level is deprecated; use power/control instead'   |
|HSW       |  irq WARN on boot  |              No irq warning                           |   black screen               |
|SNB        |  irq WARN on boot  |	          No irq warning                           |   No irq warning             |
|BDW       |  irq warn on boot   |              black screen                             |   black screen               |
|___________|___________________|____________________________________________________|____________________________|


Thanks
  --Sun, Yi

> -----Original Message-----
> From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org]
> Sent: Thursday, August 28, 2014 3:59 AM
> To: Daniel Vetter
> Cc: intel-gfx; Oliver Hartkopp; Sun, Yi
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/ilk: special case enabling of
> PCU_EVENT interrupt
> 
> 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