* Re: [PATCH v3] drm/i915: Introduce GEM proxy
2017-11-13 6:49 ` [PATCH v3] drm/i915: Introduce GEM proxy Tina Zhang
@ 2017-11-14 13:30 ` Zhang, Tina
0 siblings, 0 replies; 4+ messages in thread
From: Zhang, Tina @ 2017-11-14 13:30 UTC (permalink / raw)
To: chris@chris-wilson.co.uk, joonas.lahtinen@linux.intel.com,
zhenyuw@linux.intel.com, Lv, Zhiyuan, Wang, Zhi A, Tian, Kevin,
daniel@ffwll.ch, Yuan, Hang
Cc: Daniel Vetter, intel-gfx@lists.freedesktop.org,
intel-gvt-dev@lists.freedesktop.org
I think we might miss the discussion in the first version. Here is the address.
https://lists.freedesktop.org/archives/intel-gvt-dev/2017-November/002461.html
For convenience, I just paste it here:
> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Tuesday, October 17, 2017 10:37 PM
> To: Zhang, Tina <tina.zhang at intel.com>; zhenyuw at linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv at intel.com>; Wang, Zhi A <zhi.a.wang at intel.com>; Tian, Kevin
> <kevin.tian at intel.com>; daniel at ffwll.ch
> 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: 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 at chris-wilson.co.uk> -Chris
The following is the third version:
> -----Original Message-----
> From: Zhang, Tina
> Sent: Monday, November 13, 2017 2:50 PM
> To: chris@chris-wilson.co.uk; joonas.lahtinen@linux.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;
> Yuan, Hang <hang.yuan@intel.com>
> 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 v3] 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.
>
> v3:
> - update "Reviewed-by". (Joonas)
>
> 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> v1
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> v2
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread