Re: [PATCH v12 6/6] drm/i915: Introduce GEM proxy

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tina Zhang <tina.zhang@intel.com>,
	intel-gfx@lists.freedesktop.org,
	intel-gvt-dev@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, ville.syrjala@linux.intel.com,
	zhenyuw@linux.intel.com, zhiyuan.lv@intel.com,
	zhi.a.wang@intel.com, alex.williamson@redhat.com,
	kraxel@redhat.com, daniel@ffwll.ch, kwankhede@nvidia.com,
	kevin.tian@intel.com
Subject: Re: [PATCH v12 6/6] drm/i915: Introduce GEM proxy
Date: Wed, 19 Jul 2017 12:20:22 +0100	[thread overview]
Message-ID: <150046322223.30670.8297746352161543572@mail.alporthouse.com> (raw)
In-Reply-To: <1500461945-16801-7-git-send-email-tina.zhang@intel.com>

s/GEM proxy/a GEM proxy object/

Quoting Tina Zhang (2017-07-19 11:59:05)

Rationale goes here.

> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c        | 26 +++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_object.h |  9 +++++++++
>  drivers/gpu/drm/i915/i915_gem_tiling.c |  5 +++++
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1b2dfa8..ebacc04 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1612,6 +1612,12 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>         if (!obj)
>                 return -ENOENT;
>  
> +       /* proxy gem object does not support setting domain */

Yes, this is what the code is doing. The comment tells us why.

/*
 * Proxy objects do not control access to the backing storage, ergo
 * they cannot be used as a means to manipulate the cache domain
 * tracking for that backing storage. The proxy object is always
 * considered to be outside of any cache domain.
 */

However, set-domain does have some other side-effects that includes
waiting which should still be performed, i.e. this check should be after
the lockless wait.

> +       if (i915_gem_object_is_proxy(obj)) {
> +               err = -EPERM;
> +               goto out;
> +       }
> +
>         /* 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.
> @@ -1680,6 +1686,12 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>         if (!obj)
>                 return -ENOENT;
>  

A comment could explain that since proxy objects are barred from CPU
access, we do not need to ban sw_finish as it is a nop.

> +       /* proxy gem obj does not support this operation */
> +       if (i915_gem_object_is_proxy(obj)) {
> +               i915_gem_object_put(obj);
> +               return -EPERM;
> +       }
> +
>         /* Pinned buffers may be scanout, so flush the cache */
>         i915_gem_object_flush_if_display(obj);
>         i915_gem_object_put(obj);
> @@ -1730,7 +1742,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>          */
>         if (!obj->base.filp) {
>                 i915_gem_object_put(obj);
> -               return -EINVAL;
> +               return -EPERM;
>         }
>  
>         addr = vm_mmap(obj->base.filp, 0, args->size,
> @@ -3764,6 +3776,12 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
>         if (!obj)
>                 return -ENOENT;
>  
> +       /* proxy gem obj does not support setting caching mode */

More rationale. Also is the proxied object (its target) also banned from
changing the PTE bits or do we inherit all such changes automatically?

> +       if (i915_gem_object_is_proxy(obj)) {
> +               ret = -EPERM;
> +               goto out;
> +       }
> +
>         if (obj->cache_level == level)
>                 goto out;
>  
> @@ -4210,6 +4228,12 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>         if (!obj)
>                 return -ENOENT;
>  
> +       /* proxy gem obj does not support changing backding storage */

This one could be generalised to I915_GEM_OBJECT_IS_SHRINKABLE?

> +       if (i915_gem_object_is_proxy(obj)) {
> +               err = -EPERM;
> +               goto out;
> +       }
> +
>         err = mutex_lock_interruptible(&obj->mm.lock);
>         if (err)
>                 goto out;
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> index 5b19a49..c91e336 100644
> --- a/drivers/gpu/drm/i915/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops {
>         unsigned int flags;
>  #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0)
>  #define I915_GEM_OBJECT_IS_SHRINKABLE   BIT(1)
> +#define I915_GEM_OBJECT_IS_PROXY       BIT(2)
>  
>         /* Interface between the GEM object and its backing storage.
>          * get_pages() is called once prior to the use of the associated set
> @@ -198,6 +199,8 @@ struct drm_i915_gem_object {
>                 } userptr;
>  
>                 unsigned long scratch;
> +
> +               void *gvt_info;

Unrelated chunk, this should go into the gvt patch to use the object.

>         };
>  
>         /** for phys allocated objects */
> @@ -300,6 +303,12 @@ i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
>  }
>  
>  static inline bool
> +i915_gem_object_is_proxy(const struct drm_i915_gem_object *obj)
> +{
> +       return obj->ops->flags & I915_GEM_OBJECT_IS_PROXY;
> +}
> +
> +static inline bool
>  i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
>  {
>         return obj->active_count;
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index fb5231f..21ec066 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -345,6 +345,11 @@ i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
>         if (!obj)
>                 return -ENOENT;
>  
> +       if (i915_gem_object_is_proxy(obj)) {
> +               err = -EPERM;
> +               goto err;
> +       }

Changing the tiling mode may well be justifiable, even for a proxy since
the tiling is local to the view. The ban on GVT behalf should be done via
obj->framebuffer_references, and a good rationale given here on why the
proxy should be banned.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx