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@xxxxxxxxx>
> ---
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
