Re: [PATCH] drm/i915/guc: Move wait for GuC out of spinlock/unlock

From: Daniel Vetter <daniel@ffwll.ch>
To: Yu Dai <yu.dai@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/guc: Move wait for GuC out of spinlock/unlock
Date: Tue, 24 Nov 2015 20:13:56 +0100	[thread overview]
Message-ID: <20151124191356.GG17050@phenom.ffwll.local> (raw)
In-Reply-To: <5654AE46.1010903@intel.com>

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@intel.com wrote:
> >> > > > > From: Alex Dai <yu.dai@intel.com>
> >> > > > >
> >> > > > > 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@intel.com>
> >> > > > > ---
> >> > > > >  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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx