RE: [Intel-gfx] [RFC PATCH 5/6] drm/i915/gvt: dmabuf support for GVT-g

[Chen, Xiaoguang]

>-----Original Message-----
>From: Chris Wilson [mailto:chris@xxxxxxxxxxxxxxxxxx]
>Sent: Friday, April 28, 2017 6:09 PM
>To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>
>Cc: kraxel@xxxxxxxxxx; alex.williamson@xxxxxxxxxx; intel-
>gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi A
><zhi.a.wang@xxxxxxxxx>; zhenyuw@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; Tian, Kevin
><kevin.tian@xxxxxxxxx>
>Subject: Re: [Intel-gfx] [RFC PATCH 5/6] drm/i915/gvt: dmabuf support for GVT-g
>
>On Fri, Apr 28, 2017 at 05:35:29PM +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). 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 | 268
>> ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/gvt/dmabuf.h |  50 +++++++
>>  drivers/gpu/drm/i915/gvt/gvt.h    |   1 +
>>  4 files changed, 320 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..d776dfa
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
>> @@ -0,0 +1,268 @@
>> +/*
>> + * 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"
>> +
>> +static struct sg_table *intel_vgpu_gem_get_pages(
>> +		struct drm_i915_gem_object *obj)
>> +{
>> +	WARN_ON(1);
>> +	return NULL;
>> +}
>> +
>> +static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj,
>> +		struct sg_table *pages)
>> +{
>> +	/* like stolen memory, this should only be called during free
>> +	 * after clearing pin count.
>> +	 */
>
>Time to re-read how stolen works (see get_pages and pinning on creation).
OK.

>
>> +	sg_free_table(pages);
>> +	kfree(pages);
>> +}
>> +
>> +static const struct drm_i915_gem_object_ops intel_vgpu_gem_ops = {
>> +	.get_pages = intel_vgpu_gem_get_pages,
>> +	.put_pages = intel_vgpu_gem_put_pages, };
>> +
>> +#define GEN8_DECODE_PTE(pte) \
>> +	((dma_addr_t)(((((u64)pte) >> 12) & 0x7ffffffULL) << 12))
>> +
>> +#define GEN7_DECODE_PTE(pte) \
>> +	((dma_addr_t)(((((u64)pte) & 0x7f0) << 28) | (u64)(pte &
>> +0xfffff000)))
>> +
>> +static struct sg_table *
>> +intel_vgpu_create_sg_pages(struct drm_device *dev, u32 start, u32
>> +num_pages) {
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct sg_table *st;
>> +	struct scatterlist *sg;
>> +	int i;
>> +	gen8_pte_t __iomem *gtt_entries;
>> +
>> +	st = kmalloc(sizeof(*st), GFP_KERNEL);
>> +	if (st == NULL)
>> +		return NULL;
>> +
>> +	if (sg_alloc_table(st, num_pages, GFP_KERNEL)) {
>> +		kfree(st);
>> +		return NULL;
>> +	}
>> +
>> +	gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
>> +		(start >> PAGE_SHIFT);
>> +	for_each_sg(st->sgl, sg, num_pages, 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;
>> +	}
>
>This should be get_pages.
The reason that I did not alloc pages to the gem obj in get_pages callback is that there may be sync problems:
1) decode the vgpu's current frambuffer.
2) save the decoded information(we will use size and address of the framebuffer later)
3) create a gem obj the size of the gem obj is the above framebuffer size. 
4) associate the gem obj to a dmabuf and export the dmabuf.
..........................
5) while user access the gem obj get_pages callback of the gem obj will be called to alloc pages for the gem obj.
6) use saved frambuffer's address to look up the gtt table to get the pages of the frambuffer. Assign the pages to gem obj.

There may be interval between step 4 and step 5. The framebuffer may have changed in step 5 but we are still using the old framebuffer's address(decoded in step 1) in step 5.

>
>> +	return st;
>> +}
>> +
>> +static struct drm_i915_gem_object *intel_vgpu_create_gem(struct
>drm_device *dev,
>> +		struct intel_vgpu_dmabuf *info)
>> +{
>> +	struct drm_i915_gem_object *obj;
>> +	struct drm_i915_private *pri = dev->dev_private;
>> +
>> +	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->mm.pages = intel_vgpu_create_sg_pages(dev, info->start,
>> +			info->size);
>> +	if (obj->mm.pages == NULL) {
>> +		i915_gem_object_free(obj);
>> +		return NULL;
>> +	}
>
>Having created the obj, just call i915_gem_object_pin_pages(). Or better yet,
>don't pin the pages upon creation and just defer it to first use - which be very
>soon.
Ok. 

>
>> +	obj->cache_level = I915_CACHE_L3_LLC;
>
>Are you sure?
Will check this.

>
>
>> +	if (IS_SKYLAKE(pri)) {
>> +		unsigned int tiling_mode = 0;
>> +
>> +		switch (info->tiled << 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("tile %d not supported\n", info->tiled);
>> +		}
>> +		obj->tiling_and_stride = tiling_mode | info->stride;
>
>If tiling_mode == 0, stride must be zero. Is that enforced?
Will correct that.

>
>> +	} else {
>> +		obj->tiling_and_stride = (info->tiled ? I915_TILING_X :
>> +			I915_TILING_NONE) | (info->tiled ? info->stride : 0);
>
>Rewrite this neatly.
>Hint obj->tiling_and_stride starts as zero and you don't need to do any of this
>if !tiled.
Will simplify this logic.

>
>> +	}
>> +
>> +	return obj;
>> +}
>> +
>> +static int intel_vgpu_get_plane_info(struct drm_device *dev,
>> +		struct intel_vgpu *vgpu,
>> +		struct intel_vgpu_dmabuf *info)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_vgpu_primary_plane_format *p;
>> +	struct intel_vgpu_cursor_plane_format *c;
>> +
>> +	if (info->plane_id == INTEL_GVT_PLANE_PRIMARY) {
>> +		p = (struct intel_vgpu_primary_plane_format *)
>> +			intel_vgpu_decode_plane(dev, vgpu, info->plane_id);
>> +		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->tiled = p->tiled;
>> +			info->size = (((p->stride * p->height * p->bpp) / 8) +
>> +					(PAGE_SIZE - 1)) >> PAGE_SHIFT;
>> +		} else {
>> +			gvt_dbg_core("invalid primary plane\n");
>> +			return -EINVAL;
>> +		}
>> +	} else if (info->plane_id == INTEL_GVT_PLANE_CURSOR) {
>> +		c = (struct intel_vgpu_cursor_plane_format *)
>> +			intel_vgpu_decode_plane(dev, vgpu, info->plane_id);
>> +		if (c != NULL) {
>> +			info->start = c->base;
>> +			info->width = c->width;
>> +			info->height = c->height;
>> +			info->stride = c->width * (c->bpp / 8);
>> +			info->tiled = 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 {
>> +			gvt_dbg_core("invalid cursor plane\n");
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		gvt_vgpu_err("invalid plane id:%d\n", info->plane_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (info->start & (PAGE_SIZE - 1)) {
>> +		gvt_vgpu_err("Not aligned fb address:0x%x\n", info->start);
>> +		return -EINVAL;
>> +	}
>> +	if (((info->start >> PAGE_SHIFT) + info->size) >
>> +		ggtt_total_entries(&dev_priv->ggtt)) {
>> +		gvt_vgpu_err("Invalid GTT offset or size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct drm_i915_gem_object *intel_vgpu_create_gem_from_vgpuid(
>> +		struct drm_device *dev, struct intel_vgpu *vgpu,
>> +		struct intel_vgpu_dmabuf *info)
>> +{
>> +	struct drm_i915_gem_object *obj;
>> +	int ret;
>> +
>> +	ret = intel_vgpu_get_plane_info(dev, vgpu, info);
>> +	if (ret) {
>> +		gvt_vgpu_err("get plane info failed:%d\n", info->plane_id);
>> +		return NULL;
>
>Propagate errors.
Will remove this err message.

>
>> +	}
>> +	obj = intel_vgpu_create_gem(dev, info);
>> +
>> +	return obj;
>> +}
>> +
>> +int intel_vgpu_query_dmabuf(struct intel_vgpu *vgpu, void *args) {
>> +	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
>> +	int ret;
>> +	struct intel_vgpu_dmabuf *info = args;
>> +
>> +	ret = intel_vgpu_get_plane_info(dev, vgpu, info);
>> +	if (ret) {
>> +		gvt_vgpu_err("get plane info failed:%d\n", info->plane_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int intel_vgpu_generate_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;
>> +	int ret;
>> +	struct intel_vgpu_dmabuf *info = args;
>> +	struct dma_buf_export_info exp_info = {
>> +		.exp_name = KBUILD_MODNAME,
>> +		.owner = THIS_MODULE };
>> +
>> +	obj = intel_vgpu_create_gem_from_vgpuid(dev, vgpu, info);
>> +	if (obj == NULL) {
>> +		gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	exp_info.ops = &i915_dmabuf_ops;
>
>Please put back my private i915_dmabuf_ops from where you stole it from.
>
>Just call dmabuf = i915_gem_prime_export(dev, obj, DRM_CLOEXEC |
>DRM_RDWR);
OK.  Will do that. Thanks for reminding this :)

>
>> +	exp_info.size = obj->base.size;
>> +	exp_info.flags = DRM_CLOEXEC | DRM_RDWR;
>> +	exp_info.priv = &obj->base;
>> +	exp_info.resv = obj->resv;
>> +
>> +	dmabuf = drm_gem_dmabuf_export(dev, &exp_info);
>> +	if (IS_ERR(dmabuf)) {
>> +		gvt_vgpu_err("intel vgpu export dma-buf failed\n");
>> +		mutex_unlock(&dev->object_name_lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = dma_buf_fd(dmabuf, exp_info.flags);
>> +	if (ret < 0) {
>> +		gvt_vgpu_err("intel vgpu create dma-buf fd failed\n");
>> +		return ret;
>> +	}
>> +	info->fd = ret;
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h
>> b/drivers/gpu/drm/i915/gvt/dmabuf.h
>> new file mode 100644
>> index 0000000..c590f4a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
>> @@ -0,0 +1,50 @@
>> +/*
>> + * 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_
>> +
>> +#define INTEL_VGPU_QUERY_DMABUF		0
>> +#define INTEL_VGPU_GENERATE_DMABUF	1
>> +
>> +struct intel_vgpu_dmabuf {
>
>This looks to be uapi. What's it doing here?
This structure includes the information that will be delivered to users who request the dma-buf.

>-Chris
>
>--
>Chris Wilson, Intel Open Source Technology Centre