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

[PATCH v1 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.

V1:
- the patch is separated from the "Dma-buf support for Gvt-g"
  patch-set. (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: Changing GEM proxy backing storage is banned with ENXIO

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

-- 
2.7.4

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

[PATCH v1 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.

V1:
- the patch is separated from the "Dma-buf support for Gvt-g"
  patch-set. (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

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c        | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_object.h | 11 +++++++++--
 drivers/gpu/drm/i915/i915_gem_tiling.c |  8 ++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 82a1003..7d3116a 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);
@@ -3766,6 +3780,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 v1 2/2] drm/i915: Changing GEM proxy backing storage is banned with ENXIO

[Tina Zhang]

GEM proxy is a kind of GEM object whose backing storage cannot be changed
by host i915 driver. -ENXIO should be returned when operations are banned
from changing backing storage of this kind of GEM object.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

---
 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 7d3116a..8f61c7f 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

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

[Chris Wilson]

Quoting Tina Zhang (2017-10-16 09:57:33)
> 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.
> 
> V1:
> - the patch is separated from the "Dma-buf support for Gvt-g"
>   patch-set. (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
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c        | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_object.h | 11 +++++++++--
>  drivers/gpu/drm/i915/i915_gem_tiling.c |  8 ++++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 82a1003..7d3116a 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.
> +        */
> +

Then we probably want a GEM_BUG_ON(i915_gem_object_is_proxy()) inside
pin_to_display and for this path, flush_if_display.

Otherwise, looking very nice. The comments are very much appreciated and
useful. I just need to go over the list of ioctls and check you have
everything.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 2/2] drm/i915: Changing GEM proxy backing storage is banned with ENXIO

[Chris Wilson]

Quoting Tina Zhang (2017-10-16 09:57:34)
> GEM proxy is a kind of GEM object whose backing storage cannot be changed
> by host i915 driver. -ENXIO should be returned when operations are banned
> from changing backing storage of this kind of GEM object.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Introduce GEM proxy

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900) fdo#101705

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:459s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:472s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:389s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:563s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:287s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:527s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:533s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:541s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:532s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:578s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:441s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:277s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:606s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:442s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:463s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:499s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:594s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:659s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:669s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:534s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:513s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:471s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:596s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:434s

b00e3edd1580e0e8aaaad648aa0e46c568b16801 drm-tip: 2017y-10m-16d-08h-16m-26s UTC integration manifest
2c8c99e3b6bb drm/i915: Changing GEM proxy backing storage is banned with ENXIO
84700df11b39 drm/i915: Introduce GEM proxy

== Logs ==

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

Re: [PATCH v1 2/2] drm/i915: Changing GEM proxy backing storage is banned with ENXIO

[Joonas Lahtinen]

"-v1" option when sending patch series, is not needed. First revision
is assumed by reader.

On Mon, 2017-10-16 at 16:57 +0800, Tina Zhang wrote:
> GEM proxy is a kind of GEM object whose backing storage cannot be changed
> by host i915 driver. -ENXIO should be returned when operations are banned
> from changing backing storage of this kind of GEM object.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>

Please update the patch description and subject to just mention objects
without backing storage eg. userptr and not "GEM proxy". By this patch,
we've not introduced the concept of GEM proxy yet.

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

✓ Fi.CI.IGT: success for drm/i915: Introduce GEM proxy

[Patchwork]

== Series Details ==

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

== Summary ==

Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

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

shard-hsw        total:2553 pass:1442 dwarn:0   dfail:0   fail:8   skip:1103 time:9640s

== Logs ==

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

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

[Chris Wilson]

Quoting Tina Zhang (2017-10-16 09:57:33)
> 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.
> 
> V1:
> - the patch is separated from the "Dma-buf support for Gvt-g"
>   patch-set. (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

gem_busy: sees the local object, not the proxy activity; that's ok
get_caching: returns the constant value
pread: works by forcing GTT access
pwrite: works by forcing GTT access
mmap: disallowed by !filp, errno fixup in next patch
mmap_gtt: works
set-domain: fixed
sw-finish: noted as impossible (please add checks)
set-tiling: fixed
get_tiling: returns constant value
madvise: works, since it's a discard of the local page array
overlay-put-image? not supported on any relevant platforms!
gem_wait: works on local bo (as expected)

execbuf: works (or should at least work fine on a proxy object as either
a batch or one of the auxiliaries)

Hmm, use as a batch + cmdparser is broken; should result in ENODEV to
userspace. An admittedly odd result, but not an oops.

display: pin-to-display and fencing will work, I think, and
flush_for_display is irrelevant as no direct cpu access.

dmabuf: works partially, but what doesn't work is optional. Userspace
mmap is barred, most inter-driver interaction is barred. On paper a
nuisance should it want to be used, but we should not oops. Hmm, no,
we need to make the GEM_BUG_ON(!struct_page(obj)) in
i915_gem_object_pin_map() a real return.

Please add

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d699ea3ab80b..6a7ca9a502ba 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2650,7 +2650,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)

and my
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Chris Wilson (2017-10-17 15:37:16)
> execbuf: works (or should at least work fine on a proxy object as either
> a batch or one of the auxiliaries)
> 
> Hmm, use as a batch + cmdparser is broken; should result in ENODEV to
> userspace. An admittedly odd result, but not an oops.

Which actually means I've just found a bunch more errno to change to
-ENXIO.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Tina]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Tuesday, October 17, 2017 10:37 PM
> To: Zhang, Tina <tina.zhang@intel.com>; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; daniel@ffwll.ch
> 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: Re: [PATCH v1 1/2] drm/i915: Introduce GEM proxy
> 
> Quoting Tina Zhang (2017-10-16 09:57:33)
> > 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.
> >
> > V1:
> > - the patch is separated from the "Dma-buf support for Gvt-g"
> >   patch-set. (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
> 
> gem_busy: sees the local object, not the proxy activity; that's ok
> get_caching: returns the constant value
> pread: works by forcing GTT access
> pwrite: works by forcing GTT access
> mmap: disallowed by !filp, errno fixup in next patch
> mmap_gtt: works
> set-domain: fixed
> sw-finish: noted as impossible (please add checks)
I thought we were agreed on this: https://lists.freedesktop.org/archives/intel-gvt-dev/2017-July/001523.html
So, in this version I just add the comments without checking

> set-tiling: fixed
> get_tiling: returns constant value
> madvise: works, since it's a discard of the local page array overlay-put-image?
No. The original idea is that the GEM proxy could support madvise as it only invokes unpin/pin. And supporting madivse might benefit userspace.
So, do you think we'd better ban GEM proxy in madvise?

> not supported on any relevant platforms!
> gem_wait: works on local bo (as expected)
> 
> execbuf: works (or should at least work fine on a proxy object as either a batch
> or one of the auxiliaries)
> 
> Hmm, use as a batch + cmdparser is broken; should result in ENODEV to
> userspace. An admittedly odd result, but not an oops.
GEM proxy isn't designed to be used as a batch buffer, only as an auxiliary. So I think here you mean we need to ban GEM proxy in i915_gem_execbuffer/i915_gem_execbuffer2, right?

> 
> display: pin-to-display and fencing will work, I think, and flush_for_display is
> irrelevant as no direct cpu access.
> 
> dmabuf: works partially, but what doesn't work is optional. Userspace mmap is
> barred, most inter-driver interaction is barred. On paper a nuisance should it
> want to be used, but we should not oops. Hmm, no, we need to make the
> GEM_BUG_ON(!struct_page(obj)) in
> i915_gem_object_pin_map() a real return.
So, as this can benefit all kinds of gem object w/o backing storage, can we split the following part into another patch?
Thanks.

BR,
Tina
> 
> Please add
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index d699ea3ab80b..6a7ca9a502ba
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2650,7 +2650,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)
> 
> and my
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx