On Tue, Nov 24, 2015 at 10:36:54AM -0800, Yu Dai wrote: > > > On 11/24/2015 10:08 AM, Daniel Vetter wrote: > >On Tue, Nov 24, 2015 at 07:05:47PM +0200, Imre Deak wrote: > >> On ti, 2015-11-24 at 09:00 -0800, Yu Dai wrote: > >> > > >> > On 11/24/2015 05:26 AM, Imre Deak wrote: > >> > > On ti, 2015-11-24 at 14:04 +0100, Daniel Vetter wrote: > >> > > > On Mon, Nov 23, 2015 at 03:02:58PM -0800, yu.dai@xxxxxxxxx wrote: > >> > > > > From: Alex Dai <yu.dai@xxxxxxxxx> > >> > > > > > >> > > > > When GuC Work Queue is full, driver will wait GuC for avaliable > >> > > > > space by delaying 1ms. The wait needs to be out of spinlockirq > >> > > > > / > >> > > > > unlock. Otherwise, lockup happens because jiffies won't be > >> > > > > updated > >> > > > > dur to irq is disabled. > >> > > > > > >> > > > > Issue is found in igt/gem_close_race. > >> > > > > > >> > > > > Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx> > >> > > > > --- > >> > > > > drivers/gpu/drm/i915/i915_guc_submission.c | 27 > >> > > > > +++++++++++++++++- > >> > > > > --------- > >> > > > > 1 file changed, 17 insertions(+), 10 deletions(-) > >> > > > > > >> > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > >> > > > > b/drivers/gpu/drm/i915/i915_guc_submission.c > >> > > > > index 0a6b007..1418397 100644 > >> > > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > >> > > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > >> > > > > @@ -201,10 +201,13 @@ static int guc_ring_doorbell(struct > >> > > > > i915_guc_client *gc) > >> > > > > union guc_doorbell_qw *db; > >> > > > > void *base; > >> > > > > int attempt = 2, ret = -EAGAIN; > >> > > > > + unsigned long flags; > >> > > > > > >> > > > > base = kmap_atomic(i915_gem_object_get_page(gc- > >> > > > > > client_obj, 0)); > >> > > > > >> > > > We don't need kmap_atomic anymore here now, since it's outside of > >> > > > the > >> > > > spinlock. > >> > > > > >> > > > > desc = base + gc->proc_desc_offset; > >> > > > > > >> > > > > + spin_lock_irqsave(&gc->wq_lock, flags); > >> > > > > >> > > > Please don't use the super-generic _irqsave. It's expensive and > >> > > > results in > >> > > > fragile code when someone accidentally reuses something in an > >> > > > interrupt > >> > > > handler that was never meant to run in that context. > >> > > > > >> > > > Instead please use the most specific funtion: > >> > > > - spin_lock if you know you are in irq context. > >> > > > - sipn_lock_irq if you know you are not. > >> > > > >> > > Right, and simply spin_lock() if the lock is not taken in IRQ > >> > > context > >> > > ever. > >> > > >> > This is not in IRQ context. So I will use spin_lock_irq instead. > >> > >> You can just use spin_lock(). spin_lock_irq() makes only sense if you > >> take the lock in IRQ context too, which is not the case. > > > >Imo just drop both spinlocks, adding locks for debugfs is overkill imo. > > > How about using mutex_lock_interruptible(&dev->struct_mutex) instead in > debugfs, which is to replace host2guc lock. Yes. > spinlock during ring the door bell is still needed. Where/why is that needed? At least on a quick look I didn't notice anything. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
