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

[Yu Dai]

On 11/24/2015 11:13 AM, Daniel Vetter wrote:
> 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 at intel.com wrote:
> > >> > > > > From: Alex Dai <yu.dai at 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 at 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.
>

Currently there is only one guc client to do the commands submission. It 
appears we don't need the lock. When there are more clients and all 
write to the scratch registers or ring the door bell, we don't want them 
interact with each other. Also, if we implement guc to host interrupt 
(says to handle the log buffer full event), we do need to protect the 
guc client content. Well, none presents today. I can clean up these and 
test out.

Alex