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

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

[Tina Zhang]

This patch-set introduces the GEM proxy which is a kind of GEM whose
backing physical memory is pinned and produced by guest VM and is
used by host as read only.

The patch is separated from the "Dma-buf support for Gvt-g"
  patch-set. (Joonas)

v2:
- return -ENXIO when pin and map pages of GEM proxy to kernel space.
  (Chris)
- update the patch description and subject to just mention objects w/o
  backing storage, instead of "GEM proxy". (Joonas)

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


Tina Zhang (2):
  drm/i915: Introduce GEM proxy
  drm/i915: Object w/o backing stroage is banned by -ENXIO

 drivers/gpu/drm/i915/i915_gem.c        | 27 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_object.h | 11 +++++++++--
 drivers/gpu/drm/i915/i915_gem_tiling.c |  8 ++++++++
 3 files changed, 42 insertions(+), 4 deletions(-)

-- 
2.7.4

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

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

[Tina Zhang]

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@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@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

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

[PATCH v2 2/2] drm/i915: Object w/o backing stroage is banned by -ENXIO

[Tina Zhang]

-ENXIO should be returned when operations are banned from changing
backing storage of objects without backing storage.

v2:
- update the patch description and subject to just mention objects w/o
  backing storage, instead of "GEM proxy". (Joonas)

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 13bc18d..e85721c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1713,7 +1713,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 	 */
 	if (!obj->base.filp) {
 		i915_gem_object_put(obj);
-		return -EINVAL;
+		return -ENXIO;
 	}
 
 	addr = vm_mmap(obj->base.filp, 0, args->size,
-- 
2.7.4

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

✗ Fi.CI.BAT: failure for drm/i915: Introduce GEM proxy

[Patchwork]

== Series Details ==

Series: drm/i915: Introduce GEM proxy
URL   : https://patchwork.freedesktop.org/series/32953/
State : failure

== Summary ==

Series 32953v1 drm/i915: Introduce GEM proxy
https://patchwork.freedesktop.org/api/1.0/series/32953/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-skl-6700k)
Test pm_backlight:
        Subgroup basic-brightness:
                incomplete -> PASS       (fi-glk-1) fdo#102035

fdo#102035 https://bugs.freedesktop.org/show_bug.cgi?id=102035

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:446s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:457s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:378s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:548s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:277s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:507s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:502s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:506s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:486s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:555s
fi-cnl-y         total:217  pass:196  dwarn:0   dfail:0   fail:0   skip:20 
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:421s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:260s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:589s
fi-glk-dsi       total:289  pass:258  dwarn:0   dfail:0   fail:1   skip:30  time:495s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:432s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:426s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:496s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:562s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:592s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:569s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:593s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:644s
fi-skl-6700k     total:230  pass:209  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:500s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:461s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:567s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s

ec9f75873be66e10cbcfa5ac1e32a79a82b6820c drm-tip: 2017y-10m-31d-23h-03m-56s UTC integration manifest
fa30681a8b83 drm/i915: Object w/o backing stroage is banned by -ENXIO
f213aaf4ccd0 drm/i915: Introduce GEM proxy

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6287/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Object w/o backing stroage is banned by -ENXIO

[Joonas Lahtinen]

Hi Tina,

Please send this patch alone (or in the beginning of your series), so it can be merged already.

That'll save you the effort of carrying this patch.

Regards, Joonas

On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> -ENXIO should be returned when operations are banned from changing
> backing storage of objects without backing storage.
> 
> v2:
> - update the patch description and subject to just mention objects w/o
>   backing storage, instead of "GEM proxy". (Joonas)
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 13bc18d..e85721c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1713,7 +1713,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>  	 */
>  	if (!obj->base.filp) {
>  		i915_gem_object_put(obj);
> -		return -EINVAL;
> +		return -ENXIO;
>  	}
>  
>  	addr = vm_mmap(obj->base.filp, 0, args->size,
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> 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@intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

<SNIP>

> @@ -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);

You should have marked this change in the changelog and then marked the
Reviewed-by tags to be valid only to the previous version of this
patch.

It's not a fair game to claim a patch to be "Reviewed-by" at the
current version, when you've made changes that were not agreed upon.

So that's some meta-review. Back to the actual review;

Which codepath was hitting the GEM_BUG_ON? Wondering if it would be
cleaner to avoid the call to this function on that single codepath.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Joonas Lahtinen
> Sent: Monday, November 6, 2017 7:24 PM
> To: Zhang, Tina <tina.zhang@intel.com>; zhenyuw@linux.intel.com; Wang, Zhi
> A <zhi.a.wang@intel.com>; daniel@ffwll.ch; chris@chris-wilson.co.uk
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> 
> On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> > 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@intel.com>
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> <SNIP>
> 
> > @@ -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);
> 
> You should have marked this change in the changelog and then marked the
> Reviewed-by tags to be valid only to the previous version of this patch.
> 
> It's not a fair game to claim a patch to be "Reviewed-by" at the current version,
> when you've made changes that were not agreed upon.
I thought we were agreed on this :)

> 
> So that's some meta-review. Back to the actual review;
> 
> Which codepath was hitting the GEM_BUG_ON? Wondering if it would be
> cleaner to avoid the call to this function on that single codepath.
Here is the previously comments:
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-October/002278.html
Thanks.

BR,
Tina
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Object w/o backing stroage is banned by -ENXIO

[Zhang, Tina]

> -----Original Message-----
> From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]
> Sent: Monday, November 6, 2017 6:57 PM
> To: Zhang, Tina <tina.zhang@intel.com>; zhenyuw@linux.intel.com; Wang, Zhi
> A <zhi.a.wang@intel.com>; daniel@ffwll.ch; chris@chris-wilson.co.uk
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH v2 2/2] drm/i915: Object w/o backing stroage is banned by
> -ENXIO
> 
> Hi Tina,
> 
> Please send this patch alone (or in the beginning of your series), so it can be
> merged already.
> 
> That'll save you the effort of carrying this patch.
Thank you very much :)

BR,
Tina
> 
> Regards, Joonas
> 
> On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> > -ENXIO should be returned when operations are banned from changing
> > backing storage of objects without backing storage.
> >
> > v2:
> > - update the patch description and subject to just mention objects w/o
> >   backing storage, instead of "GEM proxy". (Joonas)
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 13bc18d..e85721c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1713,7 +1713,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
> >  	 */
> >  	if (!obj->base.filp) {
> >  		i915_gem_object_put(obj);
> > -		return -EINVAL;
> > +		return -ENXIO;
> >  	}
> >
> >  	addr = vm_mmap(obj->base.filp, 0, args->size,
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On Tue, 2017-11-07 at 04:53 +0000, Zhang, Tina wrote:
> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> > Behalf Of Joonas Lahtinen
> > Sent: Monday, November 6, 2017 7:24 PM
> > To: Zhang, Tina <tina.zhang@intel.com>; zhenyuw@linux.intel.com; Wang, Zhi
> > A <zhi.a.wang@intel.com>; daniel@ffwll.ch; chris@chris-wilson.co.uk
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> > intel-gvt-dev@lists.freedesktop.org
> > Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> > 
> > On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> > > 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@intel.com>
> > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > <SNIP>
> > 
> > > @@ -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);
> > 
> > You should have marked this change in the changelog and then marked the
> > Reviewed-by tags to be valid only to the previous version of this patch.
> > 
> > It's not a fair game to claim a patch to be "Reviewed-by" at the current version,
> > when you've made changes that were not agreed upon.
> 
> I thought we were agreed on this :)
> 
> > 
> > So that's some meta-review. Back to the actual review;
> > 
> > Which codepath was hitting the GEM_BUG_ON? Wondering if it would be
> > cleaner to avoid the call to this function on that single codepath.
> 
> Here is the previously comments:
> https://lists.freedesktop.org/archives/intel-gvt-dev/2017-October/002278.html
> Thanks.

I never even noticed such an e-mail, so the correct response would've
been;

Reviewed-by: Joonas #vX
Reviewed-by: Chris

Where #vX is the version I actually agreed to.

Reviewed-by tags are are ones you need to be especially careful about
in addition to the Signed-off-bys because they carry special meaning:

https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html#r
eviewer-s-statement-of-oversight

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Joonas Lahtinen
> Sent: Tuesday, November 7, 2017 9:06 PM
> To: Zhang, Tina <tina.zhang@intel.com>; zhenyuw@linux.intel.com; Wang, Zhi
> A <zhi.a.wang@intel.com>; daniel@ffwll.ch; chris@chris-wilson.co.uk
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; intel-gfx@lists.freedesktop.org;
> intel-gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> 
> On Tue, 2017-11-07 at 04:53 +0000, Zhang, Tina wrote:
> > > -----Original Message-----
> > > From: intel-gvt-dev
> > > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of
> > > Joonas Lahtinen
> > > Sent: Monday, November 6, 2017 7:24 PM
> > > To: Zhang, Tina <tina.zhang@intel.com>; zhenyuw@linux.intel.com;
> > > Wang, Zhi A <zhi.a.wang@intel.com>; daniel@ffwll.ch;
> > > chris@chris-wilson.co.uk
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>;
> > > intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org
> > > Subject: Re: [PATCH v2 1/2] drm/i915: Introduce GEM proxy
> > >
> > > On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> > > > 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@intel.com>
> > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > <SNIP>
> > >
> > > > @@ -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);
> > >
> > > You should have marked this change in the changelog and then marked
> > > the Reviewed-by tags to be valid only to the previous version of this patch.
> > >
> > > It's not a fair game to claim a patch to be "Reviewed-by" at the
> > > current version, when you've made changes that were not agreed upon.
> >
> > I thought we were agreed on this :)
> >
> > >
> > > So that's some meta-review. Back to the actual review;
> > >
> > > Which codepath was hitting the GEM_BUG_ON? Wondering if it would be
> > > cleaner to avoid the call to this function on that single codepath.
> >
> > Here is the previously comments:
> > https://lists.freedesktop.org/archives/intel-gvt-dev/2017-October/0022
> > 78.html
> > Thanks.
> 
> I never even noticed such an e-mail, so the correct response would've been;
> 
> Reviewed-by: Joonas #vX
> Reviewed-by: Chris
> 
> Where #vX is the version I actually agreed to.
> 
> Reviewed-by tags are are ones you need to be especially careful about in
> addition to the Signed-off-bys because they carry special meaning:
> 
> https://www.kernel.org/doc/html/v4.13/process/submitting-patches.html#r
> eviewer-s-statement-of-oversight
Indeed, thank you :)

BR,
Tina
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@linux.intel.com; Wang, Zhi A <zhi.a.wang@intel.com>;
> daniel@ffwll.ch; joonas.lahtinen@linux.intel.com; chris@chris-wilson.co.uk
> Cc: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org; intel-
> gvt-dev@lists.freedesktop.org; Daniel Vetter <daniel.vetter@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@intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@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

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