Re: [PATCH v5 4/5] drm/i915/gvt: Dmabuf support for GVT-g
From: Alex Williamson
Date: Tue May 23 2017 - 15:26:23 EST
On Tue, 23 May 2017 18:32:00 +0800
Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gvt/Makefile | 2 +-
> drivers/gpu/drm/i915/gvt/dmabuf.c | 276 +++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/gvt/dmabuf.h | 53 +++++++
> drivers/gpu/drm/i915/gvt/gvt.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 8 +
> drivers/gpu/drm/i915/i915_gem_object.h | 9 ++
> 6 files changed, 348 insertions(+), 1 deletion(-)
> 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..415453b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -0,0 +1,276 @@
> +/*
> + * 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@xxxxxxxxx>
> + *
> + * Contributors:
> + * Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx>
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <drm/drmP.h>
> +
> +#include "i915_drv.h"
> +#include "gvt.h"
> +
> +#define GEN8_DECODE_PTE(pte) \
> + ((dma_addr_t)(((((u64)pte) >> 12) & 0x7ffffffULL) << 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;
> + unsigned int fb_gma = 0, fb_size = 0;
> + struct intel_vgpu_plane_info *plane_info;
> +
> + plane_info = (struct intel_vgpu_plane_info *)obj->gvt_plane_info;
I can't find where gvt_plane_info is defined, but it's curious why it's
not already this type.
> + if (WARN_ON(!plane_info))
> + return ERR_PTR(-EINVAL);
> +
> + fb_gma = plane_info->start;
> + fb_size = plane_info->size;
> +
> + st = kmalloc(sizeof(*st), GFP_KERNEL);
> + if (!st) {
> + ret = -ENOMEM;
> + return ERR_PTR(ret);
> + }
> +
> + ret = sg_alloc_table(st, fb_size, GFP_KERNEL);
> + if (ret) {
> + kfree(st);
> + return ERR_PTR(ret);
> + }
> + gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> + (fb_gma >> PAGE_SHIFT);
> + for_each_sg(st->sgl, sg, fb_size, i) {
> + sg->offset = 0;
> + sg->length = PAGE_SIZE;
> + sg_dma_address(sg) =
> + GEN8_DECODE_PTE(readq(>t_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)
> +{
> + struct intel_vgpu_plane_info *plane_info;
> +
> + plane_info = (struct intel_vgpu_plane_info *)obj->gvt_plane_info;
> + if (WARN_ON(!plane_info))
> + return;
> +
> + sg_free_table(pages);
> + kfree(pages);
> +}
> +
> +static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = {
> + .flags = I915_GEM_OBJECT_IS_GVT_DMABUF,
> + .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 intel_vgpu_plane_info *info)
> +{
> + struct drm_i915_private *pri = dev->dev_private;
> + struct drm_i915_gem_object *obj;
> +
> + obj = i915_gem_object_alloc(pri);
> + 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;
> + obj->framebuffer_references++;
> + obj->gvt_plane_info = info;
> +
> + if (IS_SKYLAKE(pri)) {
> + unsigned int tiling_mode = 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;
> + break;
> + case PLANE_CTL_TILED_Y:
> + tiling_mode = I915_TILING_Y;
> + break;
> + default:
> + gvt_dbg_core("not supported tiling mode\n");
> + }
> + obj->tiling_and_stride = tiling_mode | info->stride;
> + } else {
> + obj->tiling_and_stride = info->drm_format_mod ?
> + I915_TILING_X : 0;
> + }
> +
> + return obj;
> +}
> +
> +static struct intel_vgpu_plane_info *intel_vgpu_get_plane_info(
> + struct drm_device *dev,
> + struct intel_vgpu *vgpu, uint32_t plane_id)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_vgpu_primary_plane_format *p;
> + struct intel_vgpu_cursor_plane_format *c;
> + struct intel_vgpu_plane_info *info;
> + struct intel_vgpu_pipe_format *pipe;
> +
> + info = kmalloc(sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return NULL;
> +
> + pipe = intel_vgpu_decode_plane(dev, vgpu);
> + if (pipe == NULL)
> + return NULL;
Leaks info, reverse order?
> +
> + if (plane_id == INTEL_GVT_PLANE_PRIMARY) {
> + p = &pipe->primary;
> + if (p != NULL) {
> + info->start = p->base;
> + info->width = p->width;
> + info->height = p->height;
> + info->stride = p->stride;
> + info->drm_format = p->drm_format;
> + info->drm_format_mod = p->tiled;
> + info->size = (((p->stride * p->height * p->bpp) / 8) +
> + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
> + } else {
> + kfree(info);
> + gvt_vgpu_err("invalid primary plane\n");
> + return NULL;
> + }
> + } else if (plane_id == INTEL_GVT_PLANE_CURSOR) {
> + c = &pipe->cursor;
> + if (c != NULL) {
> + info->start = c->base;
> + info->width = c->width;
> + info->height = c->height;
> + info->stride = c->width * (c->bpp / 8);
> + info->drm_format_mod = 0;
> + info->x_pos = c->x_pos;
> + info->y_pos = c->y_pos;
> + info->size = (((info->stride * c->height * c->bpp) / 8)
> + + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
> + } else {
> + kfree(info);
> + gvt_vgpu_err("invalid cursor plane\n");
> + return NULL;
> + }
> + } else {
> + kfree(info);
> + gvt_vgpu_err("invalid plane id:%d\n", plane_id);
> + return NULL;
> + }
> +
> + if (info->size == 0) {
> + kfree(info);
> + gvt_vgpu_err("fb size is zero\n");
> + return NULL;
> + }
> +
> + if (info->start & (PAGE_SIZE - 1)) {
> + kfree(info);
> + gvt_vgpu_err("Not aligned fb address:0x%x\n", info->start);
> + return NULL;
> + }
> + if (((info->start >> PAGE_SHIFT) + info->size) >
> + ggtt_total_entries(&dev_priv->ggtt)) {
> + kfree(info);
> + gvt_vgpu_err("Invalid GTT offset or size\n");
> + return NULL;
> + }
> +
> + if (!intel_gvt_ggtt_validate_range(vgpu, info->start, info->size)) {
> + kfree(info);
> + gvt_vgpu_err("invalid gma addr\n");
> + return NULL;
> + }
Would it be useful to use ERR_PTR() so we could pass a relevant errno
back up the stack to the user?
> +
> + return info;
> +}
> +
> +int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args)
> +{
> + struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
> + struct intel_vgpu_plane_info *info = args;
> +
> + info = intel_vgpu_get_plane_info(dev, vgpu, info->plane_id);
> + if (info == NULL)
> + return -EINVAL;
Rather than this?
> +
> + return 0;
> +}
> +
> +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 intel_vgpu_dmabuf *gvt_dmabuf = args;
> + struct intel_vgpu_plane_info *info;
> + int ret;
> +
> + info = intel_vgpu_get_plane_info(dev, vgpu,
> + gvt_dmabuf->plane_info.plane_id);
> + if (info == NULL)
> + return -EINVAL;
> +
> + obj = intel_vgpu_create_gem(dev, info);
> + if (obj == NULL) {
> + gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
> + return -EINVAL;
@info is leaked?
> + }
> +
> + dmabuf = i915_gem_prime_export(dev, &obj->base, DRM_CLOEXEC | DRM_RDWR);
> +
> + if (IS_ERR(dmabuf)) {
> + gvt_vgpu_err("export dma-buf failed\n");
> + return -EINVAL;
@info & @obj leaked?
> + }
> +
> + ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR);
> + if (ret < 0) {
> + gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
> + return -EINVAL;
same
Also, the fd is provided to the user and is a reference to the device,
we need to increment the vfio_device reference and decrement on release.
> + }
> + gvt_dmabuf->fd = ret;
> + gvt_dmabuf->plane_info = *info;
> +
> + return 0;
same
> +}
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h b/drivers/gpu/drm/i915/gvt/dmabuf.h
> new file mode 100644
> index 0000000..43562af
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
> @@ -0,0 +1,53 @@
> +
> +/*
> + * Copyright(c) 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.
> + *
> + */
> +
> +#ifndef _GVT_DMABUF_H_
> +#define _GVT_DMABUF_H_
> +
> +struct intel_vgpu_plane_info {
> + uint32_t plane_id;
> + uint32_t drm_format;
> + uint32_t width;
> + uint32_t height;
> + uint32_t stride;
> + uint32_t start;
> + uint32_t x_pos;
> + uint32_t y_pos;
> + uint32_t size;
> + uint64_t drm_format_mod;
> +};
Alignment issue as Gerd mentions.
> +
> +#define INTEL_VGPU_QUERY_PLANE 0
> +#define INTEL_VGPU_GENERATE_DMABUF 1
> +
> +struct intel_vgpu_dmabuf {
> + uint32_t fd;
> + struct intel_vgpu_plane_info plane_info;
> +};
And here, also as Gerd mentions.
> +
> +int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args);
> +int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args);
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index c42266c..763a8c5 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -47,6 +47,7 @@
> #include "render.h"
> #include "cmd_parser.h"
> #include "fb_decoder.h"
> +#include "dmabuf.h"
>
> #define GVT_MAX_VGPU 8
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0c1cbe9..54f7a0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1609,6 +1609,10 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> if (!obj)
> return -ENOENT;
>
> + /* The obj is a gvt dma-buf object and set domain is not supported */
> + if (i915_gem_object_is_gvt_dmabuf(obj))
> + return -EPERM;
> +
> /* 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.
> @@ -3717,6 +3721,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> if (!obj)
> return -ENOENT;
>
> + /* the obj is a gvt dma-buf and set caching mode is not supported */
> + if (i915_gem_object_is_gvt_dmabuf(obj))
> + return -EPERM;
> +
> if (obj->cache_level == level)
> goto out;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> index 174cf92..986af43 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 0x1
> #define I915_GEM_OBJECT_IS_SHRINKABLE 0x2
> +#define I915_GEM_OBJECT_IS_GVT_DMABUF 0x4
>
> /* Interface between the GEM object and its backing storage.
> * get_pages() is called once prior to the use of the associated set
> @@ -184,6 +185,8 @@ struct drm_i915_gem_object {
> } userptr;
>
> unsigned long scratch;
> +
> + void *gvt_plane_info;
> };
>
> /** for phys allocated objects */
> @@ -286,6 +289,12 @@ i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
> }
>
> static inline bool
> +i915_gem_object_is_gvt_dmabuf(const struct drm_i915_gem_object *obj)
> +{
> + return obj->ops->flags & I915_GEM_OBJECT_IS_GVT_DMABUF;
> +}
> +
> +static inline bool
> i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
> {
> return obj->active_count;