Re: [PATCH 12/16] drm/i915: Reduce locking inside swfinish ioctl

Chris Wilson
2016-08-04T10:49:04Z

On Thu, Aug 04, 2016 at 01:32:52PM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> >   /* Pinned buffers may be scanout, so flush the cache */
> > - if (obj->pin_display)
> > + if (READ_ONCE(obj->pin_display)) {
> > + ret = i915_mutex_lock_interruptible(dev);
> > + if (ret)
> > + goto unref;
>
> See below.
>
> > +
> >   i915_gem_object_flush_cpu_write_domain(obj);
> >
> > - i915_gem_object_put(obj);
> > -unlock:
> > - mutex_unlock(&dev->struct_mutex);
> > + i915_gem_object_put(obj);
> > + mutex_unlock(&dev->struct_mutex);
> > + } else {
> > + ret = 0;
> > +unref:
>
> No, nope, nein, ei, njet, inte, nack; this shall not pass.
>
> Most inappropriate use of goto I've seen shortly.

It is correct though :-p

Jump forward a few patches, so I can write:

obj = i915_gem_object_lookup(file, args->handle);
if (!obj)
return -ENOENT;

/* Pinned buffers may be scanout, so flush the cache */
ret = 0;
if (READ_ONCE(obj->pin_display)) {
ret = i915_mutex_lock_interruptible(dev);
if (ret == 0) {
i915_gem_object_flush_cpu_write_domain(obj);
mutex_unlock(&dev->struct_mutex);
}
}

i915_gem_object_put(obj);
return ret;

Hmm, and the struct_mutex there is to protect the obj->base.write_domain.
Maybe cmpxchg...

Anyway, I don't care about swfinish that much, so the above with
put_unlocked for now.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre