[Intel-gfx] [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