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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
