[PATCH] drm/i915: Flush idle work when changing missed-irq fault injection

[PATCH] drm/i915: Flush idle work when changing missed-irq fault injection

[Chris Wilson]

In order for the missed-irq update to take effect, the device must be
idle. So when the user updates the fault injection via debugfs, idle the
device.

Testcase: igt/drv_missed_irq
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4a3e5b9552f8..9b769ee231e9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4151,16 +4151,35 @@ i915_ring_missed_irq_set(void *data, u64 val)
 {
 	struct drm_i915_private *dev_priv = data;
 	struct drm_device *dev = &dev_priv->drm;
-	int ret;
+	int err;
+
+	err = mutex_lock_interruptible(&dev->struct_mutex);
+	if (err)
+		return err;
+
+	err = i915_gem_wait_for_idle(dev_priv,
+				     I915_WAIT_LOCKED |
+				     I915_WAIT_INTERRUPTIBLE);
+	if (err)
+		goto err_unlock;
+
+	/* Retire to kick idle work */
+	i915_gem_retire_requests(dev_priv);
+	GEM_BUG_ON(dev_priv->gt.active_requests);
 
-	/* Lock against concurrent debugfs callers */
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
 	dev_priv->gpu_error.missed_irq_rings = val;
 	mutex_unlock(&dev->struct_mutex);
 
+	/* Flush idle worker to disarm irq */
+	while (flush_delayed_work(&dev_priv->gt.idle_work))
+		;
+
+
 	return 0;
+
+err_unlock:
+	mutex_unlock(&dev->struct_mutex);
+	return err;
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_ring_missed_irq_fops,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Flush idle work when changing missed-irq fault injection

[Chris Wilson]

In order for the missed-irq update to take effect, the device must be
idle. So when the user updates the fault injection via debugfs, idle the
device.

v2: Idle is explicitly required for setting test_irq, and good behaviour
for clearing the missed_irq.

Testcase: igt/drv_missed_irq
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4a3e5b9552f8..511d3541d3d5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4138,6 +4138,39 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
 			"%llu\n");
 
 static int
+fault_irq_set(struct drm_i915_private *i915, unsigned long *irq, u64 val)
+{
+	int err;
+
+	err = mutex_lock_interruptible(&i915->drm.struct_mutex);
+	if (err)
+		return err;
+
+	err = i915_gem_wait_for_idle(i915,
+				     I915_WAIT_LOCKED |
+				     I915_WAIT_INTERRUPTIBLE);
+	if (err)
+		goto err_unlock;
+
+	/* Retire to kick idle work */
+	i915_gem_retire_requests(i915);
+	GEM_BUG_ON(i915->gt.active_requests);
+
+	*irq = val & INTEL_INFO(i915)->ring_mask;
+	mutex_unlock(&i915->drm.struct_mutex);
+
+	/* Flush idle worker to disarm irq */
+	while (flush_delayed_work(&i915->gt.idle_work))
+		;
+
+	return 0;
+
+err_unlock:
+	mutex_unlock(&i915->drm.struct_mutex);
+	return err;
+}
+
+static int
 i915_ring_missed_irq_get(void *data, u64 *val)
 {
 	struct drm_i915_private *dev_priv = data;
@@ -4149,18 +4182,8 @@ i915_ring_missed_irq_get(void *data, u64 *val)
 static int
 i915_ring_missed_irq_set(void *data, u64 val)
 {
-	struct drm_i915_private *dev_priv = data;
-	struct drm_device *dev = &dev_priv->drm;
-	int ret;
-
-	/* Lock against concurrent debugfs callers */
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-	dev_priv->gpu_error.missed_irq_rings = val;
-	mutex_unlock(&dev->struct_mutex);
-
-	return 0;
+	struct drm_i915_private *i915 = data;
+	return fault_irq_set(i915, &i915->gpu_error.missed_irq_rings, val);
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_ring_missed_irq_fops,
@@ -4180,13 +4203,11 @@ i915_ring_test_irq_get(void *data, u64 *val)
 static int
 i915_ring_test_irq_set(void *data, u64 val)
 {
-	struct drm_i915_private *dev_priv = data;
+	struct drm_i915_private *i915 = data;
 
-	val &= INTEL_INFO(dev_priv)->ring_mask;
+	val &= INTEL_INFO(i915)->ring_mask;
 	DRM_DEBUG_DRIVER("Masking interrupts on rings 0x%08llx\n", val);
-	dev_priv->gpu_error.test_irq_rings = val;
-
-	return 0;
+	return fault_irq_set(i915, &i915->gpu_error.test_irq_rings, val);
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_ring_test_irq_fops,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Flush idle work when changing missed-irq fault injection (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Flush idle work when changing missed-irq fault injection (rev2)
URL   : https://patchwork.freedesktop.org/series/20752/
State : failure

== Summary ==

Series 20752v2 drm/i915: Flush idle work when changing missed-irq fault injection
https://patchwork.freedesktop.org/api/1.0/series/20752/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-FAIL (fi-ivb-3520m)
        Subgroup hang-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-ivb-3520m)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-bxt-j4205)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> SKIP       (fi-bxt-j4205)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> SKIP       (fi-bxt-j4205)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:256  dwarn:1   dfail:0   fail:0   skip:21 
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ivb-3520m     total:221  pass:206  dwarn:0   dfail:1   fail:0   skip:13 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

60c1d84d4f819f2c0a028dd776a6d64dc4528026 drm-tip: 2017y-03m-06d-12h-39m-38s UTC integration manifest
00d4023 drm/i915: Flush idle work when changing missed-irq fault injection

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4070/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Flush idle work when changing missed-irq fault injection (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Flush idle work when changing missed-irq fault injection (rev2)
URL   : https://patchwork.freedesktop.org/series/20752/
State : success

== Summary ==

Series 20752v2 drm/i915: Flush idle work when changing missed-irq fault injection
https://patchwork.freedesktop.org/api/1.0/series/20752/revisions/2/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo:
                dmesg-warn -> PASS       (fi-bsw-n3050)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 472s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 617s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 536s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 621s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 504s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 503s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 439s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 445s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 512s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 478s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 481s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 504s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 597s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 502s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 549s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 554s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 426s

e06000745435e65b4c056fe8f5bf149b298a0526 drm-tip: 2017y-03m-06d-14h-39m-38s UTC integration manifest
d022b825 drm/i915: Flush idle work when changing missed-irq fault injection

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4073/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Flush idle work when changing missed-irq fault injection

[Tvrtko Ursulin]

On 06/03/2017 10:15, Chris Wilson wrote:
> In order for the missed-irq update to take effect, the device must be
> idle. So when the user updates the fault injection via debugfs, idle the
> device.
>
> v2: Idle is explicitly required for setting test_irq, and good behaviour
> for clearing the missed_irq.
>
> Testcase: igt/drv_missed_irq
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 55 +++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4a3e5b9552f8..511d3541d3d5 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4138,6 +4138,39 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
>  			"%llu\n");
>
>  static int
> +fault_irq_set(struct drm_i915_private *i915, unsigned long *irq, u64 val)
> +{
> +	int err;
> +
> +	err = mutex_lock_interruptible(&i915->drm.struct_mutex);
> +	if (err)
> +		return err;
> +
> +	err = i915_gem_wait_for_idle(i915,
> +				     I915_WAIT_LOCKED |
> +				     I915_WAIT_INTERRUPTIBLE);
> +	if (err)
> +		goto err_unlock;
> +
> +	/* Retire to kick idle work */
> +	i915_gem_retire_requests(i915);
> +	GEM_BUG_ON(i915->gt.active_requests);
> +
> +	*irq = val & INTEL_INFO(i915)->ring_mask;

Looks like a type width mismatch on 32-bit.

Should we change missed_irq_rings to an u64?

> +	mutex_unlock(&i915->drm.struct_mutex);
> +
> +	/* Flush idle worker to disarm irq */
> +	while (flush_delayed_work(&i915->gt.idle_work))
> +		;

Worth sticking a schedule in here or something? Not worth it for debugfs 
I guess since we don't have it elsewhere.

> +
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&i915->drm.struct_mutex);
> +	return err;
> +}
> +
> +static int
>  i915_ring_missed_irq_get(void *data, u64 *val)
>  {
>  	struct drm_i915_private *dev_priv = data;
> @@ -4149,18 +4182,8 @@ i915_ring_missed_irq_get(void *data, u64 *val)
>  static int
>  i915_ring_missed_irq_set(void *data, u64 val)
>  {
> -	struct drm_i915_private *dev_priv = data;
> -	struct drm_device *dev = &dev_priv->drm;
> -	int ret;
> -
> -	/* Lock against concurrent debugfs callers */
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		return ret;
> -	dev_priv->gpu_error.missed_irq_rings = val;
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	return 0;
> +	struct drm_i915_private *i915 = data;
> +	return fault_irq_set(i915, &i915->gpu_error.missed_irq_rings, val);
>  }
>
>  DEFINE_SIMPLE_ATTRIBUTE(i915_ring_missed_irq_fops,
> @@ -4180,13 +4203,11 @@ i915_ring_test_irq_get(void *data, u64 *val)
>  static int
>  i915_ring_test_irq_set(void *data, u64 val)
>  {
> -	struct drm_i915_private *dev_priv = data;
> +	struct drm_i915_private *i915 = data;
>
> -	val &= INTEL_INFO(dev_priv)->ring_mask;
> +	val &= INTEL_INFO(i915)->ring_mask;
>  	DRM_DEBUG_DRIVER("Masking interrupts on rings 0x%08llx\n", val);
> -	dev_priv->gpu_error.test_irq_rings = val;
> -
> -	return 0;
> +	return fault_irq_set(i915, &i915->gpu_error.test_irq_rings, val);
>  }
>
>  DEFINE_SIMPLE_ATTRIBUTE(i915_ring_test_irq_fops,
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Flush idle work when changing missed-irq fault injection

[Chris Wilson]

On Mon, Mar 06, 2017 at 05:45:30PM +0000, Tvrtko Ursulin wrote:
> 
> On 06/03/2017 10:15, Chris Wilson wrote:
> > static int
> >+fault_irq_set(struct drm_i915_private *i915, unsigned long *irq, u64 val)
> >+{
> >+	int err;
> >+
> >+	err = mutex_lock_interruptible(&i915->drm.struct_mutex);
> >+	if (err)
> >+		return err;
> >+
> >+	err = i915_gem_wait_for_idle(i915,
> >+				     I915_WAIT_LOCKED |
> >+				     I915_WAIT_INTERRUPTIBLE);
> >+	if (err)
> >+		goto err_unlock;
> >+
> >+	/* Retire to kick idle work */
> >+	i915_gem_retire_requests(i915);
> >+	GEM_BUG_ON(i915->gt.active_requests);
> >+
> >+	*irq = val & INTEL_INFO(i915)->ring_mask;
> 
> Looks like a type width mismatch on 32-bit.
> 
> Should we change missed_irq_rings to an u64?

Currently unsigned long, and for convenience with test_bit() should
remain as an array of unsigned longs (i.e. bitmap). The mismatch here is
probably better served by s/u64 val/unsigned long val/. If we need to go
a full bitmap in the future, we'll have to switch to a bitmap_parse_str.
So unsigned long looks to be the future proof type.

> 
> >+	mutex_unlock(&i915->drm.struct_mutex);
> >+
> >+	/* Flush idle worker to disarm irq */
> >+	while (flush_delayed_work(&i915->gt.idle_work))
> >+		;
> 
> Worth sticking a schedule in here or something? Not worth it for
> debugfs I guess since we don't have it elsewhere.

flush_delayed_work() is itself a schedule (if active), underneath it
does a wait-for-completion on the work.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx