Message-ID: <20170601091547.GB11742@nuc-i3427.alporthouse.com>
Date:   Thu, 1 Jun 2017 10:15:47 +0100
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Xiaoguang Chen <xiaoguang.chen@...el.com>
Cc:     alex.williamson@...hat.com, kraxel@...hat.com,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        zhenyuw@...ux.intel.com, zhiyuan.lv@...el.com,
        intel-gvt-dev@...ts.freedesktop.org, zhi.a.wang@...el.com,
        kevin.tian@...el.com
Subject: Re: [PATCH v6 5/6] drm/i915/gvt: Dmabuf support for GVT-g

On Sat, May 27, 2017 at 04:38:51PM +0800, Xiaoguang Chen wrote:
> dmabuf for GVT-g can be exported to users who can use the dmabuf to show
> the desktop of vm which use intel vgpu.
> 
> Currently we provide query and create new dmabuf operations.
> 
> Users of dmabuf can cache some created dmabufs and related information
> such as the framebuffer's address, size, tiling mode, width, height etc.
> When refresh the screen first query the currnet vgpu's frambuffer and
> compare with the cached ones(address, size, tiling, width, height etc)
> if found one then reuse the found dmabuf to gain performance improvment.
> 
> If there is no dmabuf created yet or not found in the cached dmabufs then
> need to create a new dmabuf. To create a dmabuf first a gem object will
> be created and the backing storage of this gem object is the vgpu's
> framebuffer(primary/cursor).
> Set caching mode, change tiling mode and set domains of this gem object
> is not supported.
> Then associate this gem object to a dmabuf and export this dmabuf.
> A file descriptor will be generated for this dmabuf and this file
> descriptor can be sent to user space to do display.
> 
> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@...el.com>
> ---
>  drivers/gpu/drm/i915/gvt/Makefile      |   2 +-
>  drivers/gpu/drm/i915/gvt/dmabuf.c      | 269 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/dmabuf.h      |  37 +++++
>  drivers/gpu/drm/i915/gvt/gvt.h         |   1 +
>  drivers/gpu/drm/i915/i915_gem.c        |  26 +++-
>  drivers/gpu/drm/i915/i915_gem_object.h |   9 ++
>  6 files changed, 342 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
>  create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h
> 
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
> index 192ca26..e480f7d 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -2,7 +2,7 @@ GVT_DIR := gvt
>  GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
>  	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
>  	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
> -	fb_decoder.o
> +	fb_decoder.o dmabuf.o
>  
>  ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR) -Wall
>  i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
> new file mode 100644
> index 0000000..c831e91
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -0,0 +1,269 @@
> +/*
> + * Copyright 2017 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Zhiyuan Lv <zhiyuan.lv@...el.com>
> + *
> + * Contributors:
> + *    Xiaoguang Chen <xiaoguang.chen@...el.com>
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <drm/drmP.h>
> +#include <linux/vfio.h>
> +
> +#include "i915_drv.h"
> +#include "gvt.h"
> +
> +#define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12))
> +
> +static struct sg_table *intel_vgpu_gem_get_pages(
> +		struct drm_i915_gem_object *obj)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> +	struct sg_table *st;
> +	struct scatterlist *sg;
> +	int i, ret;
> +	gen8_pte_t __iomem *gtt_entries;
> +	struct intel_vgpu_fb_info *fb_info;
> +
> +	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
> +	if (WARN_ON(!fb_info))
> +		return ERR_PTR(-ENODEV);
> +
> +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> +	if (!st) {
> +		ret = -ENOMEM;
> +		return ERR_PTR(ret);

Tidier.

> +	}
> +
> +	ret = sg_alloc_table(st, fb_info->fb_size, GFP_KERNEL);
> +	if (ret) {
> +		kfree(st);
> +		return ERR_PTR(ret);
> +	}
> +	gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> +		(fb_info->fb_addr >> PAGE_SHIFT);
> +	for_each_sg(st->sgl, sg, fb_info->fb_size, i) {
> +		sg->offset = 0;
> +		sg->length = PAGE_SIZE;
> +		sg_dma_address(sg) =
> +			GEN8_DECODE_PTE(readq(&gtt_entries[i]));
> +		sg_dma_len(sg) = PAGE_SIZE;
> +	}
> +
> +	return st;
> +}
> +
> +static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj,
> +		struct sg_table *pages)
> +{
> +	sg_free_table(pages);
> +	kfree(pages);
> +}
> +
> +static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = {
> +	.flags = I915_GEM_OBJECT_IS_PROXY,
> +	.get_pages = intel_vgpu_gem_get_pages,
> +	.put_pages = intel_vgpu_gem_put_pages,
> +};
> +
> +static struct drm_i915_gem_object *intel_vgpu_create_gem(struct drm_device *dev,
> +		struct vfio_vgpu_plane_info *info)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_gem_object *obj;
> +
> +	obj = i915_gem_object_alloc(dev_priv);
> +	if (obj == NULL)
> +		return NULL;
> +
> +	drm_gem_private_object_init(dev, &obj->base,
> +		info->size << PAGE_SHIFT);
> +	i915_gem_object_init(obj, &intel_vgpu_gem_ops);
> +
> +	obj->base.read_domains = I915_GEM_DOMAIN_GTT;
> +	obj->base.write_domain = 0;
> +	/* Change tiling mode from user space is not supported to a GVT-g's
> +	 * dma-buf gem object.
> +	 * We increase the obj's framebuffer_references to disable this kind
> +	 * of operation.
> +	 */
> +	obj->framebuffer_references++;

If it is only for that, don't. Just use the is_proxy: return -EPERM. So
that you don't make the mistake of leaving the counter dangling and the
error code is consistent.

What you probably should consider though is setting obj->pin_display.
That will fix up coherency along a few more paths.

> +	if (IS_SKYLAKE(dev_priv)) {
> +		unsigned int tiling_mode = 0;
> +		unsigned int stride = 0;
> +
> +		switch (info->drm_format_mod << 10) {
> +		case PLANE_CTL_TILED_LINEAR:
> +			tiling_mode = I915_TILING_NONE;
> +			break;
> +		case PLANE_CTL_TILED_X:
> +			tiling_mode = I915_TILING_X;
> +			stride = info->stride;
> +			break;
> +		case PLANE_CTL_TILED_Y:
> +			tiling_mode = I915_TILING_Y;
> +			stride = info->stride;
> +			break;
> +		default:
> +			gvt_dbg_core("not supported tiling mode\n");
> +		}
> +		obj->tiling_and_stride = tiling_mode | stride;
> +	} else {
> +		obj->tiling_and_stride = info->drm_format_mod ?
> +					I915_TILING_X : 0;
> +	}
> +
> +	return obj;
> +}

> +
> +int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args)
> +{
> +	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
> +	struct vfio_vgpu_plane_info *info = args;
> +
> +	return intel_vgpu_get_plane_info(dev, vgpu, info);
> +}
> +
> +int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
> +{
> +	struct dma_buf *dmabuf;
> +	struct drm_i915_gem_object *obj;
> +	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
> +	struct vfio_vgpu_dmabuf_info *gvt_dmabuf = args;
> +	struct intel_vgpu_fb_info *fb_info;
> +	int ret;
> +
> +	ret = intel_vgpu_get_plane_info(dev, vgpu, &gvt_dmabuf->plane_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	obj = intel_vgpu_create_gem(dev, &gvt_dmabuf->plane_info);
> +	if (obj == NULL) {
> +		gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
> +		return -ENOMEM;
> +	}
> +	fb_info = kmalloc(sizeof(*fb_info), GFP_KERNEL);
> +	if (!fb_info) {
> +		i915_gem_object_put(obj);
> +		gvt_vgpu_err("allocate intel vgpu fb info failed\n");
> +		return -ENOMEM;
> +	}
> +	fb_info->fb_addr = gvt_dmabuf->plane_info.start;
> +	fb_info->fb_size = gvt_dmabuf->plane_info.size;
> +	fb_info->vgpu = vgpu;
> +	obj->gvt_info = fb_info;
> +
> +	dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);
> +
> +	if (IS_ERR(dmabuf)) {
> +		kfree(fb_info);
> +		i915_gem_object_free(obj);
> +		gvt_vgpu_err("export dma-buf failed\n");

Develop a habit for using onion unwind for errors.

> +		return PTR_ERR(dmabuf);
> +	}
> +
> +	ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR);
> +	if (ret < 0) {
> +		kfree(fb_info);
> +		i915_gem_object_free(obj);
> +		gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
> +		return ret;
> +	}
> +	gvt_dmabuf->fd = ret;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0c1cbe9..14be3b0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1609,6 +1609,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)) {
> +		i915_gem_object_put(obj);
> +		return -EPERM;

Hook into the onion.

> +	}
> +
>  	/* 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.
> @@ -1677,6 +1683,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);
> @@ -1727,7 +1739,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,
> @@ -3717,6 +3729,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)) {
> +		i915_gem_object_put(obj);
> +		return -EPERM;

Use the onion.
> +	}
> +
>  	if (obj->cache_level == level)
>  		goto out;
>  
> @@ -4168,6 +4186,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)) {
> +		i915_gem_object_put(obj);

More onion.

> +		return -EPERM;
> +	}
> +
>  	err = mutex_lock_interruptible(&obj->mm.lock);
>  	if (err)
>  		goto out;

-- 
Chris Wilson, Intel Open Source Technology Centre

Powered by blists - more mailing lists