[PATCH v2 1/2] drm/i915: Introduce GEM proxy

[Zhang, Tina]

Hi,

As the "Dma-buf support for GVT-g" patch-set relys on this patch, can I collect the comments for this version? Do we all agree on it?
Thanks.

Comments of the previous version:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-October/002278.html

BR,
Tina

> -----Original Message-----
> From: Zhang, Tina
> Sent: Wednesday, November 1, 2017 5:22 PM
> To: zhenyuw at linux.intel.com; Wang, Zhi A <zhi.a.wang at intel.com>;
> daniel at ffwll.ch; joonas.lahtinen at linux.intel.com; chris at chris-wilson.co.uk
> Cc: Zhang, Tina <tina.zhang at intel.com>; intel-gfx at lists.freedesktop.org; intel-
> gvt-dev at lists.freedesktop.org; Daniel Vetter <daniel.vetter at ffwll.ch>
> Subject: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> 
> GEM proxy is a kind of GEM, whose backing physical memory is pinned and
> produced by guest VM and is used by host as read only. With GEM proxy, host is
> able to access guest physical memory through GEM object interface. As GEM
> proxy is such a special kind of GEM, a new flag I915_GEM_OBJECT_IS_PROXY is
> introduced to ban host from changing the backing storage of GEM proxy.
> 
> v2:
> - return -ENXIO when pin and map pages of GEM proxy to kernel space.
>   (Chris)
> 
> Here are the histories of this patch in "Dma-buf support for Gvt-g"
> patch-set:
> 
> v14:
> - return -ENXIO when gem proxy object is banned by ioctl.
>   (Chris) (Daniel)
> 
> v13:
> - add comments to GEM proxy. (Chris)
> - don't ban GEM proxy in i915_gem_sw_finish_ioctl. (Chris)
> - check GEM proxy bar after finishing i915_gem_object_wait. (Chris)
> - remove GEM proxy bar in i915_gem_madvise_ioctl.
> 
> v6:
> - add gem proxy barrier in the following ioctls. (Chris)
>   i915_gem_set_caching_ioctl
>   i915_gem_set_domain_ioctl
>   i915_gem_sw_finish_ioctl
>   i915_gem_set_tiling_ioctl
>   i915_gem_madvise_ioctl
> 
> Signed-off-by: Tina Zhang <tina.zhang at intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c        | 25 ++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_object.h | 11 +++++++++--
> drivers/gpu/drm/i915/i915_gem_tiling.c |  8 ++++++++
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 82a1003..13bc18d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1593,6 +1593,16 @@ i915_gem_set_domain_ioctl(struct drm_device
> *dev, void *data,
>  	if (err)
>  		goto out;
> 
> +	/* 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.
> +	 */
> +	if (i915_gem_object_is_proxy(obj)) {
> +		err = -ENXIO;
> +		goto out;
> +	}
> +
>  	/* Flush and acquire obj->pages so that we are coherent through
>  	 * direct access in memory with previous cached writes through
>  	 * shmemfs and that our cache domain tracking remains valid.
> @@ -1649,6 +1659,10 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev,
> void *data,
>  	if (!obj)
>  		return -ENOENT;
> 
> +	/* Proxy objects are barred from CPU access, so there is no
> +	 * need to ban sw_finish as it is a nop.
> +	 */
> +
>  	/* Pinned buffers may be scanout, so flush the cache */
>  	i915_gem_object_flush_if_display(obj);
>  	i915_gem_object_put(obj);
> @@ -2614,7 +2628,8 @@ void *i915_gem_object_pin_map(struct
> drm_i915_gem_object *obj,
>  	void *ptr;
>  	int ret;
> 
> -	GEM_BUG_ON(!i915_gem_object_has_struct_page(obj));
> +	if (unlikely(!i915_gem_object_has_struct_page(obj)))
> +		return ERR_PTR(-ENODEV);
> 
>  	ret = mutex_lock_interruptible(&obj->mm.lock);
>  	if (ret)
> @@ -3766,6 +3781,14 @@ int i915_gem_set_caching_ioctl(struct drm_device
> *dev, void *data,
>  	if (!obj)
>  		return -ENOENT;
> 
> +	/* The caching mode of proxy object is handled by its generator, and
> not
> +	 * expected to be changed by user mode.
> +	 */
> +	if (i915_gem_object_is_proxy(obj)) {
> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
>  	if (obj->cache_level == level)
>  		goto out;
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h
> b/drivers/gpu/drm/i915/i915_gem_object.h
> index 956c911..78d65db 100644
> --- a/drivers/gpu/drm/i915/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -53,8 +53,9 @@ struct i915_lut_handle {
> 
>  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_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 @@ -
> 355,6 +356,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..f617012 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -345,6 +345,14 @@ i915_gem_set_tiling_ioctl(struct drm_device *dev,
> void *data,
>  	if (!obj)
>  		return -ENOENT;
> 
> +	/* The tiling mode of proxy objects is handled by its generator, and not
> +	 * expected to be changed by user mode.
> +	 */
> +	if (i915_gem_object_is_proxy(obj)) {
> +		err = -ENXIO;
> +		goto err;
> +	}
> +
>  	if (!i915_tiling_ok(obj, args->tiling_mode, args->stride)) {
>  		err = -EINVAL;
>  		goto err;
> --
> 2.7.4