[Intel-gfx] [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl

[Chris Wilson]

Quoting Ville Syrjälä (2019-03-22 14:28:37)
> On Thu, Mar 21, 2019 at 04:19:08PM +0000, Chris Wilson wrote:
> > If we are already in the desired write domain of a set-domain ioctl,
> > then there is nothing for us to do and we can quickly return back to
> > userspace, avoiding any lock contention. By recognising that the
> > write_domain is always a subset of the read_domains, and excluding the
> > no-op case of requiring 0 read_domains in the ioctl, we can infer if the
> > current write_domain matches the target read_domains, there is nothing
> > for us to do.
> > 
> > Secondary aspect of this is that we undo the arbitrary fetching and
> > potential flushing of all pages for a set-domain(.write=CPU) call on a
> > fresh object -- which was introduced simply because we do the get-pages
> > before taking the struct_mutex.
> > 
> > References: 40e62d5d6be8 ("drm/i915: Acquire the backing storage outside of struct_mutex in set-domain")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld at gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 26 +++++++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 72374e952e4b..36f557002005 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1484,17 +1484,37 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> >       if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> >               return -EINVAL;
> >  
> > -     /* Having something in the write domain implies it's in the read
> > +     /*
> > +      * Having something in the write domain implies it's in the read
> >        * domain, and only that read domain.  Enforce that in the request.
> >        */
> > -     if (write_domain != 0 && read_domains != write_domain)
> > +     if (write_domain && read_domains != write_domain)
> >               return -EINVAL;
> >  
> > +     if (!read_domains)
> > +             return 0;
> 
> Hopefully no one is relying on read_domains==0 meaning cpu domain.
> That seems to be how this was handled before.

Hopefully not. None of the userspace has tried that, and I hope that the
idea of write_domain==0 meaning don't set a write_domain has conditioned
everyone into not using it.

> Or maybe we want -EIVNAL here?

Introducing new -EINVAL is also risky.

Hmm. So in case of trouble we should

	if (!read_domains)
		read_domain = DOMAIN_CPU.

Hopefully no one even notices such a subtle ABI change. Bugzilla watch
out!
-Chris