[Intel-gfx] [PATCH v12 6/6] drm/i915: Introduce GEM proxy
Zhang, Tina
tina.zhang at intel.com
Fri Jul 21 04:49:48 UTC 2017
> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Wednesday, July 19, 2017 7:20 PM
> To: Zhang, Tina <tina.zhang at intel.com>; intel-gfx at lists.freedesktop.org; intel-
> gvt-dev at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> ville.syrjala at linux.intel.com; zhenyuw at linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv at intel.com>; Wang, Zhi A <zhi.a.wang at intel.com>;
> alex.williamson at redhat.com; kraxel at redhat.com; daniel at ffwll.ch;
> kwankhede at nvidia.com; Tian, Kevin <kevin.tian at intel.com>
> Cc: Zhang, Tina <tina.zhang at intel.com>
> Subject: Re: [PATCH v12 6/6] drm/i915: Introduce GEM proxy
>
> s/GEM proxy/a GEM proxy object/
>
> Quoting Tina Zhang (2017-07-19 11:59:05)
>
> Rationale goes here.
Thanks for the comments. The idea behind the GEM proxy is that we want to propose a kind of GEM which content is produced by the guest VM and used by host. So, to host, this kind of GEM should be read only (both for CPU and GPU), and host cannot use ioctls to change or modify the GEM.
I will add more comments in the next version. Thanks.
>
> > Signed-off-by: Tina Zhang <tina.zhang at 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.
Is it the requirement of this ioctl? Other ioctls here in this patch, won't need it?
>
> > + 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.
Agree.
>
> > + /* 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?
From v2, cache_level isn't be set during the GEM generation, i.e. cache_level=0. And the PTE bits which is set by guest, won't be modified by host.
>
> > + 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?
I suppose no. The backend pages should always be the guest framebuffer. And when to release it is up to user mode. If the kernel part senses the guest framebuffer is changed, it will generate a new GEM/dma-buf with the new framebuffer backend, to user mode.
>
> > + 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.
The GEM's content and tiling mode is produced by guest, in host, how could we guarantee the correction of the view if we allow the tiling modification through this ioctl?
And I guess we tried the obj->framebuffer_references before, based on the discussion in v6 (https://lists.freedesktop.org/archives/intel-gvt-dev/2017-June/001146.html)
Thanks.
Tina
> -Chris
More information about the Intel-gfx
mailing list