| Message ID | 1500461945-16801-7-git-send-email-tina.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
s/GEM proxy/a GEM proxy object/ Quoting Tina Zhang (2017-07-19 11:59:05) Rationale goes here. > Signed-off-by: Tina Zhang <tina.zhang@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. > + 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. > + /* 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? > + 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? > + 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. -Chris
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Wednesday, July 19, 2017 7:20 PM > To: Zhang, Tina <tina.zhang@intel.com>; intel-gfx@lists.freedesktop.org; intel- > gvt-dev@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > ville.syrjala@linux.intel.com; zhenyuw@linux.intel.com; Lv, Zhiyuan > <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; > alex.williamson@redhat.com; kraxel@redhat.com; daniel@ffwll.ch; > kwankhede@nvidia.com; Tian, Kevin <kevin.tian@intel.com> > Cc: Zhang, Tina <tina.zhang@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@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
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 */ + 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; + /* 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 */ + 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 */ + 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; }; /** 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; + } + if (!i915_tiling_ok(obj, args->tiling_mode, args->stride)) { err = -EINVAL; goto err;
Signed-off-by: Tina Zhang <tina.zhang@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(-)