[PATCH 1/2] drm/i915: Fix irq_enabled checks in vlv display irq enable/disable

[PATCH 1/2] drm/i915: Fix irq_enabled checks in vlv display irq enable/disable

[Daniel Vetter]

In our suspend/resume and driver load code we enable power wells and
interrupts in different order than with runtime pm, which means code
needs to check for that and act accordingly. Unfortunately with the
SOiX support the "are interrupts enabled" checks went out of sync.

Fix up more places. This specific one was caught by the recently added
interrupt checks in pipestate_enable/disable functions.

This resulted in the following backtrace

Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Stopping disk
------------[ cut here ]------------
WARNING: CPU: 1 PID: 20572 at drivers/gpu/drm/i915/i915_irq.c:619 i915_disable_pipestat+0x94/0x122 [i915]()
Modules linked in: dm_mod iTCO_wdt iTCO_vendor_support snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep pcspkr snd_pcm serio_raw snd_timer i2c_i801 lpc_ich snd mfd_core soundcore iosf_mbi battery ac acpi_cpufreq i915 button video drm_kms_helper drm
CPU: 1 PID: 20572 Comm: kworker/u8:2 Tainted: G W 3.17.0-rc4_prts_a44085_20140912_debug+ #62
Workqueue: events_unbound async_run_entry_fn
0000000000000000 ffff88006f0dbb18 ffffffff81815c9c 0000000000000000
ffff88006f0dbb50 ffffffff8103e87f ffffffffa00b32e1 ffff880003690000
0000000000001c00 000000001c000000 00000000001f0024 ffff88006f0dbb60
Call Trace:
[] dump_stack+0x45/0x56
[] warn_slowpath_common+0x7f/0x98
[] ? i915_disable_pipestat+0x94/0x122 [i915]
[] warn_slowpath_null+0x1a/0x1c
[] i915_disable_pipestat+0x94/0x122 [i915]
[] valleyview_display_irqs_uninstall+0x99/0x101 [i915]
[] valleyview_disable_display_irqs+0x37/0x39 [i915]
[] vlv_display_power_well_disable+0x53/0x79 [i915]
[] intel_display_power_put+0xe3/0x111 [i915]
[] intel_display_set_init_power+0x31/0x3d [i915]
[] i915_drm_freeze+0x1c0/0x1cd [i915]
[] i915_pm_suspend+0x44/0x46 [i915]
[] pci_pm_suspend+0x85/0x106
[] ? pci_pm_freeze+0xb1/0xb1
[] dpm_run_callback+0x43/0xd3
[] __device_suspend+0x1e3/0x263
[] async_suspend+0x1f/0x8a
[] async_run_entry_fn+0x61/0x10b
[] process_one_work+0x221/0x3f2
[] ? process_one_work+0x1ac/0x3f2
[] worker_thread+0x288/0x37c
[] ? cancel_delayed_work_sync+0x15/0x15
[] kthread+0xf6/0xfe
[] ? kthread_create_on_node+0x19a/0x19a
[] ret_from_fork+0x7c/0xb0
[] ? kthread_create_on_node+0x19a/0x19a
---[ end trace f0b4cc6544a9afea ]---

Cc: Imre Deak <imre.deak@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7db0029558a4..20a5d6150919 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3723,7 +3723,7 @@ void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv)
 
 	dev_priv->display_irqs_enabled = true;
 
-	if (dev_priv->dev->irq_enabled)
+	if (dev_priv->pm._irqs_disabled)
 		valleyview_display_irqs_install(dev_priv);
 }
 
@@ -3736,7 +3736,7 @@ void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv)
 
 	dev_priv->display_irqs_enabled = false;
 
-	if (dev_priv->dev->irq_enabled)
+	if (dev_priv->pm._irqs_disabled)
 		valleyview_display_irqs_uninstall(dev_priv);
 }
 
-- 
2.0.1

[PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Daniel Vetter]

Yet another place that wasn't properly transformed when implementing
SOix. While at it convert the checks to WARN_ON on gen5+ (since we
don't have UMS potentially doing stupid things on those platforms).
And also add the corresponding checks to the put functions (again with
a WARN_ON) for gen5+.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |  4 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 23 ++++++++++++++---------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index bafd38b5703e..5fb61d76fd60 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1063,7 +1063,7 @@ static bool gen8_logical_ring_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
@@ -1082,6 +1082,8 @@ static void gen8_logical_ring_put_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
+	WARN_ON(!intel_irqs_enabled(dev_priv));
+
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
 		I915_WRITE_IMR(ring, ~ring->irq_keep_mask);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 264d2827b2fb..b568c1f2c57b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1197,7 +1197,7 @@ gen5_ring_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
@@ -1215,6 +1215,8 @@ gen5_ring_put_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
+	WARN_ON(!intel_irqs_enabled(dev_priv));
+
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0)
 		gen5_disable_gt_irq(dev_priv, ring->irq_enable_mask);
@@ -1228,7 +1230,7 @@ i9xx_ring_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
+	if (!intel_irqs_enabled(dev_priv))
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
@@ -1265,7 +1267,7 @@ i8xx_ring_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
+	if (!intel_irqs_enabled(dev_priv))
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
@@ -1399,8 +1401,8 @@ gen6_ring_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
-	       return false;
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
+		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount++ == 0) {
@@ -1424,6 +1426,8 @@ gen6_ring_put_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
+	WARN_ON(!intel_irqs_enabled(dev_priv));
+
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
 		if (HAS_L3_DPF(dev) && ring->id == RCS)
@@ -1442,7 +1446,7 @@ hsw_vebox_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
@@ -1462,8 +1466,7 @@ hsw_vebox_put_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
-		return;
+	WARN_ON(!intel_irqs_enabled(dev_priv));
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
@@ -1480,7 +1483,7 @@ gen8_ring_get_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
-	if (!dev->irq_enabled)
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
@@ -1506,6 +1509,8 @@ gen8_ring_put_irq(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long flags;
 
+	WARN_ON(!intel_irqs_enabled(dev_priv));
+
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
 		if (HAS_L3_DPF(dev) && ring->id == RCS) {
-- 
2.0.1

Re: [PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Chris Wilson]

On Mon, Sep 15, 2014 at 11:41:19AM +0200, Daniel Vetter wrote:
> Yet another place that wasn't properly transformed when implementing
> SOix. While at it convert the checks to WARN_ON on gen5+ (since we
> don't have UMS potentially doing stupid things on those platforms).
> And also add the corresponding checks to the put functions (again with
> a WARN_ON) for gen5+.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c        |  4 +++-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 23 ++++++++++++++---------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index bafd38b5703e..5fb61d76fd60 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1063,7 +1063,7 @@ static bool gen8_logical_ring_get_irq(struct intel_engine_cs *ring)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long flags;
>  
> -	if (!dev->irq_enabled)
> +	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>  		return false;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> @@ -1082,6 +1082,8 @@ static void gen8_logical_ring_put_irq(struct intel_engine_cs *ring)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long flags;
>  
> +	WARN_ON(!intel_irqs_enabled(dev_priv));

Please no. This would be a BUG().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Daniel Vetter]

On Mon, Sep 15, 2014 at 11:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> intel_engine_cs *ring)
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       unsigned long flags;
>>
>> +     WARN_ON(!intel_irqs_enabled(dev_priv));
>
> Please no. This would be a BUG().

No BUG if not doing it means the driver will survive for a bit longer.
And doing a few bogus register writes usually means it'll surive.

Similar checks I've added just recently to pipestate_enable/disable
caught a bug in the resume code. Using a BUG instead of WARN would
have meant some serious debugging, but as-is a look at the backtrace
was all that was needed to analyze the bug.

I know a lot of developers disagree, but debugging random crap in the
field is _so_ much easier with WARN than BUG that it's not even up for
discussion imo.

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

Re: [PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Chris Wilson]

On Mon, Sep 15, 2014 at 11:51:42AM +0200, Daniel Vetter wrote:
> On Mon, Sep 15, 2014 at 11:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > intel_engine_cs *ring)
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>       unsigned long flags;
> >>
> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >
> > Please no. This would be a BUG().
> 
> No BUG if not doing it means the driver will survive for a bit longer.
> And doing a few bogus register writes usually means it'll surive.

I mean that this offers no additional benefit over the first WARN. Which
is already redundant as we WARN in the caller. There are more meaningful
places where that WARN would be appropriate, but after the event of
something else screwing up is usually close to useless.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Daniel Vetter]

On Mon, Sep 15, 2014 at 11:56 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Sep 15, 2014 at 11:51:42AM +0200, Daniel Vetter wrote:
>> On Mon, Sep 15, 2014 at 11:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > intel_engine_cs *ring)
>> >>       struct drm_i915_private *dev_priv = dev->dev_private;
>> >>       unsigned long flags;
>> >>
>> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
>> >
>> > Please no. This would be a BUG().
>>
>> No BUG if not doing it means the driver will survive for a bit longer.
>> And doing a few bogus register writes usually means it'll surive.
>
> I mean that this offers no additional benefit over the first WARN. Which
> is already redundant as we WARN in the caller. There are more meaningful
> places where that WARN would be appropriate, but after the event of
> something else screwing up is usually close to useless.

If we drop the runtime pm reference before the put_irq then we'll WARN
here. Which is somewhat likely if some waiter doesn't have it's own
runtime pm reference but relies upon someone else holding that for
him. Then the get_irq will likely happen while the gpu is still busy,
but the put_irq has a good chance to only happen once we've cleaned
up.

Of course it might mean that we get 2 backtraces in some case if e.g.
a wait happens somehow before anything is really set up (in driver
load). But I think the additional coverage makes this worth it.

If it's too annoying we can always back it out again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Chris Wilson]

On Mon, Sep 15, 2014 at 12:08:39PM +0200, Daniel Vetter wrote:
> On Mon, Sep 15, 2014 at 11:56 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Sep 15, 2014 at 11:51:42AM +0200, Daniel Vetter wrote:
> >> On Mon, Sep 15, 2014 at 11:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > intel_engine_cs *ring)
> >> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>       unsigned long flags;
> >> >>
> >> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >> >
> >> > Please no. This would be a BUG().
> >>
> >> No BUG if not doing it means the driver will survive for a bit longer.
> >> And doing a few bogus register writes usually means it'll surive.
> >
> > I mean that this offers no additional benefit over the first WARN. Which
> > is already redundant as we WARN in the caller. There are more meaningful
> > places where that WARN would be appropriate, but after the event of
> > something else screwing up is usually close to useless.
> 
> If we drop the runtime pm reference before the put_irq then we'll WARN
> here. Which is somewhat likely if some waiter doesn't have it's own
> runtime pm reference but relies upon someone else holding that for
> him. Then the get_irq will likely happen while the gpu is still busy,
> but the put_irq has a good chance to only happen once we've cleaned
> up.

Put the warn before you sleep awaiting the irq. Every other condition is
besides the point. You can't detect some other thread breaking in whilst
you are asleep and when you are awake the request is either complete or
it is not. Reading any registers with the rpm is itself going to
generate the WARN so adding yet another WARN afterwards is simply
confusing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre