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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Wed, 2017-11-01 at 17:22 +0800, Tina Zhang wrote:
> 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.
> 
> 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

<SNIP>

> @@ -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);

You should have marked this change in the changelog and then marked the
Reviewed-by tags to be valid only to the previous version of this
patch.

It's not a fair game to claim a patch to be "Reviewed-by" at the
current version, when you've made changes that were not agreed upon.

So that's some meta-review. Back to the actual review;

Which codepath was hitting the GEM_BUG_ON? Wondering if it would be
cleaner to avoid the call to this function on that single codepath.

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