Re: [PATCH v2 12/22] drm/i915: Replace wait-on-mutex with wait-on-bit in reset worker

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, Sep 08, 2016 at 01:52:21PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Since we have a cooperative mode now with a direct reset, we can avoid
> > the contention on struct_mutex and instead try then sleep on the
> > I915_RESET_IN_PROGRESS bit. If the mutex is held and that bit is
> > cleared, all is fine. Otherwise, we sleep for a bit and try again. In
> > the worst case we sleep for an extra second waiting for the mutex to be
> > released (no one touching the GPU is allowed the struct_mutex whilst the
> > I915_RESET_IN_PROGRESS bit is set). But when we have a direct reset,
> > this allows us to clean up the reset worker faster.
> >
> > v2: Remember to call wake_up_bit() after changing (for the faster wakeup
> > as promised)
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |  6 ++++--
> >  drivers/gpu/drm/i915/i915_irq.c | 30 ++++++++++++++++++------------
> >  2 files changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ff4173e6e298..c1b890dbd6cc 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1798,11 +1798,13 @@ int i915_reset(struct drm_i915_private *dev_priv)
> >  	intel_sanitize_gt_powersave(dev_priv);
> >  	intel_autoenable_gt_powersave(dev_priv);
> >  
> > -	return 0;
> > +out:
> > +	wake_up_bit(&error->flags, I915_RESET_IN_PROGRESS);
> > +	return ret;
> >  
> >  error:
> >  	set_bit(I915_WEDGED, &error->flags);
> > -	return ret;
> > +	goto out;
> 
> After initial surprice, and surprices are negative,
> this is short, keeps the error stuff out from bulkcode
> and keeps wakeup neatly in both paths.
> 
> To reduce the surprice effect, label could be wakeup_out.

I had to balance out what I'm used to as well. It's not common that we
do an error path jumping back to the normal path.

Some alternatives also used are:

	if (foo)
		goto err;

	goto out;

error:
	undo_foo();
out:
	bar();
	return ret;

but my preference is to keep the error paths out-of-line of the normal
control flow.

Alternatively... we could just move the wake_up_bit earlier and then
have the worker spin on the mutex for longer... Except it doesn't, does
it. The worker will be woken on the bit and exit. Oops, we can make that
simpler.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx