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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@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.

Or maybe we want -EIVNAL here?

> +
>  	obj = i915_gem_object_lookup(file, args->handle);
>  	if (!obj)
>  		return -ENOENT;
>  
> -	/* Try to flush the object off the GPU without holding the lock.
> +	/*
> +	 * Already in the desired target write domain? Nothing for us to!
> +	 *
> +	 * We apply a little bit of cunning here to catch a broader set of
> +	 * no-ops. If obj->write_domain is set, we must be in the same
> +	 * obj->read_domains, and only that domain. Therefore, if that
> +	 * obj->write_domain matches the request read_domains, we are
> +	 * already in the same read/write domain and can skip the operation,
> +	 * without having to further check the requested write_domain.
> +	 */
> +	if (READ_ONCE(obj->write_domain) == read_domains) {
> +		err = 0;
> +		goto out;
> +	}

Hard to argue with that logic. 

Haven't paid too much attention to this area lately but this
makes sense to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	/*
> +	 * Try to flush the object off the GPU without holding the lock.
>  	 * We will repeat the flush holding the lock in the normal manner
>  	 * to catch cases where we are gazumped.
>  	 */
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx