[RFC PATCH 0/6] drm/i915/gvt: dma-buf support for GVT-g

[RFC PATCH 0/6] drm/i915/gvt: dma-buf support for GVT-g

[Xiaoguang Chen]

This patch set adds the dma-buf support for intel GVT-g.
dma-buf is a uniform mechanism to share DMA buffers across different
devices and sub-systems.
dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer
to other users or sub-systems so they can use the dma-buf to show the
desktop of a vm which uses intel vgpu.

The main idea is we create a gem object and set vgpu's framebuffer as
the backing storage of this gem object. And associate this gem obj
to a dma-buf object then export this dma-buf at the meantime
generate a file descriptor for this dma-buf. Finally deliver this file
descriptor to end users.
In the implementation we create an anonymous fd and send this fd to users
using an vfio device region. Once users get this anonymous fd they can do
ioctl using this anonymous fd to get the file descriptor we generated for
dma-buf.

We have an example program on how to use the dma-buf. You can download
the program to have a try :)
git repo: https://github.com/01org/igvtg-qemu branch:kvmgt_dmabuf_example

Xiaoguang Chen (6):
  drm/i915/gvt: extend the GVT-g architecture to support vfio device
    region
  drm/i915/gvt: OpRegion support for GVT-g
  drm/i915/gvt: framebuffer decoder support for GVT-g
  drm/i915: export i915 dmabuf_ops
  drm/i915/gvt: dmabuf support for GVT-g
  drm/i915/gvt: support QEMU getting the dmabuf

 drivers/gpu/drm/i915/gvt/Makefile      |   3 +-
 drivers/gpu/drm/i915/gvt/display.c     |   2 +-
 drivers/gpu/drm/i915/gvt/display.h     |   2 +
 drivers/gpu/drm/i915/gvt/dmabuf.c      | 268 ++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h      |  50 ++++
 drivers/gpu/drm/i915/gvt/fb_decoder.c  | 487 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h  | 170 ++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.c         |   2 +
 drivers/gpu/drm/i915/gvt/gvt.h         |   4 +
 drivers/gpu/drm/i915/gvt/kvmgt.c       | 227 ++++++++++++++-
 drivers/gpu/drm/i915/gvt/opregion.c    |  12 +-
 drivers/gpu/drm/i915/i915_drv.h        |   2 +
 drivers/gpu/drm/i915/i915_gem_dmabuf.c |   2 +-
 include/uapi/linux/vfio.h              |   1 +
 14 files changed, 1224 insertions(+), 8 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h

-- 
1.9.1

[RFC PATCH 1/6] drm/i915/gvt: extend the GVT-g architecture to support vfio device region

[Xiaoguang Chen]

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 1ae0b40..3c6a02b 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -53,11 +53,21 @@
 #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT)
 #define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
 
+struct vfio_region;
+struct intel_vgpu_regops {
+	size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
+			size_t count, loff_t *ppos, bool iswrite);
+	void (*release)(struct intel_vgpu *vgpu,
+			struct vfio_region *region);
+};
+
 struct vfio_region {
 	u32				type;
 	u32				subtype;
 	size_t				size;
 	u32				flags;
+	const struct intel_vgpu_regops	*ops;
+	void				*data;
 };
 
 struct kvmgt_pgfn {
@@ -642,7 +652,7 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	int ret = -EINVAL;
 
 
-	if (index >= VFIO_PCI_NUM_REGIONS) {
+	if (index >= VFIO_PCI_NUM_REGIONS + vgpu->vdev.num_regions) {
 		gvt_vgpu_err("invalid index: %u\n", index);
 		return -EINVAL;
 	}
@@ -676,8 +686,11 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	case VFIO_PCI_BAR5_REGION_INDEX:
 	case VFIO_PCI_VGA_REGION_INDEX:
 	case VFIO_PCI_ROM_REGION_INDEX:
+		break;
 	default:
-		gvt_vgpu_err("unsupported region: %u\n", index);
+		index -= VFIO_PCI_NUM_REGIONS;
+		return vgpu->vdev.region[index].ops->rw(vgpu, buf, count,
+				ppos, is_write);
 	}
 
 	return ret == 0 ? count : ret;
@@ -940,7 +953,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 
 		info.flags = VFIO_DEVICE_FLAGS_PCI;
 		info.flags |= VFIO_DEVICE_FLAGS_RESET;
-		info.num_regions = VFIO_PCI_NUM_REGIONS;
+		info.num_regions = VFIO_PCI_NUM_REGIONS +
+				vgpu->vdev.num_regions;
 		info.num_irqs = VFIO_PCI_NUM_IRQS;
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
@@ -1061,6 +1075,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		}
 
 		if (caps.size) {
+			info.flags |= VFIO_REGION_INFO_FLAG_CAPS;
 			if (info.argsz < sizeof(info) + caps.size) {
 				info.argsz = sizeof(info) + caps.size;
 				info.cap_offset = 0;
-- 
1.9.1

[RFC PATCH 2/6] drm/i915/gvt: OpRegion support for GVT-g

[Xiaoguang Chen]

OpRegion is needed to support display related operation for
intel vgpu.

A vfio device region is added to intel vgpu to deliver the
host OpRegion information to user space so user space can
construct the OpRegion for vgpu.

Signed-off-by: Bing Niu <bing.niu@intel.com>
Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c    | 97 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/opregion.c | 12 ++++-
 2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 3c6a02b..389f072 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -53,6 +53,8 @@
 #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT)
 #define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
 
+#define OPREGION_SIGNATURE "IntelGraphicsMem"
+
 struct vfio_region;
 struct intel_vgpu_regops {
 	size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
@@ -436,6 +438,92 @@ static void kvmgt_protect_table_del(struct kvmgt_guest_info *info,
 	}
 }
 
+static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf,
+		size_t count, loff_t *ppos, bool iswrite)
+{
+	unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
+			VFIO_PCI_NUM_REGIONS;
+	void *base = vgpu->vdev.region[i].data;
+	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
+
+	if (pos >= vgpu->vdev.region[i].size || iswrite) {
+		gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n");
+		return -EINVAL;
+	}
+	count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
+	memcpy(buf, base + pos, count);
+
+	return count;
+}
+
+static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu,
+		struct vfio_region *region)
+{
+	memunmap(region->data);
+}
+
+static const struct intel_vgpu_regops intel_vgpu_regops_opregion = {
+	.rw = intel_vgpu_reg_rw_opregion,
+	.release = intel_vgpu_reg_release_opregion,
+};
+
+static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
+		unsigned int type, unsigned int subtype,
+		const struct intel_vgpu_regops *ops,
+		size_t size, u32 flags, void *data)
+{
+	struct vfio_region *region;
+
+	region = krealloc(vgpu->vdev.region,
+			(vgpu->vdev.num_regions + 1) * sizeof(*region),
+			GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	vgpu->vdev.region = region;
+	vgpu->vdev.region[vgpu->vdev.num_regions].type = type;
+	vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype;
+	vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops;
+	vgpu->vdev.region[vgpu->vdev.num_regions].size = size;
+	vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
+	vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
+	vgpu->vdev.num_regions++;
+
+	return 0;
+}
+
+static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
+{
+	unsigned int addr;
+	void *base;
+	int ret;
+
+	addr = vgpu->gvt->opregion.opregion_pa;
+	if (!addr || !(~addr))
+		return -ENODEV;
+
+	base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB);
+	if (!base)
+		return -ENOMEM;
+
+	if (memcmp(base, OPREGION_SIGNATURE, 16)) {
+		memunmap(base);
+		return -EINVAL;
+	}
+
+	ret = intel_vgpu_register_reg(vgpu,
+			PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
+			VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
+			&intel_vgpu_regops_opregion, OPREGION_SIZE,
+			VFIO_REGION_INFO_FLAG_READ, base);
+	if (ret) {
+		memunmap(base);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -467,6 +555,15 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 	vgpu->vdev.mdev = mdev;
 	mdev_set_drvdata(mdev, vgpu);
 
+	ret = intel_vgpu_reg_init_opregion(vgpu);
+	if (ret) {
+		gvt_vgpu_err("create OpRegion failed\n");
+		goto out;
+	}
+
+	gvt_dbg_core("create OpRegion succeeded for mdev:%s\n",
+			dev_name(mdev_dev(mdev)));
+
 	gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
 		     dev_name(mdev_dev(mdev)));
 	ret = 0;
diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 3117991..99591bc 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -283,14 +283,22 @@ int intel_vgpu_emulate_opregion_request(struct intel_vgpu *vgpu, u32 swsci)
 {
 	u32 *scic, *parm;
 	u32 func, subfunc;
+	u32 scic_val, parm_val;
+	u32 gpa;
 
-	scic = vgpu_opregion(vgpu)->va + INTEL_GVT_OPREGION_SCIC;
-	parm = vgpu_opregion(vgpu)->va + INTEL_GVT_OPREGION_PARM;
+	memcpy(&gpa, vgpu_cfg_space(vgpu) + INTEL_GVT_PCI_OPREGION,
+			sizeof(gpa));
+	intel_gvt_hypervisor_read_gpa(vgpu, gpa + INTEL_GVT_OPREGION_SCIC,
+			&scic_val, sizeof(scic_val));
+	intel_gvt_hypervisor_read_gpa(vgpu, gpa + INTEL_GVT_OPREGION_PARM,
+			&parm_val, sizeof(parm_val));
 
 	if (!(swsci & SWSCI_SCI_SELECT)) {
 		gvt_vgpu_err("requesting SMI service\n");
 		return 0;
 	}
+	scic = &scic_val;
+	parm = &parm_val;
 	/* ignore non 0->1 trasitions */
 	if ((vgpu_cfg_space(vgpu)[INTEL_GVT_PCI_SWSCI]
 				& SWSCI_SCI_TRIGGER) ||
-- 
1.9.1

[RFC PATCH 3/6] drm/i915/gvt: framebuffer decoder support for GVT-g

[Xiaoguang Chen]

decode frambuffer attributes of primary, cursor and sprite plane

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/Makefile     |   3 +-
 drivers/gpu/drm/i915/gvt/display.c    |   2 +-
 drivers/gpu/drm/i915/gvt/display.h    |   2 +
 drivers/gpu/drm/i915/gvt/fb_decoder.c | 487 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h | 170 ++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h        |   1 +
 6 files changed, 663 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c
 create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index b123c20..192ca26 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -1,7 +1,8 @@
 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
+	execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \
+	fb_decoder.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/display.c b/drivers/gpu/drm/i915/gvt/display.c
index 4cf2b29..9f8d126 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu)
 	return 1;
 }
 
-static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
+int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
 
diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h
index d73de22..b46b868 100644
--- a/drivers/gpu/drm/i915/gvt/display.h
+++ b/drivers/gpu/drm/i915/gvt/display.h
@@ -179,4 +179,6 @@ static inline char *vgpu_edid_str(enum intel_vgpu_edid id)
 void intel_vgpu_reset_display(struct intel_vgpu *vgpu);
 void intel_vgpu_clean_display(struct intel_vgpu *vgpu);
 
+int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe);
+
 #endif
diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c
new file mode 100644
index 0000000..954f047
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
@@ -0,0 +1,487 @@
+/*
+ * Copyright(c) 2011-2016 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:
+ *    Kevin Tian <kevin.tian@intel.com>
+ *
+ * Contributors:
+ *    Bing Niu <bing.niu@intel.com>
+ *    Xu Han <xu.han@intel.com>
+ *    Ping Gao <ping.a.gao@intel.com>
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Yang Liu <yang2.liu@intel.com>
+ *
+ */
+
+#include <uapi/drm/drm_fourcc.h>
+#include "i915_drv.h"
+#include "gvt.h"
+
+/* The below definitions are required by guest. */
+// [63:0] x:R:G:B 16:16:16:16 little endian
+#define DRM_FORMAT_XRGB161616_GVT  fourcc_code('X', 'R', '4', '8')
+// [63:0] x:B:G:R 16:16:16:16 little endian
+#define DRM_FORMAT_XBGR161616_GVT  fourcc_code('X', 'B', '4', '8')
+
+#define FORMAT_NUM	16
+struct pixel_format {
+	int	drm_format;	/* Pixel format in DRM definition */
+	int	bpp;		/* Bits per pixel, 0 indicates invalid */
+	char *desc;		/* The description */
+};
+
+/* non-supported format has bpp default to 0 */
+static struct pixel_format primary_pixel_formats[FORMAT_NUM] = {
+	[0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"},
+	[0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
+	[0x6] = {DRM_FORMAT_XRGB8888, 32,
+		"32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"},
+	[0x8] = {DRM_FORMAT_XBGR2101010, 32,
+		"32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
+	[0xa] = {DRM_FORMAT_XRGB2101010, 32,
+		"32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+	[0xc] = {DRM_FORMAT_XRGB161616_GVT, 64,
+		"64-bit RGBX Floating Point(16:16:16:16 MSB-X:B:G:R)"},
+	[0xe] = {DRM_FORMAT_XBGR8888, 32,
+		"32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
+};
+
+/* non-supported format has bpp default to 0 */
+static struct pixel_format skl_pixel_formats[] = {
+	{DRM_FORMAT_YUYV, 16, "16-bit packed YUYV (8:8:8:8 MSB-V:Y2:U:Y1)"},
+	{DRM_FORMAT_UYVY, 16, "16-bit packed UYVY (8:8:8:8 MSB-Y2:V:Y1:U)"},
+	{DRM_FORMAT_YVYU, 16, "16-bit packed YVYU (8:8:8:8 MSB-U:Y2:V:Y1)"},
+	{DRM_FORMAT_VYUY, 16, "16-bit packed VYUY (8:8:8:8 MSB-Y2:U:Y1:V)"},
+
+	{DRM_FORMAT_C8, 8, "8-bit Indexed"},
+	{DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"},
+	{DRM_FORMAT_ABGR8888, 32, "32-bit RGBA (8:8:8:8 MSB-A:B:G:R)"},
+	{DRM_FORMAT_XBGR8888, 32, "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"},
+
+	{DRM_FORMAT_ARGB8888, 32, "32-bit BGRA (8:8:8:8 MSB-A:R:G:B)"},
+	{DRM_FORMAT_XRGB8888, 32, "32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"},
+	{DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"},
+	{DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"},
+
+	{DRM_FORMAT_XRGB161616_GVT, 64,
+		"64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"},
+	{DRM_FORMAT_XBGR161616_GVT, 64,
+		"64-bit XBGR (16:16:16:16 MSB-X:B:G:R)"},
+
+	/* non-supported format has bpp default to 0 */
+	{0, 0, NULL},
+};
+
+static int skl_format_to_drm(int format, bool rgb_order, bool alpha,
+	int yuv_order)
+{
+	int skl_pixel_formats_index = 14;
+
+	switch (format) {
+	case PLANE_CTL_FORMAT_INDEXED:
+		skl_pixel_formats_index = 4;
+		break;
+	case PLANE_CTL_FORMAT_RGB_565:
+		skl_pixel_formats_index = 5;
+		break;
+	case PLANE_CTL_FORMAT_XRGB_8888:
+		if (rgb_order)
+			skl_pixel_formats_index = alpha ? 6 : 7;
+		else
+			skl_pixel_formats_index = alpha ? 8 : 9;
+		break;
+	case PLANE_CTL_FORMAT_XRGB_2101010:
+		skl_pixel_formats_index = rgb_order ? 10 : 11;
+		break;
+
+	case PLANE_CTL_FORMAT_XRGB_16161616F:
+		skl_pixel_formats_index = rgb_order ? 12 : 13;
+		break;
+
+	case PLANE_CTL_FORMAT_YUV422:
+		skl_pixel_formats_index = yuv_order >> 16;
+		if (skl_pixel_formats_index > 3)
+			return -EINVAL;
+		break;
+
+	default:
+		break;
+	}
+
+	return skl_pixel_formats_index;
+}
+
+static u32 intel_vgpu_get_stride(struct intel_vgpu *vgpu, int pipe,
+	u32 tiled, int stride_mask, int bpp)
+{
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+
+	u32 stride_reg = vgpu_vreg(vgpu, DSPSTRIDE(pipe)) & stride_mask;
+	u32 stride = stride_reg;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		switch (tiled) {
+		case PLANE_CTL_TILED_LINEAR:
+			stride = stride_reg * 64;
+			break;
+		case PLANE_CTL_TILED_X:
+			stride = stride_reg * 512;
+			break;
+		case PLANE_CTL_TILED_Y:
+			stride = stride_reg * 128;
+			break;
+		case PLANE_CTL_TILED_YF:
+			if (bpp == 8)
+				stride = stride_reg * 64;
+			else if (bpp == 16 || bpp == 32 || bpp == 64)
+				stride = stride_reg * 128;
+			else
+				gvt_dbg_core("skl: unsupported bpp:%d\n", bpp);
+			break;
+		default:
+			gvt_dbg_core("skl: unsupported tile format:%x\n",
+				tiled);
+		}
+	}
+
+	return stride;
+}
+
+static int intel_vgpu_decode_primary_plane_format(struct intel_vgpu *vgpu,
+	int pipe, struct intel_vgpu_primary_plane_format *plane)
+{
+	u32	val, fmt;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+
+	val = vgpu_vreg(vgpu, DSPCNTR(pipe));
+	plane->enabled = !!(val & DISPLAY_PLANE_ENABLE);
+	if (!plane->enabled)
+		return 0;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		plane->tiled = (val & PLANE_CTL_TILED_MASK) >>
+		_PLANE_CTL_TILED_SHIFT;
+		fmt = skl_format_to_drm(
+			val & PLANE_CTL_FORMAT_MASK,
+			val & PLANE_CTL_ORDER_RGBX,
+			val & PLANE_CTL_ALPHA_MASK,
+			val & PLANE_CTL_YUV422_ORDER_MASK);
+		plane->bpp = skl_pixel_formats[fmt].bpp;
+		plane->drm_format = skl_pixel_formats[fmt].drm_format;
+	} else {
+		plane->tiled = !!(val & DISPPLANE_TILED);
+		fmt = (val & DISPPLANE_PIXFORMAT_MASK) >> _PRI_PLANE_FMT_SHIFT;
+		plane->bpp = primary_pixel_formats[fmt].bpp;
+		plane->drm_format = primary_pixel_formats[fmt].drm_format;
+	}
+
+	if ((IS_SKYLAKE(dev_priv) && !skl_pixel_formats[fmt].bpp)
+		|| (!IS_SKYLAKE(dev_priv) &&
+			!primary_pixel_formats[fmt].bpp)) {
+		gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
+		return -EINVAL;
+	}
+
+	plane->hw_format = fmt;
+
+	plane->base = vgpu_vreg(vgpu, DSPSURF(pipe)) & GTT_PAGE_MASK;
+
+	plane->stride = intel_vgpu_get_stride(vgpu, pipe, (plane->tiled << 10),
+		(IS_SKYLAKE(dev_priv)) ? (_PRI_PLANE_STRIDE_MASK >> 6) :
+		_PRI_PLANE_STRIDE_MASK, plane->bpp);
+
+	plane->width = (vgpu_vreg(vgpu, PIPESRC(pipe)) & _PIPE_H_SRCSZ_MASK) >>
+		_PIPE_H_SRCSZ_SHIFT;
+	plane->width += 1;
+	plane->height = (vgpu_vreg(vgpu, PIPESRC(pipe)) &
+			_PIPE_V_SRCSZ_MASK) >> _PIPE_V_SRCSZ_SHIFT;
+	plane->height += 1;	/* raw height is one minus the real value */
+
+	val = vgpu_vreg(vgpu, DSPTILEOFF(pipe));
+	plane->x_offset = (val & _PRI_PLANE_X_OFF_MASK) >>
+		_PRI_PLANE_X_OFF_SHIFT;
+	plane->y_offset = (val & _PRI_PLANE_Y_OFF_MASK) >>
+		_PRI_PLANE_Y_OFF_SHIFT;
+
+	return 0;
+}
+
+#define CURSOR_MODE_NUM	(1 << 6)
+struct cursor_mode_format {
+	int	drm_format;	/* Pixel format in DRM definition */
+	u8	bpp;		/* Bits per pixel; 0 indicates invalid */
+	u32	width;		/* In pixel */
+	u32	height;		/* In lines */
+	char	*desc;		/* The description */
+};
+
+/* non-supported format has bpp default to 0 */
+static struct cursor_mode_format cursor_pixel_formats[CURSOR_MODE_NUM] = {
+	[0x22]  = {DRM_FORMAT_ARGB8888, 32, 128, 128, "128x128 32bpp ARGB"},
+	[0x23]  = {DRM_FORMAT_ARGB8888, 32, 256, 256, "256x256 32bpp ARGB"},
+	[0x27]  = {DRM_FORMAT_ARGB8888, 32, 64, 64, "64x64 32bpp ARGB"},
+	[0x7]  = {DRM_FORMAT_ARGB8888, 32, 64, 64, "64x64 32bpp ARGB"},
+};
+
+static int intel_vgpu_decode_cursor_plane_format(struct intel_vgpu *vgpu,
+	int pipe, struct intel_vgpu_cursor_plane_format *plane)
+{
+	u32 val, mode;
+	u32 alpha_plane, alpha_force;
+	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+
+	val = vgpu_vreg(vgpu, CURCNTR(pipe));
+	mode = val & CURSOR_MODE;
+	plane->enabled = (mode != CURSOR_MODE_DISABLE);
+	if (!plane->enabled)
+		return 0;
+
+	if (!cursor_pixel_formats[mode].bpp) {
+		gvt_vgpu_err("Non-supported cursor mode (0x%x)\n", mode);
+		return -EINVAL;
+	}
+	plane->mode = mode;
+	plane->bpp = cursor_pixel_formats[mode].bpp;
+	plane->drm_format = cursor_pixel_formats[mode].drm_format;
+	plane->width = cursor_pixel_formats[mode].width;
+	plane->height = cursor_pixel_formats[mode].height;
+
+	alpha_plane = (val & _CURSOR_ALPHA_PLANE_MASK) >>
+				_CURSOR_ALPHA_PLANE_SHIFT;
+	alpha_force = (val & _CURSOR_ALPHA_FORCE_MASK) >>
+				_CURSOR_ALPHA_FORCE_SHIFT;
+	if (alpha_plane || alpha_force)
+		gvt_dbg_core("alpha_plane=0x%x, alpha_force=0x%x\n",
+			alpha_plane, alpha_force);
+
+	plane->base = vgpu_vreg(vgpu, CURBASE(pipe)) & GTT_PAGE_MASK;
+
+	val = vgpu_vreg(vgpu, CURPOS(pipe));
+	plane->x_pos = (val & _CURSOR_POS_X_MASK) >> _CURSOR_POS_X_SHIFT;
+	plane->x_sign = (val & _CURSOR_SIGN_X_MASK) >> _CURSOR_SIGN_X_SHIFT;
+	plane->y_pos = (val & _CURSOR_POS_Y_MASK) >> _CURSOR_POS_Y_SHIFT;
+	plane->y_sign = (val & _CURSOR_SIGN_Y_MASK) >> _CURSOR_SIGN_Y_SHIFT;
+
+	return 0;
+}
+
+#define FORMAT_NUM_SRRITE	(1 << 3)
+
+static struct pixel_format sprite_pixel_formats[FORMAT_NUM_SRRITE] = {
+	[0x0]  = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"},
+	[0x1]  = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"},
+	[0x2]  = {DRM_FORMAT_XRGB8888, 32, "RGB 32-bit 8:8:8:8"},
+	[0x3]  = {DRM_FORMAT_XRGB161616_GVT, 64,
+		    "RGB 64-bit 16:16:16:16 Floating Point"},
+	[0x4] = {DRM_FORMAT_AYUV, 32,
+		"YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"},
+};
+
+static int intel_vgpu_decode_sprite_plane_format(struct intel_vgpu *vgpu,
+	int pipe, struct intel_vgpu_sprite_plane_format *plane)
+{
+	u32 val, fmt;
+	u32 width;
+	u32 color_order, yuv_order;
+	int drm_format;
+
+	val = vgpu_vreg(vgpu, SPRCTL(pipe));
+	plane->enabled = !!(val & SPRITE_ENABLE);
+	if (!plane->enabled)
+		return 0;
+
+	plane->tiled = !!(val & SPRITE_TILED);
+	color_order = !!(val & SPRITE_RGB_ORDER_RGBX);
+	yuv_order = (val & SPRITE_YUV_BYTE_ORDER_MASK) >>
+				_SPRITE_YUV_ORDER_SHIFT;
+
+	fmt = (val & SPRITE_PIXFORMAT_MASK) >> _SPRITE_FMT_SHIFT;
+	if (!sprite_pixel_formats[fmt].bpp) {
+		gvt_vgpu_err("Non-supported pixel format (0x%x)\n", fmt);
+		return -EINVAL;
+	}
+	plane->hw_format = fmt;
+	plane->bpp = sprite_pixel_formats[fmt].bpp;
+	drm_format = sprite_pixel_formats[fmt].drm_format;
+
+	/* Order of RGB values in an RGBxxx buffer may be ordered RGB or
+	 * BGR depending on the state of the color_order field
+	 */
+	if (!color_order) {
+		if (drm_format == DRM_FORMAT_XRGB2101010)
+			drm_format = DRM_FORMAT_XBGR2101010;
+		else if (drm_format == DRM_FORMAT_XRGB8888)
+			drm_format = DRM_FORMAT_XBGR8888;
+	}
+
+	if (drm_format == DRM_FORMAT_YUV422) {
+		switch (yuv_order) {
+		case	0:
+			drm_format = DRM_FORMAT_YUYV;
+			break;
+		case	1:
+			drm_format = DRM_FORMAT_UYVY;
+			break;
+		case	2:
+			drm_format = DRM_FORMAT_YVYU;
+			break;
+		case	3:
+			drm_format = DRM_FORMAT_VYUY;
+			break;
+		default:
+			/* yuv_order has only 2 bits */
+			break;
+		}
+	}
+
+	plane->drm_format = drm_format;
+
+	plane->base = vgpu_vreg(vgpu, SPRSURF(pipe)) & GTT_PAGE_MASK;
+	plane->width = vgpu_vreg(vgpu, SPRSTRIDE(pipe)) &
+				_SPRITE_STRIDE_MASK;
+	plane->width /= plane->bpp / 8;	/* raw width in bytes */
+
+	val = vgpu_vreg(vgpu, SPRSIZE(pipe));
+	plane->height = (val & _SPRITE_SIZE_HEIGHT_MASK) >>
+		_SPRITE_SIZE_HEIGHT_SHIFT;
+	width = (val & _SPRITE_SIZE_WIDTH_MASK) >> _SPRITE_SIZE_WIDTH_SHIFT;
+	plane->height += 1;	/* raw height is one minus the real value */
+	width += 1;		/* raw width is one minus the real value */
+	if (plane->width != width)
+		gvt_dbg_core("sprite_plane: plane->width=%d, width=%d\n",
+			plane->width, width);
+
+	val = vgpu_vreg(vgpu, SPRPOS(pipe));
+	plane->x_pos = (val & _SPRITE_POS_X_MASK) >> _SPRITE_POS_X_SHIFT;
+	plane->y_pos = (val & _SPRITE_POS_Y_MASK) >> _SPRITE_POS_Y_SHIFT;
+
+	val = vgpu_vreg(vgpu, SPROFFSET(pipe));
+	plane->x_offset = (val & _SPRITE_OFFSET_START_X_MASK) >>
+			   _SPRITE_OFFSET_START_X_SHIFT;
+	plane->y_offset = (val & _SPRITE_OFFSET_START_Y_MASK) >>
+			   _SPRITE_OFFSET_START_Y_SHIFT;
+	return 0;
+}
+
+static int intel_vgpu_decode_fb_format(struct intel_gvt *gvt, int id,
+	struct intel_vgpu_fb_format *fb)
+{
+	int i;
+	struct intel_vgpu *vgpu = NULL;
+	int ret = 0;
+	struct drm_i915_private *dev_priv = gvt->dev_priv;
+
+	if (!fb)
+		return -EINVAL;
+
+	/* TODO: use fine-grained refcnt later */
+	mutex_lock(&gvt->lock);
+
+	for_each_active_vgpu(gvt, vgpu, i)
+		if (vgpu->id == id)
+			break;
+
+	if (!vgpu) {
+		gvt_vgpu_err("Invalid vgpu ID (%d)\n", id);
+		mutex_unlock(&gvt->lock);
+		return -ENODEV;
+	}
+
+	for (i = 0; i < I915_MAX_PIPES; i++) {
+		struct intel_vgpu_pipe_format *pipe = &fb->pipes[i];
+		u32 ddi_func_ctl = vgpu_vreg(vgpu, TRANS_DDI_FUNC_CTL(i));
+
+		if (!(ddi_func_ctl & TRANS_DDI_FUNC_ENABLE)) {
+			pipe->ddi_port = DDI_PORT_NONE;
+		} else {
+			u32 port = (ddi_func_ctl & TRANS_DDI_PORT_MASK) >>
+						TRANS_DDI_PORT_SHIFT;
+			if (port <= DDI_PORT_E)
+				pipe->ddi_port = port;
+			else
+				pipe->ddi_port = DDI_PORT_NONE;
+		}
+
+		ret |= intel_vgpu_decode_primary_plane_format(vgpu,
+			i, &pipe->primary);
+		ret |= intel_vgpu_decode_sprite_plane_format(vgpu,
+			i, &pipe->sprite);
+		ret |= intel_vgpu_decode_cursor_plane_format(vgpu,
+			i, &pipe->cursor);
+
+		if (ret) {
+			gvt_vgpu_err("Decode format error for pipe(%d)\n", i);
+			ret = -EINVAL;
+			break;
+		}
+	}
+
+	mutex_unlock(&gvt->lock);
+
+	return ret;
+}
+
+/**
+ * intel_vgpu_decode_plane - Decode plane based on plane id
+ * @dev: drm device
+ * @vgpu: input vgpu
+ * @plane_id: plane id to be decoded
+ * This function is called for decoding plane
+ *
+ * Returns:
+ * decoded plane on success, NULL if failed.
+ */
+void *intel_vgpu_decode_plane(struct drm_device *dev,
+		struct intel_vgpu *vgpu, u32 plane_id)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_vgpu_fb_format fb;
+	struct intel_vgpu_pipe_format *pipe;
+	int i;
+
+	if (intel_vgpu_decode_fb_format(dev_priv->gvt, vgpu->id, &fb))
+		return NULL;
+
+	for (i = 0; i < I915_MAX_PIPES; i++)
+		if (pipe_is_enabled(vgpu, i))
+			break;
+	if (i >= I915_MAX_PIPES) {
+		gvt_dbg_core("No enabled pipes\n");
+		return NULL;
+	}
+	pipe = &fb.pipes[i];
+
+	if (!pipe || !pipe->primary.enabled) {
+		gvt_dbg_core("Invalid pipe_id :%d\n", i);
+		return NULL;
+	}
+	if (plane_id == INTEL_GVT_PLANE_PRIMARY)
+		return  &pipe->primary;
+
+	if (plane_id == INTEL_GVT_PLANE_CURSOR)
+		return &pipe->cursor;
+
+	gvt_dbg_core("Invalid plane_id: %d\n", plane_id);
+
+	return NULL;
+}
+
diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.h b/drivers/gpu/drm/i915/gvt/fb_decoder.h
new file mode 100644
index 0000000..412bd5b
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/fb_decoder.h
@@ -0,0 +1,170 @@
+/*
+ * Copyright(c) 2011-2016 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:
+ *    Kevin Tian <kevin.tian@intel.com>
+ *
+ * Contributors:
+ *    Bing Niu <bing.niu@intel.com>
+ *    Xu Han <xu.han@intel.com>
+ *    Ping Gao <ping.a.gao@intel.com>
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ *    Yang Liu <yang2.liu@intel.com>
+ *
+ */
+
+#ifndef _GVT_FB_DECODER_H_
+#define _GVT_FB_DECODER_H_
+
+#define _PLANE_CTL_FORMAT_SHIFT		24
+#define _PLANE_CTL_TILED_SHIFT		10
+#define _PIPE_V_SRCSZ_SHIFT		0
+#define _PIPE_V_SRCSZ_MASK		(0xfff << _PIPE_V_SRCSZ_SHIFT)
+#define _PIPE_H_SRCSZ_SHIFT		16
+#define _PIPE_H_SRCSZ_MASK		(0x1fff << _PIPE_H_SRCSZ_SHIFT)
+
+#define _PRI_PLANE_FMT_SHIFT		26
+#define _PRI_PLANE_STRIDE_MASK		(0x3ff << 6)
+#define _PRI_PLANE_X_OFF_SHIFT		0
+#define _PRI_PLANE_X_OFF_MASK		(0x1fff << _PRI_PLANE_X_OFF_SHIFT)
+#define _PRI_PLANE_Y_OFF_SHIFT		16
+#define _PRI_PLANE_Y_OFF_MASK		(0xfff << _PRI_PLANE_Y_OFF_SHIFT)
+
+#define _CURSOR_MODE			0x3f
+#define _CURSOR_ALPHA_FORCE_SHIFT	8
+#define _CURSOR_ALPHA_FORCE_MASK	(0x3 << _CURSOR_ALPHA_FORCE_SHIFT)
+#define _CURSOR_ALPHA_PLANE_SHIFT	10
+#define _CURSOR_ALPHA_PLANE_MASK	(0x3 << _CURSOR_ALPHA_PLANE_SHIFT)
+#define _CURSOR_POS_X_SHIFT		0
+#define _CURSOR_POS_X_MASK		(0x1fff << _CURSOR_POS_X_SHIFT)
+#define _CURSOR_SIGN_X_SHIFT		15
+#define _CURSOR_SIGN_X_MASK		(1 << _CURSOR_SIGN_X_SHIFT)
+#define _CURSOR_POS_Y_SHIFT		16
+#define _CURSOR_POS_Y_MASK		(0xfff << _CURSOR_POS_Y_SHIFT)
+#define _CURSOR_SIGN_Y_SHIFT		31
+#define _CURSOR_SIGN_Y_MASK		(1 << _CURSOR_SIGN_Y_SHIFT)
+
+#define _SPRITE_FMT_SHIFT		25
+#define _SPRITE_COLOR_ORDER_SHIFT	20
+#define _SPRITE_YUV_ORDER_SHIFT		16
+#define _SPRITE_STRIDE_SHIFT		6
+#define _SPRITE_STRIDE_MASK		(0x1ff << _SPRITE_STRIDE_SHIFT)
+#define _SPRITE_SIZE_WIDTH_SHIFT	0
+#define _SPRITE_SIZE_HEIGHT_SHIFT	16
+#define _SPRITE_SIZE_WIDTH_MASK		(0x1fff << _SPRITE_SIZE_WIDTH_SHIFT)
+#define _SPRITE_SIZE_HEIGHT_MASK	(0xfff << _SPRITE_SIZE_HEIGHT_SHIFT)
+#define _SPRITE_POS_X_SHIFT		0
+#define _SPRITE_POS_Y_SHIFT		16
+#define _SPRITE_POS_X_MASK		(0x1fff << _SPRITE_POS_X_SHIFT)
+#define _SPRITE_POS_Y_MASK		(0xfff << _SPRITE_POS_Y_SHIFT)
+#define _SPRITE_OFFSET_START_X_SHIFT	0
+#define _SPRITE_OFFSET_START_Y_SHIFT	16
+#define _SPRITE_OFFSET_START_X_MASK	(0x1fff << _SPRITE_OFFSET_START_X_SHIFT)
+#define _SPRITE_OFFSET_START_Y_MASK	(0xfff << _SPRITE_OFFSET_START_Y_SHIFT)
+
+#define INTEL_GVT_PLANE_PRIMARY		1
+#define INTEL_GVT_PLANE_SPRITE		2
+#define INTEL_GVT_PLANE_CURSOR		3
+
+typedef enum {
+	FB_MODE_SET_START = 1,
+	FB_MODE_SET_END,
+	FB_DISPLAY_FLIP,
+} gvt_fb_event_t;
+
+typedef enum {
+	DDI_PORT_NONE	= 0,
+	DDI_PORT_B	= 1,
+	DDI_PORT_C	= 2,
+	DDI_PORT_D	= 3,
+	DDI_PORT_E	= 4
+} ddi_port_t;
+
+struct intel_gvt;
+
+struct gvt_fb_notify_msg {
+	unsigned int vm_id;
+	unsigned int pipe_id; /* id starting from 0 */
+	unsigned int plane_id; /* primary, cursor, or sprite */
+};
+
+/* color space conversion and gamma correction are not included */
+struct intel_vgpu_primary_plane_format {
+	u8	enabled;	/* plane is enabled */
+	u8	tiled;		/* X-tiled */
+	u8	bpp;		/* bits per pixel */
+	u32	hw_format;	/* format field in the PRI_CTL register */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* framebuffer base in graphics memory */
+	u32	x_offset;	/* in pixels */
+	u32	y_offset;	/* in lines */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	stride;		/* in bytes */
+};
+
+struct intel_vgpu_sprite_plane_format {
+	u8	enabled;	/* plane is enabled */
+	u8	tiled;		/* X-tiled */
+	u8	bpp;		/* bits per pixel */
+	u32	hw_format;	/* format field in the SPR_CTL register */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* sprite base in graphics memory */
+	u32	x_pos;		/* in pixels */
+	u32	y_pos;		/* in lines */
+	u32	x_offset;	/* in pixels */
+	u32	y_offset;	/* in lines */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+};
+
+struct intel_vgpu_cursor_plane_format {
+	u8	enabled;
+	u8	mode;		/* cursor mode select */
+	u8	bpp;		/* bits per pixel */
+	u32	drm_format;	/* format in DRM definition */
+	u32	base;		/* cursor base in graphics memory */
+	u32	x_pos;		/* in pixels */
+	u32	y_pos;		/* in lines */
+	u8	x_sign;		/* X Position Sign */
+	u8	y_sign;		/* Y Position Sign */
+	u32	width;		/* in pixels */
+	u32	height;		/* in lines */
+	u32	x_hot;		/* in pixels */
+	u32	y_hot;		/* in pixels */
+};
+
+struct intel_vgpu_pipe_format {
+	struct intel_vgpu_primary_plane_format	primary;
+	struct intel_vgpu_sprite_plane_format	sprite;
+	struct intel_vgpu_cursor_plane_format	cursor;
+	ddi_port_t ddi_port;  /* the DDI port that the pipe is connected to */
+};
+
+struct intel_vgpu_fb_format {
+	struct intel_vgpu_pipe_format	pipes[4];
+};
+
+void *intel_vgpu_decode_plane(struct drm_device *dev,
+                struct intel_vgpu *vgpu, u32 plane_id);
+
+#endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 930732e..c42266c 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -46,6 +46,7 @@
 #include "sched_policy.h"
 #include "render.h"
 #include "cmd_parser.h"
+#include "fb_decoder.h"
 
 #define GVT_MAX_VGPU 8
 
-- 
1.9.1

[RFC PATCH 4/6] drm/i915: export i915 dmabuf_ops

[Xiaoguang Chen]

GVT-g will use i915's dmabuf_ops to implement its own dmabuf so
exporting it.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        | 2 ++
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bb6fc1e..470d461 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3476,6 +3476,8 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 				struct drm_gem_object *gem_obj, int flags);
 
+extern const struct dma_buf_ops i915_dmabuf_ops;
+
 static inline struct i915_hw_ppgtt *
 i915_vm_to_ppgtt(struct i915_address_space *vm)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 11898cd..4dafc99 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -196,7 +196,7 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct
 	return err;
 }
 
-static const struct dma_buf_ops i915_dmabuf_ops =  {
+const struct dma_buf_ops i915_dmabuf_ops =  {
 	.map_dma_buf = i915_gem_map_dma_buf,
 	.unmap_dma_buf = i915_gem_unmap_dma_buf,
 	.release = drm_gem_dmabuf_release,
-- 
1.9.1

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

[Xiaoguang Chen]

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@intel.com>
---
 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@intel.com>
+ *
+ * Contributors:
+ *    Xiaoguang Chen <xiaoguang.chen@intel.com>
+ */
+
+#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.
+	 */
+	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;
+	}
+
+	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;
+	}
+	obj->cache_level = I915_CACHE_L3_LLC;
+	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;
+	} else {
+		obj->tiling_and_stride = (info->tiled ? I915_TILING_X :
+			I915_TILING_NONE) | (info->tiled ? info->stride : 0);
+	}
+
+	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;
+	}
+	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;
+	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 {
+	__u32 plane_id;
+	/* out */
+	__u32 fd;
+	__u32 drm_format;
+	__u32 width;
+	__u32 height;
+	__u32 stride;
+	__u32 start;
+	__u32 x_pos;
+	__u32 y_pos;
+	__u32 size;
+	__u32 tiled;
+};
+
+int intel_vgpu_query_dmabuf(struct intel_vgpu *vgpu, void *args);
+int intel_vgpu_generate_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
 
-- 
1.9.1

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

[Chris Wilson]

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@intel.com>
> ---
>  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@intel.com>
> + *
> + * Contributors:
> + *    Xiaoguang Chen <xiaoguang.chen@intel.com>
> + */
> +
> +#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).

> +	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.

> +	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.

> +	obj->cache_level = I915_CACHE_L3_LLC;

Are you sure?


> +	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?

> +	} 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.

> +	}
> +
> +	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.

> +	}
> +	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);

> +	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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chen, Xiaoguang]

>-----Original Message-----
>From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
>Sent: Friday, April 28, 2017 6:09 PM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: kraxel@redhat.com; alex.williamson@redhat.com; intel-
>gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
><zhi.a.wang@intel.com>; zhenyuw@linux.intel.com; linux-
>kernel@vger.kernel.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Tian, Kevin
><kevin.tian@intel.com>
>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@intel.com>
>> ---
>>  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@intel.com>
>> + *
>> + * Contributors:
>> + *    Xiaoguang Chen <xiaoguang.chen@intel.com>
>> + */
>> +
>> +#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

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

[Chen, Xiaoguang]

Hi Chis, do you have any comments for this problem?

>> +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;
>> +}

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Chen, Xiaoguang
>Sent: Tuesday, May 02, 2017 3:40 PM
>To: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; alex.williamson@redhat.com;
>kraxel@redhat.com; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
><zhi.a.wang@intel.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>
>Subject: RE: [Intel-gfx] [RFC PATCH 5/6] drm/i915/gvt: dmabuf support for GVT-g
>
>
>
>>-----Original Message-----
>>From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
>>Sent: Friday, April 28, 2017 6:09 PM
>>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>>Cc: kraxel@redhat.com; alex.williamson@redhat.com; intel-
>>gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; Wang,
>>Zhi A <zhi.a.wang@intel.com>; zhenyuw@linux.intel.com; linux-
>>kernel@vger.kernel.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Tian, Kevin
>><kevin.tian@intel.com>
>>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@intel.com>
>>> ---
>>>  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@intel.com>
>>> + *
>>> + * Contributors:
>>> + *    Xiaoguang Chen <xiaoguang.chen@intel.com>
>>> + */
>>> +
>>> +#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
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

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

[Gerd Hoffmann]

  Hi,

> > +#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?

It is indeed, should go to include/uapi/

cheers,
  Gerd

[RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Xiaoguang Chen]

GVT-g will create an anonymous fd and a vfio device region to deliver
the fd to QEMU.
QEMU can do ioctl using this fd to query/generate dmabuf on an intel vgpu.

Signed-off-by: Xiaoguang Chen <xiaoguang.chen@intel.com>
---
 drivers/gpu/drm/i915/gvt/gvt.c   |   2 +
 drivers/gpu/drm/i915/gvt/gvt.h   |   2 +
 drivers/gpu/drm/i915/gvt/kvmgt.c | 109 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h        |   1 +
 4 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index 7dea5e5..c266d31 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -54,6 +54,8 @@
 	.vgpu_reset = intel_gvt_reset_vgpu,
 	.vgpu_activate = intel_gvt_activate_vgpu,
 	.vgpu_deactivate = intel_gvt_deactivate_vgpu,
+	.vgpu_query_dmabuf = intel_vgpu_query_dmabuf,
+	.vgpu_generate_dmabuf = intel_vgpu_generate_dmabuf,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 763a8c5..2733a69 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -467,6 +467,8 @@ struct intel_gvt_ops {
 	void (*vgpu_reset)(struct intel_vgpu *);
 	void (*vgpu_activate)(struct intel_vgpu *);
 	void (*vgpu_deactivate)(struct intel_vgpu *);
+	int (*vgpu_query_dmabuf)(struct intel_vgpu *, void *);
+	int (*vgpu_generate_dmabuf)(struct intel_vgpu *, void *);
 };
 
 
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 389f072..beb5356 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -41,6 +41,7 @@
 #include <linux/kvm_host.h>
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/anon_inodes.h>
 
 #include "i915_drv.h"
 #include "gvt.h"
@@ -524,6 +525,106 @@ static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
 	return ret;
 }
 
+static int intel_vgpu_gvtg_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	WARN_ON(1);
+
+	return 0;
+}
+
+static int intel_vgpu_gvtg_release(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static long intel_vgpu_gvtg_ioctl(struct file *filp,
+		unsigned int ioctl, unsigned long arg)
+{
+	struct intel_vgpu *vgpu = filp->private_data;
+	int minsz;
+	struct intel_vgpu_dmabuf dmabuf;
+	int ret;
+
+	minsz = offsetofend(struct intel_vgpu_dmabuf, y_pos);
+	if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+		return -EFAULT;
+	if (ioctl == INTEL_VGPU_QUERY_DMABUF)
+		ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
+	else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
+		ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu, &dmabuf);
+	else {
+		gvt_vgpu_err("unsupported dmabuf operation\n");
+		return -EINVAL;
+	}
+
+	if (ret != 0) {
+		gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
+		return -EINVAL;
+	}
+
+	return copy_to_user((void __user *)arg, &dmabuf, minsz) ? -EFAULT : 0;
+}
+
+static const struct file_operations intel_vgpu_gvtg_ops = {
+	.release        = intel_vgpu_gvtg_release,
+	.unlocked_ioctl = intel_vgpu_gvtg_ioctl,
+	.mmap           = intel_vgpu_gvtg_mmap,
+	.llseek         = noop_llseek,
+};
+
+static size_t intel_vgpu_reg_rw_gvtg(struct intel_vgpu *vgpu, char *buf,
+		size_t count, loff_t *ppos, bool iswrite)
+{
+	unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
+			VFIO_PCI_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
+	int fd;
+
+	if (pos >= vgpu->vdev.region[i].size || iswrite) {
+		gvt_vgpu_err("invalid op or offset for Intel vgpu fd region\n");
+		return -EINVAL;
+	}
+
+	fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
+			O_RDWR | O_CLOEXEC);
+	if (fd < 0) {
+		gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
+		return -EINVAL;
+	}
+
+	count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
+	memcpy(buf, &fd, count);
+
+	return count;
+}
+
+static void intel_vgpu_reg_release_gvtg(struct intel_vgpu *vgpu,
+		struct vfio_region *region)
+{
+}
+
+static const struct intel_vgpu_regops intel_vgpu_regops_gvtg = {
+	.rw = intel_vgpu_reg_rw_gvtg,
+	.release = intel_vgpu_reg_release_gvtg,
+};
+
+static int intel_vgpu_reg_init_gvtg(struct intel_vgpu *vgpu)
+{
+	int ret;
+
+	ret = intel_vgpu_register_reg(vgpu,
+		PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
+		VFIO_REGION_SUBTYPE_INTEL_IGD_GVTG,
+		&intel_vgpu_regops_gvtg, sizeof(int),
+		VFIO_REGION_INFO_FLAG_READ, NULL);
+	if (ret) {
+		gvt_vgpu_err("failed to register gvtg region:%d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
 	struct intel_vgpu *vgpu = NULL;
@@ -564,6 +665,14 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 	gvt_dbg_core("create OpRegion succeeded for mdev:%s\n",
 			dev_name(mdev_dev(mdev)));
 
+	ret = intel_vgpu_reg_init_gvtg(vgpu);
+	if (ret) {
+		gvt_vgpu_err("create gvtg region failed\n");
+		goto out;
+	}
+	gvt_dbg_core("create gvtg region succeeded for mdev:%s\n",
+			dev_name(mdev_dev(mdev)));
+
 	gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
 		     dev_name(mdev_dev(mdev)));
 	ret = 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 519eff3..96d2c58 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -297,6 +297,7 @@ struct vfio_region_info_cap_type {
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION	(1)
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
+#define VFIO_REGION_SUBTYPE_INTEL_IGD_GVTG	(4)
 
 /**
  * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
-- 
1.9.1

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:
> +static size_t intel_vgpu_reg_rw_gvtg(struct intel_vgpu *vgpu, char
> *buf,
> +               size_t count, loff_t *ppos, bool iswrite)
> +{
> +       unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
> +                       VFIO_PCI_NUM_REGIONS;
> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +       int fd;
> +
> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
> region\n");
> +               return -EINVAL;
> +       }
> +
> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
> +                       O_RDWR | O_CLOEXEC);
> +       if (fd < 0) {
> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
> +               return -EINVAL;
> +       }
> +
> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
> +       memcpy(buf, &fd, count);
> +
> +       return count;
> +}

Hmm, that looks like a rather strange way to return a file descriptor.

What is the reason to not use ioctls on the vfio file handle, like older
version of these patches did?

cheers,
  Gerd

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

>-----Original Message-----
>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>Sent: Tuesday, May 02, 2017 5:51 PM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: alex.williamson@redhat.com; intel-gfx@lists.freedesktop.org; intel-gvt-
>dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>;
>zhenyuw@linux.intel.com; linux-kernel@vger.kernel.org; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; Tian, Kevin <kevin.tian@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:
>> +static size_t intel_vgpu_reg_rw_gvtg(struct intel_vgpu *vgpu, char
>> *buf,
>> +               size_t count, loff_t *ppos, bool iswrite) {
>> +       unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
>> +                       VFIO_PCI_NUM_REGIONS;
>> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>> +       int fd;
>> +
>> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
>> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
>> region\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
>> +                       O_RDWR | O_CLOEXEC);
>> +       if (fd < 0) {
>> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
>> +               return -EINVAL;
>> +       }
>> +
>> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
>> +       memcpy(buf, &fd, count);
>> +
>> +       return count;
>> +}
>
>Hmm, that looks like a rather strange way to return a file descriptor.
>
>What is the reason to not use ioctls on the vfio file handle, like older version of
>these patches did?
If I understood correctly that Alex prefer not to change the ioctls on the vfio file handle like the old version.
So I used this way the smallest change to general vfio framework only adding a subregion definition.

>
>cheers,
>  Gerd

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Alex, do you have any comments for this interface?

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Chen, Xiaoguang
>Sent: Wednesday, May 03, 2017 9:39 AM
>To: Gerd Hoffmann <kraxel@redhat.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; alex.williamson@redhat.com;
>Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
>Zhi A <zhi.a.wang@intel.com>
>Subject: RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>
>
>>-----Original Message-----
>>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>>Sent: Tuesday, May 02, 2017 5:51 PM
>>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>>Cc: alex.williamson@redhat.com; intel-gfx@lists.freedesktop.org;
>>intel-gvt- dev@lists.freedesktop.org; Wang, Zhi A
>><zhi.a.wang@intel.com>; zhenyuw@linux.intel.com;
>>linux-kernel@vger.kernel.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Tian,
>>Kevin <kevin.tian@intel.com>
>>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
>>dmabuf
>>
>>On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:
>>> +static size_t intel_vgpu_reg_rw_gvtg(struct intel_vgpu *vgpu, char
>>> *buf,
>>> +               size_t count, loff_t *ppos, bool iswrite) {
>>> +       unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
>>> +                       VFIO_PCI_NUM_REGIONS;
>>> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>>> +       int fd;
>>> +
>>> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
>>> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
>>> region\n");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
>>> +                       O_RDWR | O_CLOEXEC);
>>> +       if (fd < 0) {
>>> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
>>> +       memcpy(buf, &fd, count);
>>> +
>>> +       return count;
>>> +}
>>
>>Hmm, that looks like a rather strange way to return a file descriptor.
>>
>>What is the reason to not use ioctls on the vfio file handle, like
>>older version of these patches did?
>If I understood correctly that Alex prefer not to change the ioctls on the vfio file
>handle like the old version.
>So I used this way the smallest change to general vfio framework only adding a
>subregion definition.
>
>>
>>cheers,
>>  Gerd
>
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Thu, 4 May 2017 03:09:40 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Alex, do you have any comments for this interface?
> 
> >-----Original Message-----
> >From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> >Behalf Of Chen, Xiaoguang
> >Sent: Wednesday, May 03, 2017 9:39 AM
> >To: Gerd Hoffmann <kraxel@redhat.com>
> >Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; alex.williamson@redhat.com;
> >Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> >Zhi A <zhi.a.wang@intel.com>
> >Subject: RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >
> >  
> >>-----Original Message-----
> >>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> >>Sent: Tuesday, May 02, 2017 5:51 PM
> >>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> >>Cc: alex.williamson@redhat.com; intel-gfx@lists.freedesktop.org;
> >>intel-gvt- dev@lists.freedesktop.org; Wang, Zhi A
> >><zhi.a.wang@intel.com>; zhenyuw@linux.intel.com;
> >>linux-kernel@vger.kernel.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Tian,
> >>Kevin <kevin.tian@intel.com>
> >>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
> >>dmabuf
> >>
> >>On Fr, 2017-04-28 at 17:35 +0800, Xiaoguang Chen wrote:  
> >>> +static size_t intel_vgpu_reg_rw_gvtg(struct intel_vgpu *vgpu, char
> >>> *buf,
> >>> +               size_t count, loff_t *ppos, bool iswrite) {
> >>> +       unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
> >>> +                       VFIO_PCI_NUM_REGIONS;
> >>> +       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> >>> +       int fd;
> >>> +
> >>> +       if (pos >= vgpu->vdev.region[i].size || iswrite) {
> >>> +               gvt_vgpu_err("invalid op or offset for Intel vgpu fd
> >>> region\n");
> >>> +               return -EINVAL;
> >>> +       }
> >>> +
> >>> +       fd = anon_inode_getfd("gvtg", &intel_vgpu_gvtg_ops, vgpu,
> >>> +                       O_RDWR | O_CLOEXEC);
> >>> +       if (fd < 0) {
> >>> +               gvt_vgpu_err("create intel vgpu fd failed:%d\n", fd);
> >>> +               return -EINVAL;
> >>> +       }
> >>> +
> >>> +       count = min(count, (size_t)(vgpu->vdev.region[i].size - pos));
> >>> +       memcpy(buf, &fd, count);
> >>> +
> >>> +       return count;
> >>> +}  
> >>
> >>Hmm, that looks like a rather strange way to return a file descriptor.
> >>
> >>What is the reason to not use ioctls on the vfio file handle, like
> >>older version of these patches did?  
> >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> >handle like the old version.
> >So I used this way the smallest change to general vfio framework only adding a
> >subregion definition.

I think I was hoping we could avoid a separate file descriptor
altogether and use a vfio region instead.  However, it was explained
previously why this really needs to be a separate fd and I agree that
using a region to expose an fd is really awkward.  If we're going to
have a separate fd, let's use a device specific ioctl to get it.
Thanks,

Alex

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> > >>Hmm, that looks like a rather strange way to return a file descriptor.
> > >>
> > >>What is the reason to not use ioctls on the vfio file handle, like
> > >>older version of these patches did?  
> > >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> > >handle like the old version.
> > >So I used this way the smallest change to general vfio framework only adding a
> > >subregion definition.
> 
> I think I was hoping we could avoid a separate file descriptor
> altogether and use a vfio region instead.

What exactly did you have in mind?  Put the framebuffer information
(struct intel_vgpu_dmabuf) into the vfio region, then access it using
read/write/mmap?

> However, it was explained
> previously why this really needs to be a separate fd and I agree that
> using a region to expose an fd is really awkward.

Now with this patchset we have *two* kinds of separate file handles.
First the anon-fd created by reading from the region.  This is then used
to run the intel ioctls on, which in turn create the other kind of file
handle (dma-buf-fd).

The dma-buf-fd really needs to be a separate fd, because it gets passed
around as handle and because this is the way dma-bufs work (guess this
is the discussion you are referring to).

I can't see a compelling reason for the anon-fd though.  I suspect this
was done due to a misunderstanding ...

cheers,
  Gerd

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Fri, 05 May 2017 08:55:31 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > >>Hmm, that looks like a rather strange way to return a file descriptor.
> > > >>
> > > >>What is the reason to not use ioctls on the vfio file handle, like
> > > >>older version of these patches did?    
> > > >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> > > >handle like the old version.
> > > >So I used this way the smallest change to general vfio framework only adding a
> > > >subregion definition.  
> > 
> > I think I was hoping we could avoid a separate file descriptor
> > altogether and use a vfio region instead.  
> 
> What exactly did you have in mind?  Put the framebuffer information
> (struct intel_vgpu_dmabuf) into the vfio region, then access it using
> read/write/mmap?

Yeah, that was my hope.  Adding a new file descriptor means we have one
more reference floating around complicating the life cycle of the
device, group, and container.  Furthermore this one is really only
visible to the mdev vendor driver, so we can't rely on vfio-core, the
vendor driver will need to consider the reference when releasing the
device.

> > However, it was explained
> > previously why this really needs to be a separate fd and I agree that
> > using a region to expose an fd is really awkward.  
> 
> Now with this patchset we have *two* kinds of separate file handles.
> First the anon-fd created by reading from the region.  This is then used
> to run the intel ioctls on, which in turn create the other kind of file
> handle (dma-buf-fd).
> 
> The dma-buf-fd really needs to be a separate fd, because it gets passed
> around as handle and because this is the way dma-bufs work (guess this
> is the discussion you are referring to).

Yep, we're going to need to trust the vendor driver to manage it, we
have lots of places where we need to trust the vendor driver for an
mdev device, unfortunately.
 
> I can't see a compelling reason for the anon-fd though.  I suspect this
> was done due to a misunderstanding ...

Yeah, vfio-core passes device ioctls to the vendor driver, so the
vendor driver should be able to implement a
VFIO_DEVICE_GVT_GET_DMABUF_FD ioctl direclty.  Ideally maybe this
isn't even GVT specific, and we'd s/GVT_//.  Thanks,

Alex

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Alex,

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Alex Williamson
>Sent: Friday, May 05, 2017 11:11 PM
>To: Gerd Hoffmann <kraxel@redhat.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; Chen, Xiaoguang <xiaoguang.chen@intel.com>; intel-
>gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Fri, 05 May 2017 08:55:31 +0200
>Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>   Hi,
>>
>> > > >>Hmm, that looks like a rather strange way to return a file descriptor.
>> > > >>
>> > > >>What is the reason to not use ioctls on the vfio file handle, like
>> > > >>older version of these patches did?
>> > > >If I understood correctly that Alex prefer not to change the
>> > > >ioctls on the vfio file handle like the old version.
>> > > >So I used this way the smallest change to general vfio framework
>> > > >only adding a subregion definition.
>> >
>> > I think I was hoping we could avoid a separate file descriptor
>> > altogether and use a vfio region instead.
>>
>> What exactly did you have in mind?  Put the framebuffer information
>> (struct intel_vgpu_dmabuf) into the vfio region, then access it using
>> read/write/mmap?
>
>Yeah, that was my hope.  Adding a new file descriptor means we have one more
>reference floating around complicating the life cycle of the device, group, and
>container.  Furthermore this one is really only visible to the mdev vendor driver,
>so we can't rely on vfio-core, the vendor driver will need to consider the
>reference when releasing the device.
While read the framebuffer region we have to tell the vendor driver which framebuffer we want to read? There are two framebuffers now in KVMGT that is primary and cursor.
There are two methods to implement this:
1) write the plane id first and then read the framebuffer.
2) create 2 vfio regions one for primary and one for cursor.

Which method do you prefer? Or do you have other idea to handle this problem?

chenxg

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> While read the framebuffer region we have to tell the vendor driver which framebuffer we want to read? There are two framebuffers now in KVMGT that is primary and cursor.
> There are two methods to implement this:
> 1) write the plane id first and then read the framebuffer.
> 2) create 2 vfio regions one for primary and one for cursor.

(3) Place information for both planes into one vfio region.
    Which allows to fetch both with a single read() syscall.

The question is how you'll get the file descriptor then.  If the ioctl
returns the dma-buf fd only you have a racy interface:  Things can
change between read(vfio-region) and ioctl(need-dmabuf-fd).

ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix
the race, but then it is easier to go with ioctl only interface (simliar
to the orginal one from dec last year) I think.

cheers,
  Gerd

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Thu, 11 May 2017 15:27:53 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > While read the framebuffer region we have to tell the vendor driver which framebuffer we want to read? There are two framebuffers now in KVMGT that is primary and cursor.
> > There are two methods to implement this:
> > 1) write the plane id first and then read the framebuffer.
> > 2) create 2 vfio regions one for primary and one for cursor.  
> 
> (3) Place information for both planes into one vfio region.
>     Which allows to fetch both with a single read() syscall.
> 
> The question is how you'll get the file descriptor then.  If the ioctl
> returns the dma-buf fd only you have a racy interface:  Things can
> change between read(vfio-region) and ioctl(need-dmabuf-fd).
> 
> ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix
> the race, but then it is easier to go with ioctl only interface (simliar
> to the orginal one from dec last year) I think.

If the dmabuf fd is provided by a separate mdev vendor driver specific
ioctl, I don't see how vfio regions should be involved.  Selecting
which framebuffer should be an ioctl parameter.  What sort of
information needs to be conveyed about each plane?  Is it static
information or something that needs to be read repeatedly?  Do we need
it before we get the dmabuf fd or can it be an ioctl on the dmabuf fd?
Thanks,

Alex

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Alex and Gerd,

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Alex Williamson
>Sent: Thursday, May 11, 2017 11:45 PM
>To: Gerd Hoffmann <kraxel@redhat.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; Chen, Xiaoguang <xiaoguang.chen@intel.com>; intel-
>gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Thu, 11 May 2017 15:27:53 +0200
>Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>   Hi,
>>
>> > While read the framebuffer region we have to tell the vendor driver which
>framebuffer we want to read? There are two framebuffers now in KVMGT that is
>primary and cursor.
>> > There are two methods to implement this:
>> > 1) write the plane id first and then read the framebuffer.
>> > 2) create 2 vfio regions one for primary and one for cursor.
>>
>> (3) Place information for both planes into one vfio region.
>>     Which allows to fetch both with a single read() syscall.
>>
>> The question is how you'll get the file descriptor then.  If the ioctl
>> returns the dma-buf fd only you have a racy interface:  Things can
>> change between read(vfio-region) and ioctl(need-dmabuf-fd).
>>
>> ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix
>> the race, but then it is easier to go with ioctl only interface
>> (simliar to the orginal one from dec last year) I think.
>
>If the dmabuf fd is provided by a separate mdev vendor driver specific ioctl, I
>don't see how vfio regions should be involved.  Selecting which framebuffer
>should be an ioctl parameter.  
Based on your last mail. I think the implementation looks like this:
1) user query the framebuffer information by reading the vfio region.
2) if the framebuffer changed(such as framebuffer's graphics address changed, size changed etc) we will need to create a new dmabuf fd.
3) create a new dmabuf fd using vfio device specific ioctl.

>What sort of information needs to be conveyed
>about each plane?  
Only plane id is needed.

>Is it static information or something that needs to be read
>repeatedly? 
It is static information. For our case plane id 1 represent primary plane and 3 for cursor plane. 2 means sprite plane which will not be used in our case.

>Do we need it before we get the dmabuf fd or can it be an ioctl on
>the dmabuf fd?
We need it while query the framebuffer. In kernel we need the plane id to decide which plane we should decode.
Below is my current implementation:
1) user first query the framebuffer(primary or cursor) and kernel decode the framebuffer and return the framebuffer information to user and also save a copy in kernel.
2) user compared the framebuffer and if the framebuffer changed  creating a new dmabuf fd.
3) kernel create a new dmabuf fd based on saved framebuffer information.

So we need plane id in step 1.
In step 3 we create a dmabuf fd only using saved framebuffer information(no other information is needed).

Chenxg.
>Thanks,
>
>Alex
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Fri, 12 May 2017 02:12:10 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Alex and Gerd,
> 
> >-----Original Message-----
> >From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> >Behalf Of Alex Williamson
> >Sent: Thursday, May 11, 2017 11:45 PM
> >To: Gerd Hoffmann <kraxel@redhat.com>
> >Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> ><zhiyuan.lv@intel.com>; Chen, Xiaoguang <xiaoguang.chen@intel.com>; intel-
> >gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >On Thu, 11 May 2017 15:27:53 +0200
> >Gerd Hoffmann <kraxel@redhat.com> wrote:
> >  
> >>   Hi,
> >>  
> >> > While read the framebuffer region we have to tell the vendor driver which  
> >framebuffer we want to read? There are two framebuffers now in KVMGT that is
> >primary and cursor.  
> >> > There are two methods to implement this:
> >> > 1) write the plane id first and then read the framebuffer.
> >> > 2) create 2 vfio regions one for primary and one for cursor.  
> >>
> >> (3) Place information for both planes into one vfio region.
> >>     Which allows to fetch both with a single read() syscall.
> >>
> >> The question is how you'll get the file descriptor then.  If the ioctl
> >> returns the dma-buf fd only you have a racy interface:  Things can
> >> change between read(vfio-region) and ioctl(need-dmabuf-fd).
> >>
> >> ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix
> >> the race, but then it is easier to go with ioctl only interface
> >> (simliar to the orginal one from dec last year) I think.  
> >
> >If the dmabuf fd is provided by a separate mdev vendor driver specific ioctl, I
> >don't see how vfio regions should be involved.  Selecting which framebuffer
> >should be an ioctl parameter.    
> Based on your last mail. I think the implementation looks like this:
> 1) user query the framebuffer information by reading the vfio region.
> 2) if the framebuffer changed(such as framebuffer's graphics address changed, size changed etc) we will need to create a new dmabuf fd.
> 3) create a new dmabuf fd using vfio device specific ioctl.
> 
> >What sort of information needs to be conveyed
> >about each plane?    
> Only plane id is needed.
> 
> >Is it static information or something that needs to be read
> >repeatedly?   
> It is static information. For our case plane id 1 represent primary plane and 3 for cursor plane. 2 means sprite plane which will not be used in our case.
> 
> >Do we need it before we get the dmabuf fd or can it be an ioctl on
> >the dmabuf fd?  
> We need it while query the framebuffer. In kernel we need the plane id to decide which plane we should decode.
> Below is my current implementation:
> 1) user first query the framebuffer(primary or cursor) and kernel decode the framebuffer and return the framebuffer information to user and also save a copy in kernel.
> 2) user compared the framebuffer and if the framebuffer changed  creating a new dmabuf fd.

If the contents of the framebuffer change or if the parameters of the
framebuffer change?  I can't image that creating a new dmabuf fd for
every visual change within the framebuffer would be efficient, but I
don't have any concept of what a dmabuf actually does.

> 3) kernel create a new dmabuf fd based on saved framebuffer information.
> 
> So we need plane id in step 1.
> In step 3 we create a dmabuf fd only using saved framebuffer information(no other information is needed).

What changes to the framebuffer require a new dmabuf fd?  Shouldn't the
user query the parameters of the framebuffer through a dmabuf fd and
shouldn't the dmabuf fd have some signaling mechanism to the user
(eventfd perhaps) to notify the user to re-evaluate the parameters?
Otherwise are you imagining that the user polls the vfio region?  Why
can a dmabuf fd not persist across changes to the framebuffer?  Can
someone explain what a dmabuf is and how it works in terms that a
non-graphics person can understand?  Thanks,

Alex

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamson@redhat.com]
>Sent: Friday, May 12, 2017 10:58 AM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>;
>intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
>dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Fri, 12 May 2017 02:12:10 +0000
>"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
>
>> Hi Alex and Gerd,
>>
>> >-----Original Message-----
>> >From: intel-gvt-dev
>> >[mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of
>> >Alex Williamson
>> >Sent: Thursday, May 11, 2017 11:45 PM
>> >To: Gerd Hoffmann <kraxel@redhat.com>
>> >Cc: Tian, Kevin <kevin.tian@intel.com>;
>> >intel-gfx@lists.freedesktop.org; linux- kernel@vger.kernel.org;
>> >zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Chen,
>> >Xiaoguang <xiaoguang.chen@intel.com>; intel-
>> >gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
>> >dmabuf
>> >
>> >On Thu, 11 May 2017 15:27:53 +0200
>> >Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >
>> >>   Hi,
>> >>
>> >> > While read the framebuffer region we have to tell the vendor
>> >> > driver which
>> >framebuffer we want to read? There are two framebuffers now in KVMGT
>> >that is primary and cursor.
>> >> > There are two methods to implement this:
>> >> > 1) write the plane id first and then read the framebuffer.
>> >> > 2) create 2 vfio regions one for primary and one for cursor.
>> >>
>> >> (3) Place information for both planes into one vfio region.
>> >>     Which allows to fetch both with a single read() syscall.
>> >>
>> >> The question is how you'll get the file descriptor then.  If the
>> >> ioctl returns the dma-buf fd only you have a racy interface:
>> >> Things can change between read(vfio-region) and ioctl(need-dmabuf-fd).
>> >>
>> >> ioctl(need-dma-buf) could return both dmabuf fd and plane info to
>> >> fix the race, but then it is easier to go with ioctl only interface
>> >> (simliar to the orginal one from dec last year) I think.
>> >
>> >If the dmabuf fd is provided by a separate mdev vendor driver
>> >specific ioctl, I don't see how vfio regions should be involved.  Selecting which
>framebuffer
>> >should be an ioctl parameter.
>> Based on your last mail. I think the implementation looks like this:
>> 1) user query the framebuffer information by reading the vfio region.
>> 2) if the framebuffer changed(such as framebuffer's graphics address changed,
>size changed etc) we will need to create a new dmabuf fd.
>> 3) create a new dmabuf fd using vfio device specific ioctl.
>>
>> >What sort of information needs to be conveyed
>> >about each plane?
>> Only plane id is needed.
>>
>> >Is it static information or something that needs to be read
>> >repeatedly?
>> It is static information. For our case plane id 1 represent primary plane and 3 for
>cursor plane. 2 means sprite plane which will not be used in our case.
>>
>> >Do we need it before we get the dmabuf fd or can it be an ioctl on
>> >the dmabuf fd?
>> We need it while query the framebuffer. In kernel we need the plane id to
>decide which plane we should decode.
>> Below is my current implementation:
>> 1) user first query the framebuffer(primary or cursor) and kernel decode the
>framebuffer and return the framebuffer information to user and also save a copy
>in kernel.
>> 2) user compared the framebuffer and if the framebuffer changed  creating a
>new dmabuf fd.
>
>If the contents of the framebuffer change or if the parameters of the framebuffer
>change?  
If the parameters of the framebuffer change we need to create new dmabuf.


>I can't image that creating a new dmabuf fd for every visual change
>within the framebuffer would be efficient, but I don't have any concept of what a
>dmabuf actually does.
>
>> 3) kernel create a new dmabuf fd based on saved framebuffer information.
>>
>> So we need plane id in step 1.
>> In step 3 we create a dmabuf fd only using saved framebuffer information(no
>other information is needed).
>
>What changes to the framebuffer require a new dmabuf fd?  Shouldn't the user
>query the parameters of the framebuffer through a dmabuf fd and shouldn't the
>dmabuf fd have some signaling mechanism to the user (eventfd perhaps) to notify
>the user to re-evaluate the parameters?
>Otherwise are you imagining that the user polls the vfio region?  Why can a
>dmabuf fd not persist across changes to the framebuffer?  Can someone explain
>what a dmabuf is and how it works in terms that a non-graphics person can
>understand?  Thanks,

A dmabuf will be associated a gem object. In our case the backing storage of the gem object is from the framebuffer.
We use the GMA(graphics memory address) to traverse the GTT(graphics translation table) to get all the pages belong to the framebuffer and assign these pages to the gem object.
So once the GMA of a framebuffer changed we have to create new dmabuf for it. The other parameters of the framebuffer such as the size of the framebuffer, the tiling mode changed we also have to create a new dmabuf.

We did consider the signal mechanism to notify the user but doing so the kernel need to save a lot of information of the framebuffers so we decide to let the user check whether to create a new dmabuf or not.
So the usage flow is:
1) query the framebuffer information
2) compare with saved dmabuf whether need to create a new dmabuf
3) create a new dmabuf(save the dmabuf and related framebuffer information such as gma, size....)


>
>Alex

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> If the contents of the framebuffer change or if the parameters of the
> framebuffer change?  I can't image that creating a new dmabuf fd for
> every visual change within the framebuffer would be efficient, but I
> don't have any concept of what a dmabuf actually does.

Ok, some background:

The drm subsystem has the concept of planes.  The most important plane
is the primary framebuffer (i.e. what gets scanned out to the physical
display).  The cursor is a plane too, and there can be additional
overlay planes for stuff like video playback.

Typically there are multiple planes in a system and only one of them
gets scanned out to the crtc, i.e. the fbdev emulation creates one plane
for the framebuffer console.  The X-Server creates a plane too, and when
you switch between X-Server and framebuffer console via ctrl-alt-fn the
intel driver just reprograms the encoder to scan out the one or the
other plane to the crtc.

The dma-buf handed out by gvt is a reference to a plane.  I think on the
host side gvt can see only the active plane (from encoder/crtc register
programming) not the inactive ones.

The dma-buf can be imported as opengl texture and then be used to render
the guest display to a host window.  I think it is even possible to use
the dma-buf as plane in the host drm driver and scan it out directly to
a physical display.  The actual framebuffer content stays in gpu memory
all the time, the cpu never has to touch it.

It is possible to cache the dma-buf handles, i.e. when the guest boots
you'll get the first for the fbcon plane, when the x-server starts the
second for the x-server framebuffer, and when the user switches to the
text console via ctrl-alt-fn you can re-use the fbcon dma-buf you
already have.

The caching becomes more important for good performance when the guest
uses pageflipping (wayland does): define two planes, render into one
while displaying the other, then flip the two for a atomic display
update.

The caching also makes it a bit difficult to create a good interface.
So, the current patch set creates:

  (a) A way to query the active planes (ioctl
      INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
  (b) A way to create a dma-buf for the active plane (ioctl
      INTEL_VGPU_GENERATE_DMABUF).

Typical userspace workflow is to first query the plane, then check if it
already has a dma-buf for it, and if not create one.

> What changes to the framebuffer require a new dmabuf fd?  Shouldn't the
> user query the parameters of the framebuffer through a dmabuf fd and
> shouldn't the dmabuf fd have some signaling mechanism to the user
> (eventfd perhaps) to notify the user to re-evaluate the parameters?

dma-bufs don't support that, they are really just a handle to a piece of
memory, all metadata (format, size) most be communicated by other means.

> Otherwise are you imagining that the user polls the vfio region?

Hmm, notification support would probably a good reason to have a
separate file handle to manage the dma-bufs (instead of using
driver-specific ioctls on the vfio fd), because the driver could also
use the management fd for notifications then.

I'm not sure how useful notification support actually is though.
Notifications when another plane gets mapped to the crtc should be easy.
But I'm not sure it is possible to get notifications when the plane
content changes, especially in case the guest does software rendering so
the display is updated without gvt seeing guest activity on the
rendering pipeline.  Without the later qemu needs a timer for display
updates _anyway_ ...

cheers,
  Gerd

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Fri, 12 May 2017 11:12:05 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > If the contents of the framebuffer change or if the parameters of the
> > framebuffer change?  I can't image that creating a new dmabuf fd for
> > every visual change within the framebuffer would be efficient, but I
> > don't have any concept of what a dmabuf actually does.  
> 
> Ok, some background:
> 
> The drm subsystem has the concept of planes.  The most important plane
> is the primary framebuffer (i.e. what gets scanned out to the physical
> display).  The cursor is a plane too, and there can be additional
> overlay planes for stuff like video playback.
> 
> Typically there are multiple planes in a system and only one of them
> gets scanned out to the crtc, i.e. the fbdev emulation creates one plane
> for the framebuffer console.  The X-Server creates a plane too, and when
> you switch between X-Server and framebuffer console via ctrl-alt-fn the
> intel driver just reprograms the encoder to scan out the one or the
> other plane to the crtc.
> 
> The dma-buf handed out by gvt is a reference to a plane.  I think on the
> host side gvt can see only the active plane (from encoder/crtc register
> programming) not the inactive ones.
> 
> The dma-buf can be imported as opengl texture and then be used to render
> the guest display to a host window.  I think it is even possible to use
> the dma-buf as plane in the host drm driver and scan it out directly to
> a physical display.  The actual framebuffer content stays in gpu memory
> all the time, the cpu never has to touch it.
> 
> It is possible to cache the dma-buf handles, i.e. when the guest boots
> you'll get the first for the fbcon plane, when the x-server starts the
> second for the x-server framebuffer, and when the user switches to the
> text console via ctrl-alt-fn you can re-use the fbcon dma-buf you
> already have.
> 
> The caching becomes more important for good performance when the guest
> uses pageflipping (wayland does): define two planes, render into one
> while displaying the other, then flip the two for a atomic display
> update.
> 
> The caching also makes it a bit difficult to create a good interface.
> So, the current patch set creates:
> 
>   (a) A way to query the active planes (ioctl
>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
>   (b) A way to create a dma-buf for the active plane (ioctl
>       INTEL_VGPU_GENERATE_DMABUF).
> 
> Typical userspace workflow is to first query the plane, then check if it
> already has a dma-buf for it, and if not create one.

Thank you!  This is immensely helpful!
 
> > What changes to the framebuffer require a new dmabuf fd?  Shouldn't the
> > user query the parameters of the framebuffer through a dmabuf fd and
> > shouldn't the dmabuf fd have some signaling mechanism to the user
> > (eventfd perhaps) to notify the user to re-evaluate the parameters?  
> 
> dma-bufs don't support that, they are really just a handle to a piece of
> memory, all metadata (format, size) most be communicated by other means.
> 
> > Otherwise are you imagining that the user polls the vfio region?  
> 
> Hmm, notification support would probably a good reason to have a
> separate file handle to manage the dma-bufs (instead of using
> driver-specific ioctls on the vfio fd), because the driver could also
> use the management fd for notifications then.

I like this idea of a separate control fd for dmabufs, it provides not
only a central management point, but also a nice abstraction for the
vfio device specific interface.  We potentially only need a single
VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management fd
(perhaps with a type parameter, ex. GFX) where maybe we could have
vfio-core incorporate this reference into the group lifecycle, so the
vendor driver only needs to fdget/put this manager fd for the various
plane dmabuf fds spawned in order to get core-level reference counting.

The dmabuf manager fd can be separately versioned from vfio or make use
of some vendor magic if there are portions that are vendor specific
(the above examples for query/get ioctls really don't seem vendor
specific).
 
> I'm not sure how useful notification support actually is though.
> Notifications when another plane gets mapped to the crtc should be easy.
> But I'm not sure it is possible to get notifications when the plane
> content changes, especially in case the guest does software rendering so
> the display is updated without gvt seeing guest activity on the
> rendering pipeline.  Without the later qemu needs a timer for display
> updates _anyway_ ...

Seems like it has benefits even if we don't have an initial use for
notification mechanisms.  Thanks,

Alex

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Alex and Gerd,

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamson@redhat.com]
>Sent: Saturday, May 13, 2017 12:38 AM
>To: Gerd Hoffmann <kraxel@redhat.com>
>Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
><zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Fri, 12 May 2017 11:12:05 +0200
>Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>   Hi,
>>
>> > If the contents of the framebuffer change or if the parameters of
>> > the framebuffer change?  I can't image that creating a new dmabuf fd
>> > for every visual change within the framebuffer would be efficient,
>> > but I don't have any concept of what a dmabuf actually does.
>>
>> Ok, some background:
>>
>> The drm subsystem has the concept of planes.  The most important plane
>> is the primary framebuffer (i.e. what gets scanned out to the physical
>> display).  The cursor is a plane too, and there can be additional
>> overlay planes for stuff like video playback.
>>
>> Typically there are multiple planes in a system and only one of them
>> gets scanned out to the crtc, i.e. the fbdev emulation creates one
>> plane for the framebuffer console.  The X-Server creates a plane too,
>> and when you switch between X-Server and framebuffer console via
>> ctrl-alt-fn the intel driver just reprograms the encoder to scan out
>> the one or the other plane to the crtc.
>>
>> The dma-buf handed out by gvt is a reference to a plane.  I think on
>> the host side gvt can see only the active plane (from encoder/crtc
>> register
>> programming) not the inactive ones.
>>
>> The dma-buf can be imported as opengl texture and then be used to
>> render the guest display to a host window.  I think it is even
>> possible to use the dma-buf as plane in the host drm driver and scan
>> it out directly to a physical display.  The actual framebuffer content
>> stays in gpu memory all the time, the cpu never has to touch it.
>>
>> It is possible to cache the dma-buf handles, i.e. when the guest boots
>> you'll get the first for the fbcon plane, when the x-server starts the
>> second for the x-server framebuffer, and when the user switches to the
>> text console via ctrl-alt-fn you can re-use the fbcon dma-buf you
>> already have.
>>
>> The caching becomes more important for good performance when the guest
>> uses pageflipping (wayland does): define two planes, render into one
>> while displaying the other, then flip the two for a atomic display
>> update.
>>
>> The caching also makes it a bit difficult to create a good interface.
>> So, the current patch set creates:
>>
>>   (a) A way to query the active planes (ioctl
>>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
>>   (b) A way to create a dma-buf for the active plane (ioctl
>>       INTEL_VGPU_GENERATE_DMABUF).
>>
>> Typical userspace workflow is to first query the plane, then check if
>> it already has a dma-buf for it, and if not create one.
>
>Thank you!  This is immensely helpful!
>
>> > What changes to the framebuffer require a new dmabuf fd?  Shouldn't
>> > the user query the parameters of the framebuffer through a dmabuf fd
>> > and shouldn't the dmabuf fd have some signaling mechanism to the
>> > user (eventfd perhaps) to notify the user to re-evaluate the parameters?
>>
>> dma-bufs don't support that, they are really just a handle to a piece
>> of memory, all metadata (format, size) most be communicated by other means.
>>
>> > Otherwise are you imagining that the user polls the vfio region?
>>
>> Hmm, notification support would probably a good reason to have a
>> separate file handle to manage the dma-bufs (instead of using
>> driver-specific ioctls on the vfio fd), because the driver could also
>> use the management fd for notifications then.
>
>I like this idea of a separate control fd for dmabufs, it provides not only a central
>management point, but also a nice abstraction for the vfio device specific
>interface.  We potentially only need a single
>VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management fd
>(perhaps with a type parameter, ex. GFX) where maybe we could have vfio-core
>incorporate this reference into the group lifecycle, so the vendor driver only
>needs to fdget/put this manager fd for the various plane dmabuf fds spawned in
>order to get core-level reference counting.
Following is my understanding of the management fd idea:
1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to create a fd and saved the fd in vfio group while initializing the vfio.
2) vendor driver use fdget to add reference count of the fd.
3) vendor driver use ioctl to the fd to query plane information or create dma-buf fd.
4) vendor driver use fdput when finished using this fd.

Is my understanding right?

Both QEMU and kernel vfio-core will have changes based on this proposal except the vendor part changes.
Who will make these changes?

Thanks
Chenxg

>
>The dmabuf manager fd can be separately versioned from vfio or make use of
>some vendor magic if there are portions that are vendor specific (the above
>examples for query/get ioctls really don't seem vendor specific).
>
>> I'm not sure how useful notification support actually is though.
>> Notifications when another plane gets mapped to the crtc should be easy.
>> But I'm not sure it is possible to get notifications when the plane
>> content changes, especially in case the guest does software rendering
>> so the display is updated without gvt seeing guest activity on the
>> rendering pipeline.  Without the later qemu needs a timer for display
>> updates _anyway_ ...
>
>Seems like it has benefits even if we don't have an initial use for notification
>mechanisms.  Thanks,
>
>Alex

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Mon, 15 May 2017 03:36:50 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Alex and Gerd,
> 
> >-----Original Message-----
> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
> >Sent: Saturday, May 13, 2017 12:38 AM
> >To: Gerd Hoffmann <kraxel@redhat.com>
> >Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> ><zhi.a.wang@intel.com>
> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >On Fri, 12 May 2017 11:12:05 +0200
> >Gerd Hoffmann <kraxel@redhat.com> wrote:
> >  
> >>   Hi,
> >>  
> >> > If the contents of the framebuffer change or if the parameters of
> >> > the framebuffer change?  I can't image that creating a new dmabuf fd
> >> > for every visual change within the framebuffer would be efficient,
> >> > but I don't have any concept of what a dmabuf actually does.  
> >>
> >> Ok, some background:
> >>
> >> The drm subsystem has the concept of planes.  The most important plane
> >> is the primary framebuffer (i.e. what gets scanned out to the physical
> >> display).  The cursor is a plane too, and there can be additional
> >> overlay planes for stuff like video playback.
> >>
> >> Typically there are multiple planes in a system and only one of them
> >> gets scanned out to the crtc, i.e. the fbdev emulation creates one
> >> plane for the framebuffer console.  The X-Server creates a plane too,
> >> and when you switch between X-Server and framebuffer console via
> >> ctrl-alt-fn the intel driver just reprograms the encoder to scan out
> >> the one or the other plane to the crtc.
> >>
> >> The dma-buf handed out by gvt is a reference to a plane.  I think on
> >> the host side gvt can see only the active plane (from encoder/crtc
> >> register
> >> programming) not the inactive ones.
> >>
> >> The dma-buf can be imported as opengl texture and then be used to
> >> render the guest display to a host window.  I think it is even
> >> possible to use the dma-buf as plane in the host drm driver and scan
> >> it out directly to a physical display.  The actual framebuffer content
> >> stays in gpu memory all the time, the cpu never has to touch it.
> >>
> >> It is possible to cache the dma-buf handles, i.e. when the guest boots
> >> you'll get the first for the fbcon plane, when the x-server starts the
> >> second for the x-server framebuffer, and when the user switches to the
> >> text console via ctrl-alt-fn you can re-use the fbcon dma-buf you
> >> already have.
> >>
> >> The caching becomes more important for good performance when the guest
> >> uses pageflipping (wayland does): define two planes, render into one
> >> while displaying the other, then flip the two for a atomic display
> >> update.
> >>
> >> The caching also makes it a bit difficult to create a good interface.
> >> So, the current patch set creates:
> >>
> >>   (a) A way to query the active planes (ioctl
> >>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
> >>   (b) A way to create a dma-buf for the active plane (ioctl
> >>       INTEL_VGPU_GENERATE_DMABUF).
> >>
> >> Typical userspace workflow is to first query the plane, then check if
> >> it already has a dma-buf for it, and if not create one.  
> >
> >Thank you!  This is immensely helpful!
> >  
> >> > What changes to the framebuffer require a new dmabuf fd?  Shouldn't
> >> > the user query the parameters of the framebuffer through a dmabuf fd
> >> > and shouldn't the dmabuf fd have some signaling mechanism to the
> >> > user (eventfd perhaps) to notify the user to re-evaluate the parameters?  
> >>
> >> dma-bufs don't support that, they are really just a handle to a piece
> >> of memory, all metadata (format, size) most be communicated by other means.
> >>  
> >> > Otherwise are you imagining that the user polls the vfio region?  
> >>
> >> Hmm, notification support would probably a good reason to have a
> >> separate file handle to manage the dma-bufs (instead of using
> >> driver-specific ioctls on the vfio fd), because the driver could also
> >> use the management fd for notifications then.  
> >
> >I like this idea of a separate control fd for dmabufs, it provides not only a central
> >management point, but also a nice abstraction for the vfio device specific
> >interface.  We potentially only need a single
> >VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management fd
> >(perhaps with a type parameter, ex. GFX) where maybe we could have vfio-core
> >incorporate this reference into the group lifecycle, so the vendor driver only
> >needs to fdget/put this manager fd for the various plane dmabuf fds spawned in
> >order to get core-level reference counting.  
> Following is my understanding of the management fd idea:
> 1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to create a fd and saved the fd in vfio group while initializing the vfio.

Ideally there'd be kernel work here too if we want vfio-core to
incorporate lifecycle of this fd into the device/group/container
lifecycle.  Maybe we even want to generalize it further to something
like VFIO_DEVICE_GET_FD which takes a parameter of what type of FD to
get, GFX_DMABUF_MGR_FD in this case.  vfio-core would probably allocate
the fd, tap into the release hook for reference counting and pass it to
the vfio_device_ops (mdev vendor driver in this case) to attach further.

> 2) vendor driver use fdget to add reference count of the fd.
> 3) vendor driver use ioctl to the fd to query plane information or create dma-buf fd.
> 4) vendor driver use fdput when finished using this fd.
> 
> Is my understanding right?

With the above addition, which maybe you were already considering, seems
right.

> Both QEMU and kernel vfio-core will have changes based on this proposal except the vendor part changes.
> Who will make these changes?

/me points to the folks trying to enable this functionality...

Thanks,
Alex

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Alex,

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamson@redhat.com]
>Sent: Tuesday, May 16, 2017 1:44 AM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>;
>intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
>dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Mon, 15 May 2017 03:36:50 +0000
>"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
>
>> Hi Alex and Gerd,
>>
>> >-----Original Message-----
>> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> >Sent: Saturday, May 13, 2017 12:38 AM
>> >To: Gerd Hoffmann <kraxel@redhat.com>
>> >Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
>> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
>> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
>> >Zhi A <zhi.a.wang@intel.com>
>> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
>> >dmabuf
>> >
>> >On Fri, 12 May 2017 11:12:05 +0200
>> >Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >
>> >>   Hi,
>> >>
>> >> > If the contents of the framebuffer change or if the parameters of
>> >> > the framebuffer change?  I can't image that creating a new dmabuf
>> >> > fd for every visual change within the framebuffer would be
>> >> > efficient, but I don't have any concept of what a dmabuf actually does.
>> >>
>> >> Ok, some background:
>> >>
>> >> The drm subsystem has the concept of planes.  The most important
>> >> plane is the primary framebuffer (i.e. what gets scanned out to the
>> >> physical display).  The cursor is a plane too, and there can be
>> >> additional overlay planes for stuff like video playback.
>> >>
>> >> Typically there are multiple planes in a system and only one of
>> >> them gets scanned out to the crtc, i.e. the fbdev emulation creates
>> >> one plane for the framebuffer console.  The X-Server creates a
>> >> plane too, and when you switch between X-Server and framebuffer
>> >> console via ctrl-alt-fn the intel driver just reprograms the
>> >> encoder to scan out the one or the other plane to the crtc.
>> >>
>> >> The dma-buf handed out by gvt is a reference to a plane.  I think
>> >> on the host side gvt can see only the active plane (from
>> >> encoder/crtc register
>> >> programming) not the inactive ones.
>> >>
>> >> The dma-buf can be imported as opengl texture and then be used to
>> >> render the guest display to a host window.  I think it is even
>> >> possible to use the dma-buf as plane in the host drm driver and
>> >> scan it out directly to a physical display.  The actual framebuffer
>> >> content stays in gpu memory all the time, the cpu never has to touch it.
>> >>
>> >> It is possible to cache the dma-buf handles, i.e. when the guest
>> >> boots you'll get the first for the fbcon plane, when the x-server
>> >> starts the second for the x-server framebuffer, and when the user
>> >> switches to the text console via ctrl-alt-fn you can re-use the
>> >> fbcon dma-buf you already have.
>> >>
>> >> The caching becomes more important for good performance when the
>> >> guest uses pageflipping (wayland does): define two planes, render
>> >> into one while displaying the other, then flip the two for a atomic
>> >> display update.
>> >>
>> >> The caching also makes it a bit difficult to create a good interface.
>> >> So, the current patch set creates:
>> >>
>> >>   (a) A way to query the active planes (ioctl
>> >>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
>> >>   (b) A way to create a dma-buf for the active plane (ioctl
>> >>       INTEL_VGPU_GENERATE_DMABUF).
>> >>
>> >> Typical userspace workflow is to first query the plane, then check
>> >> if it already has a dma-buf for it, and if not create one.
>> >
>> >Thank you!  This is immensely helpful!
>> >
>> >> > What changes to the framebuffer require a new dmabuf fd?
>> >> > Shouldn't the user query the parameters of the framebuffer
>> >> > through a dmabuf fd and shouldn't the dmabuf fd have some
>> >> > signaling mechanism to the user (eventfd perhaps) to notify the user to re-
>evaluate the parameters?
>> >>
>> >> dma-bufs don't support that, they are really just a handle to a
>> >> piece of memory, all metadata (format, size) most be communicated by
>other means.
>> >>
>> >> > Otherwise are you imagining that the user polls the vfio region?
>> >>
>> >> Hmm, notification support would probably a good reason to have a
>> >> separate file handle to manage the dma-bufs (instead of using
>> >> driver-specific ioctls on the vfio fd), because the driver could
>> >> also use the management fd for notifications then.
>> >
>> >I like this idea of a separate control fd for dmabufs, it provides
>> >not only a central management point, but also a nice abstraction for
>> >the vfio device specific interface.  We potentially only need a
>> >single
>> >VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management fd
>> >(perhaps with a type parameter, ex. GFX) where maybe we could have
>> >vfio-core incorporate this reference into the group lifecycle, so the
>> >vendor driver only needs to fdget/put this manager fd for the various
>> >plane dmabuf fds spawned in order to get core-level reference counting.
>> Following is my understanding of the management fd idea:
>> 1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to create a fd
>and saved the fd in vfio group while initializing the vfio.
>
>Ideally there'd be kernel work here too if we want vfio-core to incorporate
>lifecycle of this fd into the device/group/container lifecycle.  Maybe we even
>want to generalize it further to something like VFIO_DEVICE_GET_FD which takes
>a parameter of what type of FD to get, GFX_DMABUF_MGR_FD in this case.  vfio-
>core would probably allocate the fd, tap into the release hook for reference
>counting and pass it to the vfio_device_ops (mdev vendor driver in this case) to
>attach further.
I tried to implement this today and now it functionally worked.
I am still a little confuse of how to tap the fd into the release hook  of device/group/container.
I tried to create the fd in vfio core but found it is difficult to get the file operations for the fd, the file operations should be supplied by vendor drivers.
So the fd is created in kvmgt.c for now. 
Below is part of the codes:

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 389f072..d0649ba 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -41,6 +41,7 @@
 #include <linux/kvm_host.h>
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <linux/anon_inodes.h>

 #include "i915_drv.h"
 #include "gvt.h"
@@ -524,6 +525,63 @@ static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
        return ret;
 }

+static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file, struct vm_area_struct *vma)
+{
+       WARN_ON(1);
+
+       return 0;
+}
+
+static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode, struct file *filp)
+{
+       struct intel_vgpu *vgpu = filp->private_data;
+
+       if (vgpu->vdev.vfio_device != NULL)
+               vfio_device_put(vgpu->vdev.vfio_device);
+
+       return 0;
+}
+
+static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
+               unsigned int ioctl, unsigned long arg)
+{
+       struct intel_vgpu *vgpu = filp->private_data;
+       int minsz;
+       struct intel_vgpu_dmabuf dmabuf;
+       int ret;
+       struct fd f;
+       f = fdget(dmabuf.fd);
+       minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
+       if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+               return -EFAULT;
+       if (ioctl == INTEL_VGPU_QUERY_DMABUF)
+               ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
+       else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
+               ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu, &dmabuf);
+       else {
+               fdput(f);
+               gvt_vgpu_err("unsupported dmabuf operation\n");
+               return -EINVAL;
+       }
+
+       if (ret != 0) {
+               fdput(f);
+               gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
+               return -EINVAL;
+       }
+       fdput(f);
+
+       return copy_to_user((void __user *)arg, &dmabuf, minsz) ? -EFAULT : 0;
+}
+
+static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
+       .release        = intel_vgpu_dmabuf_mgr_fd_release,
+       .unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
+       .mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
+       .llseek         = noop_llseek,
+};
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
 {
        struct intel_vgpu *vgpu = NULL;
@@ -1259,6 +1317,31 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
        } else if (cmd == VFIO_DEVICE_RESET) {
                intel_gvt_ops->vgpu_reset(vgpu);
                return 0;
+       } else if (cmd == VFIO_DEVICE_GET_FD) {
+               struct vfio_fd vfio_fd;
+               int fd;
+               struct vfio_device *device;
+
+               minsz = offsetofend(struct vfio_fd, fd);
+               if (copy_from_user(&vfio_fd, (void __user *)arg, minsz))
+                       return -EINVAL;
+
+               if (vfio_fd.argsz < minsz)
+                       return -EINVAL;
+
+               fd = anon_inode_getfd("vfio_dmabuf_mgr_fd", &intel_vgpu_dmabuf_mgr_fd_ops,
+                                       vgpu, O_RDWR | O_CLOEXEC);
+               if (fd < 0)
+                       return -EINVAL;
+
+               vfio_fd.fd = fd;
+               device = vfio_device_get_from_dev(mdev_dev(mdev));
+               if (device == NULL)
+                       gvt_vgpu_err("kvmgt: vfio device is null\n");
+               else
+                       vgpu->vdev.vfio_device = device;
+
+               return copy_to_user((void __user *)arg, &vfio_fd, minsz) ? -EFAULT : 0;
        }

        return 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 519eff3..98be2e0 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -484,6 +485,20 @@ struct vfio_pci_hot_reset {

 #define VFIO_DEVICE_PCI_HOT_RESET      _IO(VFIO_TYPE, VFIO_BASE + 13)

+/**
+ * VFIO_DEVICE_GET_FD - _IOW(VFIO_TYPE, VFIO_BASE + 21, struct vfio_fd)
+ *
+ * Create a fd for a vfio device.
+ * This fd can be used for various purpose.
+ */
+struct vfio_fd {
+       __u32 argsz;
+       __u32 flags;
+       /* out */
+       __u32 fd;
+};
+#define VFIO_DEVICE_GET_FD     _IO(VFIO_TYPE, VFIO_BASE + 14)
+
 /* -------- API for Type1 VFIO IOMMU -------- */

 /**
--
2.7.4

Thanks 
chenxg


>
>> 2) vendor driver use fdget to add reference count of the fd.
>> 3) vendor driver use ioctl to the fd to query plane information or create dma-
>buf fd.
>> 4) vendor driver use fdput when finished using this fd.
>>
>> Is my understanding right?
>
>With the above addition, which maybe you were already considering, seems right.
>
>> Both QEMU and kernel vfio-core will have changes based on this proposal
>except the vendor part changes.
>> Who will make these changes?
>
>/me points to the folks trying to enable this functionality...
>
>Thanks,
>Alex

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Tue, 16 May 2017 10:16:28 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Alex,
> 
> >-----Original Message-----
> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
> >Sent: Tuesday, May 16, 2017 1:44 AM
> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> >Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>;
> >intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> >zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
> >dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >On Mon, 15 May 2017 03:36:50 +0000
> >"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
> >  
> >> Hi Alex and Gerd,
> >>  
> >> >-----Original Message-----
> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
> >> >Sent: Saturday, May 13, 2017 12:38 AM
> >> >To: Gerd Hoffmann <kraxel@redhat.com>
> >> >Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
> >> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> >> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> >> >Zhi A <zhi.a.wang@intel.com>
> >> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
> >> >dmabuf
> >> >
> >> >On Fri, 12 May 2017 11:12:05 +0200
> >> >Gerd Hoffmann <kraxel@redhat.com> wrote:
> >> >  
> >> >>   Hi,
> >> >>  
> >> >> > If the contents of the framebuffer change or if the parameters of
> >> >> > the framebuffer change?  I can't image that creating a new dmabuf
> >> >> > fd for every visual change within the framebuffer would be
> >> >> > efficient, but I don't have any concept of what a dmabuf actually does.  
> >> >>
> >> >> Ok, some background:
> >> >>
> >> >> The drm subsystem has the concept of planes.  The most important
> >> >> plane is the primary framebuffer (i.e. what gets scanned out to the
> >> >> physical display).  The cursor is a plane too, and there can be
> >> >> additional overlay planes for stuff like video playback.
> >> >>
> >> >> Typically there are multiple planes in a system and only one of
> >> >> them gets scanned out to the crtc, i.e. the fbdev emulation creates
> >> >> one plane for the framebuffer console.  The X-Server creates a
> >> >> plane too, and when you switch between X-Server and framebuffer
> >> >> console via ctrl-alt-fn the intel driver just reprograms the
> >> >> encoder to scan out the one or the other plane to the crtc.
> >> >>
> >> >> The dma-buf handed out by gvt is a reference to a plane.  I think
> >> >> on the host side gvt can see only the active plane (from
> >> >> encoder/crtc register
> >> >> programming) not the inactive ones.
> >> >>
> >> >> The dma-buf can be imported as opengl texture and then be used to
> >> >> render the guest display to a host window.  I think it is even
> >> >> possible to use the dma-buf as plane in the host drm driver and
> >> >> scan it out directly to a physical display.  The actual framebuffer
> >> >> content stays in gpu memory all the time, the cpu never has to touch it.
> >> >>
> >> >> It is possible to cache the dma-buf handles, i.e. when the guest
> >> >> boots you'll get the first for the fbcon plane, when the x-server
> >> >> starts the second for the x-server framebuffer, and when the user
> >> >> switches to the text console via ctrl-alt-fn you can re-use the
> >> >> fbcon dma-buf you already have.
> >> >>
> >> >> The caching becomes more important for good performance when the
> >> >> guest uses pageflipping (wayland does): define two planes, render
> >> >> into one while displaying the other, then flip the two for a atomic
> >> >> display update.
> >> >>
> >> >> The caching also makes it a bit difficult to create a good interface.
> >> >> So, the current patch set creates:
> >> >>
> >> >>   (a) A way to query the active planes (ioctl
> >> >>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
> >> >>   (b) A way to create a dma-buf for the active plane (ioctl
> >> >>       INTEL_VGPU_GENERATE_DMABUF).
> >> >>
> >> >> Typical userspace workflow is to first query the plane, then check
> >> >> if it already has a dma-buf for it, and if not create one.  
> >> >
> >> >Thank you!  This is immensely helpful!
> >> >  
> >> >> > What changes to the framebuffer require a new dmabuf fd?
> >> >> > Shouldn't the user query the parameters of the framebuffer
> >> >> > through a dmabuf fd and shouldn't the dmabuf fd have some
> >> >> > signaling mechanism to the user (eventfd perhaps) to notify the user to re-  
> >evaluate the parameters?  
> >> >>
> >> >> dma-bufs don't support that, they are really just a handle to a
> >> >> piece of memory, all metadata (format, size) most be communicated by  
> >other means.  
> >> >>  
> >> >> > Otherwise are you imagining that the user polls the vfio region?  
> >> >>
> >> >> Hmm, notification support would probably a good reason to have a
> >> >> separate file handle to manage the dma-bufs (instead of using
> >> >> driver-specific ioctls on the vfio fd), because the driver could
> >> >> also use the management fd for notifications then.  
> >> >
> >> >I like this idea of a separate control fd for dmabufs, it provides
> >> >not only a central management point, but also a nice abstraction for
> >> >the vfio device specific interface.  We potentially only need a
> >> >single
> >> >VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management fd
> >> >(perhaps with a type parameter, ex. GFX) where maybe we could have
> >> >vfio-core incorporate this reference into the group lifecycle, so the
> >> >vendor driver only needs to fdget/put this manager fd for the various
> >> >plane dmabuf fds spawned in order to get core-level reference counting.  
> >> Following is my understanding of the management fd idea:
> >> 1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to create a fd  
> >and saved the fd in vfio group while initializing the vfio.
> >
> >Ideally there'd be kernel work here too if we want vfio-core to incorporate
> >lifecycle of this fd into the device/group/container lifecycle.  Maybe we even
> >want to generalize it further to something like VFIO_DEVICE_GET_FD which takes
> >a parameter of what type of FD to get, GFX_DMABUF_MGR_FD in this case.  vfio-
> >core would probably allocate the fd, tap into the release hook for reference
> >counting and pass it to the vfio_device_ops (mdev vendor driver in this case) to
> >attach further.  
> I tried to implement this today and now it functionally worked.
> I am still a little confuse of how to tap the fd into the release hook  of device/group/container.
> I tried to create the fd in vfio core but found it is difficult to get the file operations for the fd, the file operations should be supplied by vendor drivers.

I'm not fully convinced there's benefit to having vfio-core attempt to
do this, I was just hoping to avoid each vendor driver needing to
implement their own reference counting.  I don't think vfio-core wants
to get into tracking each ioctl for each vendor specific fd type to
know which create new references and which don't.  Perhaps we'll come
up with easier ways to do this as we go.

> So the fd is created in kvmgt.c for now. 
> Below is part of the codes:
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 389f072..d0649ba 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -41,6 +41,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/vfio.h>
>  #include <linux/mdev.h>
> +#include <linux/anon_inodes.h>
> 
>  #include "i915_drv.h"
>  #include "gvt.h"
> @@ -524,6 +525,63 @@ static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
>         return ret;
>  }
> 
> +static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       WARN_ON(1);

A user can abuse this, simply return error.

> +
> +       return 0;
> +}
> +
> +static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode, struct file *filp)
> +{
> +       struct intel_vgpu *vgpu = filp->private_data;
> +
> +       if (vgpu->vdev.vfio_device != NULL)
> +               vfio_device_put(vgpu->vdev.vfio_device);

When does the case occur where we don't have a vfio_device?  This looks
a bit like a warning flag that reference counting isn't handled
properly.

> +
> +       return 0;
> +}
> +
> +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
> +               unsigned int ioctl, unsigned long arg)
> +{
> +       struct intel_vgpu *vgpu = filp->private_data;
> +       int minsz;
> +       struct intel_vgpu_dmabuf dmabuf;
> +       int ret;
> +       struct fd f;
> +       f = fdget(dmabuf.fd);
> +       minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
> +       if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
> +               return -EFAULT;
> +       if (ioctl == INTEL_VGPU_QUERY_DMABUF)
> +               ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
> +       else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
> +               ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu, &dmabuf);

Why do we need vendor specific ioctls here?  Aren't querying the
current plane and getting an fd for that plane very generic concepts? 
Is the resulting dmabuf Intel specific?

> +       else {
> +               fdput(f);
> +               gvt_vgpu_err("unsupported dmabuf operation\n");
> +               return -EINVAL;
> +       }
> +
> +       if (ret != 0) {
> +               fdput(f);
> +               gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
> +               return -EINVAL;
> +       }
> +       fdput(f);
> +
> +       return copy_to_user((void __user *)arg, &dmabuf, minsz) ? -EFAULT : 0;
> +}
> +
> +static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
> +       .release        = intel_vgpu_dmabuf_mgr_fd_release,
> +       .unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
> +       .mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
> +       .llseek         = noop_llseek,
> +};
> +
>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>  {
>         struct intel_vgpu *vgpu = NULL;
> @@ -1259,6 +1317,31 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
>         } else if (cmd == VFIO_DEVICE_RESET) {
>                 intel_gvt_ops->vgpu_reset(vgpu);
>                 return 0;
> +       } else if (cmd == VFIO_DEVICE_GET_FD) {
> +               struct vfio_fd vfio_fd;
> +               int fd;
> +               struct vfio_device *device;
> +
> +               minsz = offsetofend(struct vfio_fd, fd);
> +               if (copy_from_user(&vfio_fd, (void __user *)arg, minsz))
> +                       return -EINVAL;
> +
> +               if (vfio_fd.argsz < minsz)
> +                       return -EINVAL;
> +
> +               fd = anon_inode_getfd("vfio_dmabuf_mgr_fd", &intel_vgpu_dmabuf_mgr_fd_ops,
> +                                       vgpu, O_RDWR | O_CLOEXEC);
> +               if (fd < 0)
> +                       return -EINVAL;
> +
> +               vfio_fd.fd = fd;
> +               device = vfio_device_get_from_dev(mdev_dev(mdev));
> +               if (device == NULL)
> +                       gvt_vgpu_err("kvmgt: vfio device is null\n");
> +               else
> +                       vgpu->vdev.vfio_device = device;
> +
> +               return copy_to_user((void __user *)arg, &vfio_fd, minsz) ? -EFAULT : 0;
>         }
> 
>         return 0;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 519eff3..98be2e0 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -484,6 +485,20 @@ struct vfio_pci_hot_reset {
> 
>  #define VFIO_DEVICE_PCI_HOT_RESET      _IO(VFIO_TYPE, VFIO_BASE + 13)
> 
> +/**
> + * VFIO_DEVICE_GET_FD - _IOW(VFIO_TYPE, VFIO_BASE + 21, struct vfio_fd)
> + *
> + * Create a fd for a vfio device.
> + * This fd can be used for various purpose.
> + */
> +struct vfio_fd {
> +       __u32 argsz;
> +       __u32 flags;
> +       /* out */
> +       __u32 fd;
> +};
> +#define VFIO_DEVICE_GET_FD     _IO(VFIO_TYPE, VFIO_BASE + 14)


The idea was that we pass some sort of type to VFIO_DEVICE_GET_FD, for
instance we might ask for a DEVICE_FD_GRAPHICS_DMABUF and the vfio bus
driver (mdev vendor driver) would test whether it supports that type of
thing and either return an fd or error.  We can return the fd the same
way we do for VFIO_DEVICE_GET_FD.  For instance the user should do
something like:

dmabuf_fd = ioctl(device_fd,
		VFIO_DEVICE_GET_FD, DEVICE_FD_GRAPHICS_DMABUF);
if (dmabuf_fd < 0)
	/* not supported... */
else
	/* do stuff */

Thanks,
Alex

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Alex,

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamson@redhat.com]
>Sent: Thursday, May 18, 2017 5:44 AM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>;
>linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
><zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Tue, 16 May 2017 10:16:28 +0000
>"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
>
>> Hi Alex,
>>
>> >-----Original Message-----
>> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> >Sent: Tuesday, May 16, 2017 1:44 AM
>> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>> >Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin
>> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org;
>> >linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
>> ><zhiyuan.lv@intel.com>; intel-gvt- dev@lists.freedesktop.org; Wang,
>> >Zhi A <zhi.a.wang@intel.com>
>> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
>> >dmabuf
>> >
>> >On Mon, 15 May 2017 03:36:50 +0000
>> >"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
>> >
>> >> Hi Alex and Gerd,
>> >>
>> >> >-----Original Message-----
>> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> >> >Sent: Saturday, May 13, 2017 12:38 AM
>> >> >To: Gerd Hoffmann <kraxel@redhat.com>
>> >> >Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
>> >> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>> >> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
>> >> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
>> >> >Zhi A <zhi.a.wang@intel.com>
>> >> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting
>> >> >the dmabuf
>> >> >
>> >> >On Fri, 12 May 2017 11:12:05 +0200 Gerd Hoffmann
>> >> ><kraxel@redhat.com> wrote:
>> >> >
>> >> >>   Hi,
>> >> >>
>> >> >> > If the contents of the framebuffer change or if the parameters
>> >> >> > of the framebuffer change?  I can't image that creating a new
>> >> >> > dmabuf fd for every visual change within the framebuffer would
>> >> >> > be efficient, but I don't have any concept of what a dmabuf actually
>does.
>> >> >>
>> >> >> Ok, some background:
>> >> >>
>> >> >> The drm subsystem has the concept of planes.  The most important
>> >> >> plane is the primary framebuffer (i.e. what gets scanned out to
>> >> >> the physical display).  The cursor is a plane too, and there can
>> >> >> be additional overlay planes for stuff like video playback.
>> >> >>
>> >> >> Typically there are multiple planes in a system and only one of
>> >> >> them gets scanned out to the crtc, i.e. the fbdev emulation
>> >> >> creates one plane for the framebuffer console.  The X-Server
>> >> >> creates a plane too, and when you switch between X-Server and
>> >> >> framebuffer console via ctrl-alt-fn the intel driver just
>> >> >> reprograms the encoder to scan out the one or the other plane to the crtc.
>> >> >>
>> >> >> The dma-buf handed out by gvt is a reference to a plane.  I
>> >> >> think on the host side gvt can see only the active plane (from
>> >> >> encoder/crtc register
>> >> >> programming) not the inactive ones.
>> >> >>
>> >> >> The dma-buf can be imported as opengl texture and then be used
>> >> >> to render the guest display to a host window.  I think it is
>> >> >> even possible to use the dma-buf as plane in the host drm driver
>> >> >> and scan it out directly to a physical display.  The actual
>> >> >> framebuffer content stays in gpu memory all the time, the cpu never has
>to touch it.
>> >> >>
>> >> >> It is possible to cache the dma-buf handles, i.e. when the guest
>> >> >> boots you'll get the first for the fbcon plane, when the
>> >> >> x-server starts the second for the x-server framebuffer, and
>> >> >> when the user switches to the text console via ctrl-alt-fn you
>> >> >> can re-use the fbcon dma-buf you already have.
>> >> >>
>> >> >> The caching becomes more important for good performance when the
>> >> >> guest uses pageflipping (wayland does): define two planes,
>> >> >> render into one while displaying the other, then flip the two
>> >> >> for a atomic display update.
>> >> >>
>> >> >> The caching also makes it a bit difficult to create a good interface.
>> >> >> So, the current patch set creates:
>> >> >>
>> >> >>   (a) A way to query the active planes (ioctl
>> >> >>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
>> >> >>   (b) A way to create a dma-buf for the active plane (ioctl
>> >> >>       INTEL_VGPU_GENERATE_DMABUF).
>> >> >>
>> >> >> Typical userspace workflow is to first query the plane, then
>> >> >> check if it already has a dma-buf for it, and if not create one.
>> >> >
>> >> >Thank you!  This is immensely helpful!
>> >> >
>> >> >> > What changes to the framebuffer require a new dmabuf fd?
>> >> >> > Shouldn't the user query the parameters of the framebuffer
>> >> >> > through a dmabuf fd and shouldn't the dmabuf fd have some
>> >> >> > signaling mechanism to the user (eventfd perhaps) to notify
>> >> >> > the user to re-
>> >evaluate the parameters?
>> >> >>
>> >> >> dma-bufs don't support that, they are really just a handle to a
>> >> >> piece of memory, all metadata (format, size) most be
>> >> >> communicated by
>> >other means.
>> >> >>
>> >> >> > Otherwise are you imagining that the user polls the vfio region?
>> >> >>
>> >> >> Hmm, notification support would probably a good reason to have a
>> >> >> separate file handle to manage the dma-bufs (instead of using
>> >> >> driver-specific ioctls on the vfio fd), because the driver could
>> >> >> also use the management fd for notifications then.
>> >> >
>> >> >I like this idea of a separate control fd for dmabufs, it provides
>> >> >not only a central management point, but also a nice abstraction
>> >> >for the vfio device specific interface.  We potentially only need
>> >> >a single
>> >> >VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management
>> >> >fd (perhaps with a type parameter, ex. GFX) where maybe we could
>> >> >have vfio-core incorporate this reference into the group
>> >> >lifecycle, so the vendor driver only needs to fdget/put this
>> >> >manager fd for the various plane dmabuf fds spawned in order to get core-
>level reference counting.
>> >> Following is my understanding of the management fd idea:
>> >> 1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to create a
>> >> fd
>> >and saved the fd in vfio group while initializing the vfio.
>> >
>> >Ideally there'd be kernel work here too if we want vfio-core to
>> >incorporate lifecycle of this fd into the device/group/container
>> >lifecycle.  Maybe we even want to generalize it further to something
>> >like VFIO_DEVICE_GET_FD which takes a parameter of what type of FD to
>> >get, GFX_DMABUF_MGR_FD in this case.  vfio- core would probably
>> >allocate the fd, tap into the release hook for reference counting and
>> >pass it to the vfio_device_ops (mdev vendor driver in this case) to attach
>further.
>> I tried to implement this today and now it functionally worked.
>> I am still a little confuse of how to tap the fd into the release hook  of
>device/group/container.
>> I tried to create the fd in vfio core but found it is difficult to get the file
>operations for the fd, the file operations should be supplied by vendor drivers.
>
>I'm not fully convinced there's benefit to having vfio-core attempt to do this, I
>was just hoping to avoid each vendor driver needing to implement their own
>reference counting.  I don't think vfio-core wants to get into tracking each ioctl
>for each vendor specific fd type to know which create new references and which
>don't.  Perhaps we'll come up with easier ways to do this as we go.
Got it.

>
>> So the fd is created in kvmgt.c for now.
>> Below is part of the codes:
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 389f072..d0649ba 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -41,6 +41,7 @@
>>  #include <linux/kvm_host.h>
>>  #include <linux/vfio.h>
>>  #include <linux/mdev.h>
>> +#include <linux/anon_inodes.h>
>>
>>  #include "i915_drv.h"
>>  #include "gvt.h"
>> @@ -524,6 +525,63 @@ static int intel_vgpu_reg_init_opregion(struct
>intel_vgpu *vgpu)
>>         return ret;
>>  }
>>
>> +static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file, struct
>> +vm_area_struct *vma) {
>> +       WARN_ON(1);
>
>A user can abuse this, simply return error.
OK.

>
>> +
>> +       return 0;
>> +}
>> +
>> +static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode,
>> +struct file *filp) {
>> +       struct intel_vgpu *vgpu = filp->private_data;
>> +
>> +       if (vgpu->vdev.vfio_device != NULL)
>> +               vfio_device_put(vgpu->vdev.vfio_device);
>
>When does the case occur where we don't have a vfio_device?  This looks a bit
>like a warning flag that reference counting isn't handled properly.
This situation happen only when anonymous fd created successfully but error occur while trying to get the vfio_device.
We should return error while user space trying to create the management fd and print an error message while kernel release this fd.

>
>> +
>> +       return 0;
>> +}
>> +
>> +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
>> +               unsigned int ioctl, unsigned long arg) {
>> +       struct intel_vgpu *vgpu = filp->private_data;
>> +       int minsz;
>> +       struct intel_vgpu_dmabuf dmabuf;
>> +       int ret;
>> +       struct fd f;
>> +       f = fdget(dmabuf.fd);
>> +       minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
>> +       if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
>> +               return -EFAULT;
>> +       if (ioctl == INTEL_VGPU_QUERY_DMABUF)
>> +               ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
>> +       else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
>> +               ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu,
>> +&dmabuf);
>
>Why do we need vendor specific ioctls here?  Aren't querying the current plane
>and getting an fd for that plane very generic concepts?
>Is the resulting dmabuf Intel specific?
No. not Intel specific. Like Gerd said "Typical userspace workflow is to first query the plane, then 
check if it already has a dma-buf for it, and if not create one".
We first query the plane info(WITHOUT creating a fd).
User space need to check whether there's a dmabuf for the plane(user space usually cached two or three dmabuf to handle double buffer or triple buffer situation) only there's no dmabuf for the plane we will create a dmabuf for it(another ioctl).

>
>> +       else {
>> +               fdput(f);
>> +               gvt_vgpu_err("unsupported dmabuf operation\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (ret != 0) {
>> +               fdput(f);
>> +               gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
>> +               return -EINVAL;
>> +       }
>> +       fdput(f);
>> +
>> +       return copy_to_user((void __user *)arg, &dmabuf, minsz) ?
>> +-EFAULT : 0; }
>> +
>> +static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
>> +       .release        = intel_vgpu_dmabuf_mgr_fd_release,
>> +       .unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
>> +       .mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
>> +       .llseek         = noop_llseek,
>> +};
>> +
>>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device
>> *mdev)  {
>>         struct intel_vgpu *vgpu = NULL; @@ -1259,6 +1317,31 @@ static
>> long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
>>         } else if (cmd == VFIO_DEVICE_RESET) {
>>                 intel_gvt_ops->vgpu_reset(vgpu);
>>                 return 0;
>> +       } else if (cmd == VFIO_DEVICE_GET_FD) {
>> +               struct vfio_fd vfio_fd;
>> +               int fd;
>> +               struct vfio_device *device;
>> +
>> +               minsz = offsetofend(struct vfio_fd, fd);
>> +               if (copy_from_user(&vfio_fd, (void __user *)arg, minsz))
>> +                       return -EINVAL;
>> +
>> +               if (vfio_fd.argsz < minsz)
>> +                       return -EINVAL;
>> +
>> +               fd = anon_inode_getfd("vfio_dmabuf_mgr_fd",
>&intel_vgpu_dmabuf_mgr_fd_ops,
>> +                                       vgpu, O_RDWR | O_CLOEXEC);
>> +               if (fd < 0)
>> +                       return -EINVAL;
>> +
>> +               vfio_fd.fd = fd;
>> +               device = vfio_device_get_from_dev(mdev_dev(mdev));
>> +               if (device == NULL)
>> +                       gvt_vgpu_err("kvmgt: vfio device is null\n");
>> +               else
>> +                       vgpu->vdev.vfio_device = device;
>> +
>> +               return copy_to_user((void __user *)arg, &vfio_fd,
>> + minsz) ? -EFAULT : 0;
>>         }
>>
>>         return 0;
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 519eff3..98be2e0 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -484,6 +485,20 @@ struct vfio_pci_hot_reset {
>>
>>  #define VFIO_DEVICE_PCI_HOT_RESET      _IO(VFIO_TYPE, VFIO_BASE + 13)
>>
>> +/**
>> + * VFIO_DEVICE_GET_FD - _IOW(VFIO_TYPE, VFIO_BASE + 21, struct
>> +vfio_fd)
>> + *
>> + * Create a fd for a vfio device.
>> + * This fd can be used for various purpose.
>> + */
>> +struct vfio_fd {
>> +       __u32 argsz;
>> +       __u32 flags;
>> +       /* out */
>> +       __u32 fd;
>> +};
>> +#define VFIO_DEVICE_GET_FD     _IO(VFIO_TYPE, VFIO_BASE + 14)
>
>
>The idea was that we pass some sort of type to VFIO_DEVICE_GET_FD, for
>instance we might ask for a DEVICE_FD_GRAPHICS_DMABUF and the vfio bus
>driver (mdev vendor driver) would test whether it supports that type of thing and
>either return an fd or error.  We can return the fd the same way we do for
>VFIO_DEVICE_GET_FD.  For instance the user should do something like:
>
>dmabuf_fd = ioctl(device_fd,
>		VFIO_DEVICE_GET_FD, DEVICE_FD_GRAPHICS_DMABUF); if
>(dmabuf_fd < 0)
>	/* not supported... */
>else
>	/* do stuff */
OK. Got it.


>
>Thanks,
>Alex

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Thu, 18 May 2017 01:51:38 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Alex,
> 
> >-----Original Message-----
> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
> >Sent: Thursday, May 18, 2017 5:44 AM
> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> >Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>;
> >linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
> ><zhi.a.wang@intel.com>
> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >On Tue, 16 May 2017 10:16:28 +0000
> >"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
> >  
> >> Hi Alex,
> >>  
> >> >-----Original Message-----
> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
> >> >Sent: Tuesday, May 16, 2017 1:44 AM
> >> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> >> >Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin
> >> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org;
> >> >linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> >> ><zhiyuan.lv@intel.com>; intel-gvt- dev@lists.freedesktop.org; Wang,
> >> >Zhi A <zhi.a.wang@intel.com>
> >> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
> >> >dmabuf
> >> >
> >> >On Mon, 15 May 2017 03:36:50 +0000
> >> >"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
> >> >  
> >> >> Hi Alex and Gerd,
> >> >>  
> >> >> >-----Original Message-----
> >> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
> >> >> >Sent: Saturday, May 13, 2017 12:38 AM
> >> >> >To: Gerd Hoffmann <kraxel@redhat.com>
> >> >> >Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
> >> >> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >> >> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> >> >> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> >> >> >Zhi A <zhi.a.wang@intel.com>
> >> >> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting
> >> >> >the dmabuf
> >> >> >
> >> >> >On Fri, 12 May 2017 11:12:05 +0200 Gerd Hoffmann
> >> >> ><kraxel@redhat.com> wrote:
> >> >> >  
> >> >> >>   Hi,
> >> >> >>  
> >> >> >> > If the contents of the framebuffer change or if the parameters
> >> >> >> > of the framebuffer change?  I can't image that creating a new
> >> >> >> > dmabuf fd for every visual change within the framebuffer would
> >> >> >> > be efficient, but I don't have any concept of what a dmabuf actually  
> >does.  
> >> >> >>
> >> >> >> Ok, some background:
> >> >> >>
> >> >> >> The drm subsystem has the concept of planes.  The most important
> >> >> >> plane is the primary framebuffer (i.e. what gets scanned out to
> >> >> >> the physical display).  The cursor is a plane too, and there can
> >> >> >> be additional overlay planes for stuff like video playback.
> >> >> >>
> >> >> >> Typically there are multiple planes in a system and only one of
> >> >> >> them gets scanned out to the crtc, i.e. the fbdev emulation
> >> >> >> creates one plane for the framebuffer console.  The X-Server
> >> >> >> creates a plane too, and when you switch between X-Server and
> >> >> >> framebuffer console via ctrl-alt-fn the intel driver just
> >> >> >> reprograms the encoder to scan out the one or the other plane to the crtc.
> >> >> >>
> >> >> >> The dma-buf handed out by gvt is a reference to a plane.  I
> >> >> >> think on the host side gvt can see only the active plane (from
> >> >> >> encoder/crtc register
> >> >> >> programming) not the inactive ones.
> >> >> >>
> >> >> >> The dma-buf can be imported as opengl texture and then be used
> >> >> >> to render the guest display to a host window.  I think it is
> >> >> >> even possible to use the dma-buf as plane in the host drm driver
> >> >> >> and scan it out directly to a physical display.  The actual
> >> >> >> framebuffer content stays in gpu memory all the time, the cpu never has  
> >to touch it.  
> >> >> >>
> >> >> >> It is possible to cache the dma-buf handles, i.e. when the guest
> >> >> >> boots you'll get the first for the fbcon plane, when the
> >> >> >> x-server starts the second for the x-server framebuffer, and
> >> >> >> when the user switches to the text console via ctrl-alt-fn you
> >> >> >> can re-use the fbcon dma-buf you already have.
> >> >> >>
> >> >> >> The caching becomes more important for good performance when the
> >> >> >> guest uses pageflipping (wayland does): define two planes,
> >> >> >> render into one while displaying the other, then flip the two
> >> >> >> for a atomic display update.
> >> >> >>
> >> >> >> The caching also makes it a bit difficult to create a good interface.
> >> >> >> So, the current patch set creates:
> >> >> >>
> >> >> >>   (a) A way to query the active planes (ioctl
> >> >> >>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
> >> >> >>   (b) A way to create a dma-buf for the active plane (ioctl
> >> >> >>       INTEL_VGPU_GENERATE_DMABUF).
> >> >> >>
> >> >> >> Typical userspace workflow is to first query the plane, then
> >> >> >> check if it already has a dma-buf for it, and if not create one.  
> >> >> >
> >> >> >Thank you!  This is immensely helpful!
> >> >> >  
> >> >> >> > What changes to the framebuffer require a new dmabuf fd?
> >> >> >> > Shouldn't the user query the parameters of the framebuffer
> >> >> >> > through a dmabuf fd and shouldn't the dmabuf fd have some
> >> >> >> > signaling mechanism to the user (eventfd perhaps) to notify
> >> >> >> > the user to re-  
> >> >evaluate the parameters?  
> >> >> >>
> >> >> >> dma-bufs don't support that, they are really just a handle to a
> >> >> >> piece of memory, all metadata (format, size) most be
> >> >> >> communicated by  
> >> >other means.  
> >> >> >>  
> >> >> >> > Otherwise are you imagining that the user polls the vfio region?  
> >> >> >>
> >> >> >> Hmm, notification support would probably a good reason to have a
> >> >> >> separate file handle to manage the dma-bufs (instead of using
> >> >> >> driver-specific ioctls on the vfio fd), because the driver could
> >> >> >> also use the management fd for notifications then.  
> >> >> >
> >> >> >I like this idea of a separate control fd for dmabufs, it provides
> >> >> >not only a central management point, but also a nice abstraction
> >> >> >for the vfio device specific interface.  We potentially only need
> >> >> >a single
> >> >> >VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management
> >> >> >fd (perhaps with a type parameter, ex. GFX) where maybe we could
> >> >> >have vfio-core incorporate this reference into the group
> >> >> >lifecycle, so the vendor driver only needs to fdget/put this
> >> >> >manager fd for the various plane dmabuf fds spawned in order to get core-  
> >level reference counting.  
> >> >> Following is my understanding of the management fd idea:
> >> >> 1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to create a
> >> >> fd  
> >> >and saved the fd in vfio group while initializing the vfio.
> >> >
> >> >Ideally there'd be kernel work here too if we want vfio-core to
> >> >incorporate lifecycle of this fd into the device/group/container
> >> >lifecycle.  Maybe we even want to generalize it further to something
> >> >like VFIO_DEVICE_GET_FD which takes a parameter of what type of FD to
> >> >get, GFX_DMABUF_MGR_FD in this case.  vfio- core would probably
> >> >allocate the fd, tap into the release hook for reference counting and
> >> >pass it to the vfio_device_ops (mdev vendor driver in this case) to attach  
> >further.  
> >> I tried to implement this today and now it functionally worked.
> >> I am still a little confuse of how to tap the fd into the release hook  of  
> >device/group/container.  
> >> I tried to create the fd in vfio core but found it is difficult to get the file  
> >operations for the fd, the file operations should be supplied by vendor drivers.
> >
> >I'm not fully convinced there's benefit to having vfio-core attempt to do this, I
> >was just hoping to avoid each vendor driver needing to implement their own
> >reference counting.  I don't think vfio-core wants to get into tracking each ioctl
> >for each vendor specific fd type to know which create new references and which
> >don't.  Perhaps we'll come up with easier ways to do this as we go.  
> Got it.
> 
> >  
> >> So the fd is created in kvmgt.c for now.
> >> Below is part of the codes:
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> index 389f072..d0649ba 100644
> >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> @@ -41,6 +41,7 @@
> >>  #include <linux/kvm_host.h>
> >>  #include <linux/vfio.h>
> >>  #include <linux/mdev.h>
> >> +#include <linux/anon_inodes.h>
> >>
> >>  #include "i915_drv.h"
> >>  #include "gvt.h"
> >> @@ -524,6 +525,63 @@ static int intel_vgpu_reg_init_opregion(struct  
> >intel_vgpu *vgpu)  
> >>         return ret;
> >>  }
> >>
> >> +static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file, struct
> >> +vm_area_struct *vma) {
> >> +       WARN_ON(1);  
> >
> >A user can abuse this, simply return error.  
> OK.
> 
> >  
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode,
> >> +struct file *filp) {
> >> +       struct intel_vgpu *vgpu = filp->private_data;
> >> +
> >> +       if (vgpu->vdev.vfio_device != NULL)
> >> +               vfio_device_put(vgpu->vdev.vfio_device);  
> >
> >When does the case occur where we don't have a vfio_device?  This looks a bit
> >like a warning flag that reference counting isn't handled properly.  
> This situation happen only when anonymous fd created successfully but error occur while trying to get the vfio_device.
> We should return error while user space trying to create the management fd and print an error message while kernel release this fd.

I think the code should be re-ordered so the anon-fd is created last,
then we don't need to worry about this invalid release callback.

> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
> >> +               unsigned int ioctl, unsigned long arg) {
> >> +       struct intel_vgpu *vgpu = filp->private_data;
> >> +       int minsz;
> >> +       struct intel_vgpu_dmabuf dmabuf;
> >> +       int ret;
> >> +       struct fd f;
> >> +       f = fdget(dmabuf.fd);
> >> +       minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
> >> +       if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
> >> +               return -EFAULT;
> >> +       if (ioctl == INTEL_VGPU_QUERY_DMABUF)
> >> +               ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
> >> +       else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
> >> +               ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu,
> >> +&dmabuf);  
> >
> >Why do we need vendor specific ioctls here?  Aren't querying the current plane
> >and getting an fd for that plane very generic concepts?
> >Is the resulting dmabuf Intel specific?  
> No. not Intel specific. Like Gerd said "Typical userspace workflow is to first query the plane, then 
> check if it already has a dma-buf for it, and if not create one".
> We first query the plane info(WITHOUT creating a fd).
> User space need to check whether there's a dmabuf for the plane(user space usually cached two or three dmabuf to handle double buffer or triple buffer situation) only there's no dmabuf for the plane we will create a dmabuf for it(another ioctl).

If our ioctls are "Query current plane" and "Give me a dmabuf for
current plane", isn't that racey?  The current plane could have changed
between those two calls so the user doesn't absolutely know which plane
the dmabuf retrieved is for.  The "Give me a dmabuf" therefore needs to
take some sort of plane index so the user can request a specific
plane.  The vendor driver might decide to return -EAGAIN if asking for
the non-current plane, but we need that determinism built into the
ioctl interface.  Thanks,

Alex

> >> +       else {
> >> +               fdput(f);
> >> +               gvt_vgpu_err("unsupported dmabuf operation\n");
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       if (ret != 0) {
> >> +               fdput(f);
> >> +               gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
> >> +               return -EINVAL;
> >> +       }
> >> +       fdput(f);
> >> +
> >> +       return copy_to_user((void __user *)arg, &dmabuf, minsz) ?
> >> +-EFAULT : 0; }
> >> +
> >> +static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
> >> +       .release        = intel_vgpu_dmabuf_mgr_fd_release,
> >> +       .unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
> >> +       .mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
> >> +       .llseek         = noop_llseek,
> >> +};
> >> +
> >>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device
> >> *mdev)  {
> >>         struct intel_vgpu *vgpu = NULL; @@ -1259,6 +1317,31 @@ static
> >> long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
> >>         } else if (cmd == VFIO_DEVICE_RESET) {
> >>                 intel_gvt_ops->vgpu_reset(vgpu);
> >>                 return 0;
> >> +       } else if (cmd == VFIO_DEVICE_GET_FD) {
> >> +               struct vfio_fd vfio_fd;
> >> +               int fd;
> >> +               struct vfio_device *device;
> >> +
> >> +               minsz = offsetofend(struct vfio_fd, fd);
> >> +               if (copy_from_user(&vfio_fd, (void __user *)arg, minsz))
> >> +                       return -EINVAL;
> >> +
> >> +               if (vfio_fd.argsz < minsz)
> >> +                       return -EINVAL;
> >> +
> >> +               fd = anon_inode_getfd("vfio_dmabuf_mgr_fd",  
> >&intel_vgpu_dmabuf_mgr_fd_ops,  
> >> +                                       vgpu, O_RDWR | O_CLOEXEC);
> >> +               if (fd < 0)
> >> +                       return -EINVAL;
> >> +
> >> +               vfio_fd.fd = fd;
> >> +               device = vfio_device_get_from_dev(mdev_dev(mdev));
> >> +               if (device == NULL)
> >> +                       gvt_vgpu_err("kvmgt: vfio device is null\n");
> >> +               else
> >> +                       vgpu->vdev.vfio_device = device;
> >> +
> >> +               return copy_to_user((void __user *)arg, &vfio_fd,
> >> + minsz) ? -EFAULT : 0;
> >>         }
> >>
> >>         return 0;
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index 519eff3..98be2e0 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -484,6 +485,20 @@ struct vfio_pci_hot_reset {
> >>
> >>  #define VFIO_DEVICE_PCI_HOT_RESET      _IO(VFIO_TYPE, VFIO_BASE + 13)
> >>
> >> +/**
> >> + * VFIO_DEVICE_GET_FD - _IOW(VFIO_TYPE, VFIO_BASE + 21, struct
> >> +vfio_fd)
> >> + *
> >> + * Create a fd for a vfio device.
> >> + * This fd can be used for various purpose.
> >> + */
> >> +struct vfio_fd {
> >> +       __u32 argsz;
> >> +       __u32 flags;
> >> +       /* out */
> >> +       __u32 fd;
> >> +};
> >> +#define VFIO_DEVICE_GET_FD     _IO(VFIO_TYPE, VFIO_BASE + 14)  
> >
> >
> >The idea was that we pass some sort of type to VFIO_DEVICE_GET_FD, for
> >instance we might ask for a DEVICE_FD_GRAPHICS_DMABUF and the vfio bus
> >driver (mdev vendor driver) would test whether it supports that type of thing and
> >either return an fd or error.  We can return the fd the same way we do for
> >VFIO_DEVICE_GET_FD.  For instance the user should do something like:
> >
> >dmabuf_fd = ioctl(device_fd,
> >		VFIO_DEVICE_GET_FD, DEVICE_FD_GRAPHICS_DMABUF); if
> >(dmabuf_fd < 0)
> >	/* not supported... */
> >else
> >	/* do stuff */  
> OK. Got it.
> 
> 
> >
> >Thanks,
> >Alex  

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamson@redhat.com]
>Sent: Thursday, May 18, 2017 10:56 PM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin <kevin.tian@intel.com>;
>linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
><zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>On Thu, 18 May 2017 01:51:38 +0000
>"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
>
>> Hi Alex,
>>
>> >-----Original Message-----
>> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> >Sent: Thursday, May 18, 2017 5:44 AM
>> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>> >Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin
>> ><kevin.tian@intel.com>; linux-kernel@vger.kernel.org;
>> >zhenyuw@linux.intel.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>;
>> >intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A
>> ><zhi.a.wang@intel.com>
>> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the
>> >dmabuf
>> >
>> >On Tue, 16 May 2017 10:16:28 +0000
>> >"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:
>> >
>> >> Hi Alex,
>> >>
>> >> >-----Original Message-----
>> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> >> >Sent: Tuesday, May 16, 2017 1:44 AM
>> >> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>> >> >Cc: Gerd Hoffmann <kraxel@redhat.com>; Tian, Kevin
>> >> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org;
>> >> >linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
>> >> ><zhiyuan.lv@intel.com>; intel-gvt- dev@lists.freedesktop.org;
>> >> >Wang, Zhi A <zhi.a.wang@intel.com>
>> >> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting
>> >> >the dmabuf
>> >> >
>> >> >On Mon, 15 May 2017 03:36:50 +0000 "Chen, Xiaoguang"
>> >> ><xiaoguang.chen@intel.com> wrote:
>> >> >
>> >> >> Hi Alex and Gerd,
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> >> >> >Sent: Saturday, May 13, 2017 12:38 AM
>> >> >> >To: Gerd Hoffmann <kraxel@redhat.com>
>> >> >> >Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
>> >> >> ><kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>> >> >> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
>> >> >> ><zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org;
>> >> >> >Wang, Zhi A <zhi.a.wang@intel.com>
>> >> >> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting
>> >> >> >the dmabuf
>> >> >> >
>> >> >> >On Fri, 12 May 2017 11:12:05 +0200 Gerd Hoffmann
>> >> >> ><kraxel@redhat.com> wrote:
>> >> >> >
>> >> >> >>   Hi,
>> >> >> >>
>> >> >> >> > If the contents of the framebuffer change or if the
>> >> >> >> > parameters of the framebuffer change?  I can't image that
>> >> >> >> > creating a new dmabuf fd for every visual change within the
>> >> >> >> > framebuffer would be efficient, but I don't have any
>> >> >> >> > concept of what a dmabuf actually
>> >does.
>> >> >> >>
>> >> >> >> Ok, some background:
>> >> >> >>
>> >> >> >> The drm subsystem has the concept of planes.  The most
>> >> >> >> important plane is the primary framebuffer (i.e. what gets
>> >> >> >> scanned out to the physical display).  The cursor is a plane
>> >> >> >> too, and there can be additional overlay planes for stuff like video
>playback.
>> >> >> >>
>> >> >> >> Typically there are multiple planes in a system and only one
>> >> >> >> of them gets scanned out to the crtc, i.e. the fbdev
>> >> >> >> emulation creates one plane for the framebuffer console.  The
>> >> >> >> X-Server creates a plane too, and when you switch between
>> >> >> >> X-Server and framebuffer console via ctrl-alt-fn the intel
>> >> >> >> driver just reprograms the encoder to scan out the one or the other
>plane to the crtc.
>> >> >> >>
>> >> >> >> The dma-buf handed out by gvt is a reference to a plane.  I
>> >> >> >> think on the host side gvt can see only the active plane
>> >> >> >> (from encoder/crtc register
>> >> >> >> programming) not the inactive ones.
>> >> >> >>
>> >> >> >> The dma-buf can be imported as opengl texture and then be
>> >> >> >> used to render the guest display to a host window.  I think
>> >> >> >> it is even possible to use the dma-buf as plane in the host
>> >> >> >> drm driver and scan it out directly to a physical display.
>> >> >> >> The actual framebuffer content stays in gpu memory all the
>> >> >> >> time, the cpu never has
>> >to touch it.
>> >> >> >>
>> >> >> >> It is possible to cache the dma-buf handles, i.e. when the
>> >> >> >> guest boots you'll get the first for the fbcon plane, when
>> >> >> >> the x-server starts the second for the x-server framebuffer,
>> >> >> >> and when the user switches to the text console via
>> >> >> >> ctrl-alt-fn you can re-use the fbcon dma-buf you already have.
>> >> >> >>
>> >> >> >> The caching becomes more important for good performance when
>> >> >> >> the guest uses pageflipping (wayland does): define two
>> >> >> >> planes, render into one while displaying the other, then flip
>> >> >> >> the two for a atomic display update.
>> >> >> >>
>> >> >> >> The caching also makes it a bit difficult to create a good interface.
>> >> >> >> So, the current patch set creates:
>> >> >> >>
>> >> >> >>   (a) A way to query the active planes (ioctl
>> >> >> >>       INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series).
>> >> >> >>   (b) A way to create a dma-buf for the active plane (ioctl
>> >> >> >>       INTEL_VGPU_GENERATE_DMABUF).
>> >> >> >>
>> >> >> >> Typical userspace workflow is to first query the plane, then
>> >> >> >> check if it already has a dma-buf for it, and if not create one.
>> >> >> >
>> >> >> >Thank you!  This is immensely helpful!
>> >> >> >
>> >> >> >> > What changes to the framebuffer require a new dmabuf fd?
>> >> >> >> > Shouldn't the user query the parameters of the framebuffer
>> >> >> >> > through a dmabuf fd and shouldn't the dmabuf fd have some
>> >> >> >> > signaling mechanism to the user (eventfd perhaps) to notify
>> >> >> >> > the user to re-
>> >> >evaluate the parameters?
>> >> >> >>
>> >> >> >> dma-bufs don't support that, they are really just a handle to
>> >> >> >> a piece of memory, all metadata (format, size) most be
>> >> >> >> communicated by
>> >> >other means.
>> >> >> >>
>> >> >> >> > Otherwise are you imagining that the user polls the vfio region?
>> >> >> >>
>> >> >> >> Hmm, notification support would probably a good reason to
>> >> >> >> have a separate file handle to manage the dma-bufs (instead
>> >> >> >> of using driver-specific ioctls on the vfio fd), because the
>> >> >> >> driver could also use the management fd for notifications then.
>> >> >> >
>> >> >> >I like this idea of a separate control fd for dmabufs, it
>> >> >> >provides not only a central management point, but also a nice
>> >> >> >abstraction for the vfio device specific interface.  We
>> >> >> >potentially only need a single
>> >> >> >VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf
>> >> >> >management fd (perhaps with a type parameter, ex. GFX) where
>> >> >> >maybe we could have vfio-core incorporate this reference into
>> >> >> >the group lifecycle, so the vendor driver only needs to
>> >> >> >fdget/put this manager fd for the various plane dmabuf fds
>> >> >> >spawned in order to get core-
>> >level reference counting.
>> >> >> Following is my understanding of the management fd idea:
>> >> >> 1) QEMU will call VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to
>> >> >> create a fd
>> >> >and saved the fd in vfio group while initializing the vfio.
>> >> >
>> >> >Ideally there'd be kernel work here too if we want vfio-core to
>> >> >incorporate lifecycle of this fd into the device/group/container
>> >> >lifecycle.  Maybe we even want to generalize it further to
>> >> >something like VFIO_DEVICE_GET_FD which takes a parameter of what
>> >> >type of FD to get, GFX_DMABUF_MGR_FD in this case.  vfio- core
>> >> >would probably allocate the fd, tap into the release hook for
>> >> >reference counting and pass it to the vfio_device_ops (mdev vendor
>> >> >driver in this case) to attach
>> >further.
>> >> I tried to implement this today and now it functionally worked.
>> >> I am still a little confuse of how to tap the fd into the release
>> >> hook  of
>> >device/group/container.
>> >> I tried to create the fd in vfio core but found it is difficult to
>> >> get the file
>> >operations for the fd, the file operations should be supplied by vendor drivers.
>> >
>> >I'm not fully convinced there's benefit to having vfio-core attempt
>> >to do this, I was just hoping to avoid each vendor driver needing to
>> >implement their own reference counting.  I don't think vfio-core
>> >wants to get into tracking each ioctl for each vendor specific fd
>> >type to know which create new references and which don't.  Perhaps we'll
>come up with easier ways to do this as we go.
>> Got it.
>>
>> >
>> >> So the fd is created in kvmgt.c for now.
>> >> Below is part of the codes:
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> >> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> >> index 389f072..d0649ba 100644
>> >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> >> @@ -41,6 +41,7 @@
>> >>  #include <linux/kvm_host.h>
>> >>  #include <linux/vfio.h>
>> >>  #include <linux/mdev.h>
>> >> +#include <linux/anon_inodes.h>
>> >>
>> >>  #include "i915_drv.h"
>> >>  #include "gvt.h"
>> >> @@ -524,6 +525,63 @@ static int intel_vgpu_reg_init_opregion(struct
>> >intel_vgpu *vgpu)
>> >>         return ret;
>> >>  }
>> >>
>> >> +static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file, struct
>> >> +vm_area_struct *vma) {
>> >> +       WARN_ON(1);
>> >
>> >A user can abuse this, simply return error.
>> OK.
>>
>> >
>> >> +
>> >> +       return 0;
>> >> +}
>> >> +
>> >> +static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode,
>> >> +struct file *filp) {
>> >> +       struct intel_vgpu *vgpu = filp->private_data;
>> >> +
>> >> +       if (vgpu->vdev.vfio_device != NULL)
>> >> +               vfio_device_put(vgpu->vdev.vfio_device);
>> >
>> >When does the case occur where we don't have a vfio_device?  This
>> >looks a bit like a warning flag that reference counting isn't handled properly.
>> This situation happen only when anonymous fd created successfully but error
>occur while trying to get the vfio_device.
>> We should return error while user space trying to create the management fd
>and print an error message while kernel release this fd.
>
>I think the code should be re-ordered so the anon-fd is created last, then we don't
>need to worry about this invalid release callback.
OK. Will try to get the vfio device first and create the fd only when successfully get the vfio device.

>
>> >> +
>> >> +       return 0;
>> >> +}
>> >> +
>> >> +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
>> >> +               unsigned int ioctl, unsigned long arg) {
>> >> +       struct intel_vgpu *vgpu = filp->private_data;
>> >> +       int minsz;
>> >> +       struct intel_vgpu_dmabuf dmabuf;
>> >> +       int ret;
>> >> +       struct fd f;
>> >> +       f = fdget(dmabuf.fd);
>> >> +       minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
>> >> +       if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
>> >> +               return -EFAULT;
>> >> +       if (ioctl == INTEL_VGPU_QUERY_DMABUF)
>> >> +               ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
>> >> +       else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
>> >> +               ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu,
>> >> +&dmabuf);
>> >
>> >Why do we need vendor specific ioctls here?  Aren't querying the
>> >current plane and getting an fd for that plane very generic concepts?
>> >Is the resulting dmabuf Intel specific?
>> No. not Intel specific. Like Gerd said "Typical userspace workflow is
>> to first query the plane, then check if it already has a dma-buf for it, and if not
>create one".
>> We first query the plane info(WITHOUT creating a fd).
>> User space need to check whether there's a dmabuf for the plane(user space
>usually cached two or three dmabuf to handle double buffer or triple buffer
>situation) only there's no dmabuf for the plane we will create a dmabuf for
>it(another ioctl).
>
>If our ioctls are "Query current plane" and "Give me a dmabuf for current plane",
>isn't that racey?  
Yes. Indeed there is such potential problem. so when user request "Give me a dmabuf for current plane" a dmabuf for the "current" plane("maybe" not the only we queried before) is created. 
In the implementation I did not use the plane info while querying the plane but decode the current plane again to get the current plane information. This maybe not so efficient but can reduce the racey.

>The current plane could have changed between those two calls
>so the user doesn't absolutely know which plane the dmabuf retrieved is for.  The
>"Give me a dmabuf" therefore needs to take some sort of plane index so the user
>can request a specific plane.  The vendor driver might decide to return -EAGAIN if
>asking for the non-current plane, but we need that determinism built into the ioctl
>interface.  
Only adding an index can not fix this problem. 
If you check the whole v2 patch you will find we have to decode the current plane to get the plane information.
I tried two methods while coding:

Method1:
OP1: When user ioctl"query current plane" kernel will decode the current plane and return the information to user.
OP2: When user ioctl"Give me a dmabuf for current plane" kernel will decode the current plane again and use the decoded plane information to create a dma-buf. 

Method2:
OP1: when user ioctl "query current plane" kernel will decode the current plane and save the result into vgpu and return the plane information to user.
OP2: when user ioctl "Give me a dmabuf" kernel just use the saved plane information to create a dmabuf.

While creating the dmabuf we need the plane's gma(graphics memory address, gma got while decoding the plane) to traverse the GTT(Graphics Translation Table) to get the physical address of the plane.
In theory there is possibility that the plane has changed while we traverse the GTT(there is intervals between decode the plane and traverse the GTT).

There is no difference while testing the above two methods.

>Thanks,
>
>Alex
>
>> >> +       else {
>> >> +               fdput(f);
>> >> +               gvt_vgpu_err("unsupported dmabuf operation\n");
>> >> +               return -EINVAL;
>> >> +       }
>> >> +
>> >> +       if (ret != 0) {
>> >> +               fdput(f);
>> >> +               gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
>> >> +               return -EINVAL;
>> >> +       }
>> >> +       fdput(f);
>> >> +
>> >> +       return copy_to_user((void __user *)arg, &dmabuf, minsz) ?
>> >> +-EFAULT : 0; }
>> >> +
>> >> +static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
>> >> +       .release        = intel_vgpu_dmabuf_mgr_fd_release,
>> >> +       .unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
>> >> +       .mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
>> >> +       .llseek         = noop_llseek,
>> >> +};
>> >> +
>> >>  static int intel_vgpu_create(struct kobject *kobj, struct
>> >> mdev_device
>> >> *mdev)  {
>> >>         struct intel_vgpu *vgpu = NULL; @@ -1259,6 +1317,31 @@
>> >> static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
>> >>         } else if (cmd == VFIO_DEVICE_RESET) {
>> >>                 intel_gvt_ops->vgpu_reset(vgpu);
>> >>                 return 0;
>> >> +       } else if (cmd == VFIO_DEVICE_GET_FD) {
>> >> +               struct vfio_fd vfio_fd;
>> >> +               int fd;
>> >> +               struct vfio_device *device;
>> >> +
>> >> +               minsz = offsetofend(struct vfio_fd, fd);
>> >> +               if (copy_from_user(&vfio_fd, (void __user *)arg, minsz))
>> >> +                       return -EINVAL;
>> >> +
>> >> +               if (vfio_fd.argsz < minsz)
>> >> +                       return -EINVAL;
>> >> +
>> >> +               fd = anon_inode_getfd("vfio_dmabuf_mgr_fd",
>> >&intel_vgpu_dmabuf_mgr_fd_ops,
>> >> +                                       vgpu, O_RDWR | O_CLOEXEC);
>> >> +               if (fd < 0)
>> >> +                       return -EINVAL;
>> >> +
>> >> +               vfio_fd.fd = fd;
>> >> +               device = vfio_device_get_from_dev(mdev_dev(mdev));
>> >> +               if (device == NULL)
>> >> +                       gvt_vgpu_err("kvmgt: vfio device is null\n");
>> >> +               else
>> >> +                       vgpu->vdev.vfio_device = device;
>> >> +
>> >> +               return copy_to_user((void __user *)arg, &vfio_fd,
>> >> + minsz) ? -EFAULT : 0;
>> >>         }
>> >>
>> >>         return 0;
>> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> >> index 519eff3..98be2e0 100644
>> >> --- a/include/uapi/linux/vfio.h
>> >> +++ b/include/uapi/linux/vfio.h
>> >> @@ -484,6 +485,20 @@ struct vfio_pci_hot_reset {
>> >>
>> >>  #define VFIO_DEVICE_PCI_HOT_RESET      _IO(VFIO_TYPE, VFIO_BASE + 13)
>> >>
>> >> +/**
>> >> + * VFIO_DEVICE_GET_FD - _IOW(VFIO_TYPE, VFIO_BASE + 21, struct
>> >> +vfio_fd)
>> >> + *
>> >> + * Create a fd for a vfio device.
>> >> + * This fd can be used for various purpose.
>> >> + */
>> >> +struct vfio_fd {
>> >> +       __u32 argsz;
>> >> +       __u32 flags;
>> >> +       /* out */
>> >> +       __u32 fd;
>> >> +};
>> >> +#define VFIO_DEVICE_GET_FD     _IO(VFIO_TYPE, VFIO_BASE + 14)
>> >
>> >
>> >The idea was that we pass some sort of type to VFIO_DEVICE_GET_FD,
>> >for instance we might ask for a DEVICE_FD_GRAPHICS_DMABUF and the
>> >vfio bus driver (mdev vendor driver) would test whether it supports
>> >that type of thing and either return an fd or error.  We can return
>> >the fd the same way we do for VFIO_DEVICE_GET_FD.  For instance the user
>should do something like:
>> >
>> >dmabuf_fd = ioctl(device_fd,
>> >		VFIO_DEVICE_GET_FD, DEVICE_FD_GRAPHICS_DMABUF); if
>(dmabuf_fd < 0)
>> >	/* not supported... */
>> >else
>> >	/* do stuff */
>> OK. Got it.
>>
>>
>> >
>> >Thanks,
>> >Alex

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> > User space need to check whether there's a dmabuf for the plane(user space usually cached two or three dmabuf to handle double buffer or triple buffer situation) only there's no dmabuf for the plane we will create a dmabuf for it(another ioctl).
> 
> If our ioctls are "Query current plane" and "Give me a dmabuf for
> current plane", isn't that racey?  The current plane could have changed
> between those two calls so the user doesn't absolutely know which plane
> the dmabuf retrieved is for.  The "Give me a dmabuf" therefore needs to
> take some sort of plane index so the user can request a specific
> plane.

The "give me a dmabuf" ioctl returns the plane description too, so
userspace can at least figure it did hit the race window.

We could also do it the other way around:  Instead of having the kernel
returning the plane description userspace could pass it in, and the
kernel throws -EINVAL in case it doesn't match due to things having
changed meanwhile.

cheers,
  Gerd

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Gerd and Alex,

>-----Original Message-----
>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>Sent: Friday, May 19, 2017 4:05 PM
>To: Alex Williamson <alex.williamson@redhat.com>
>Cc: Chen, Xiaoguang <xiaoguang.chen@intel.com>; Tian, Kevin
><kevin.tian@intel.com>; linux-kernel@vger.kernel.org; zhenyuw@linux.intel.com;
>Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
>Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>  Hi,
>
>> > User space need to check whether there's a dmabuf for the plane(user space
>usually cached two or three dmabuf to handle double buffer or triple buffer
>situation) only there's no dmabuf for the plane we will create a dmabuf for
>it(another ioctl).
>>
>> If our ioctls are "Query current plane" and "Give me a dmabuf for
>> current plane", isn't that racey?  The current plane could have
>> changed between those two calls so the user doesn't absolutely know
>> which plane the dmabuf retrieved is for.  The "Give me a dmabuf"
>> therefore needs to take some sort of plane index so the user can
>> request a specific plane.
>
>The "give me a dmabuf" ioctl returns the plane description too, so userspace can
>at least figure it did hit the race window.
>
>We could also do it the other way around:  Instead of having the kernel returning
>the plane description userspace could pass it in, and the kernel throws -EINVAL in
>case it doesn't match due to things having changed meanwhile.
Or just return a dmabuf  based on the current plane ?
Because if user got -EINVAL while ioctl "Give me a dmabuf" user should do the ioctl again.

>
>cheers,
>  Gerd

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> >We could also do it the other way around:  Instead of having the kernel returning
> 
> >the plane description userspace could pass it in, and the kernel throws -EINVAL in
> 
> >case it doesn't match due to things having changed meanwhile.
> 
> Or just return a dmabuf  based on the current plane ?

If gvt is able to hand out dma-bufs for inactive planes, then yes, we
can do this.  Have one ioctl where we pass in the plane id, get back a
plane description struct, and another where we pass in the plane
description struct and get back a dma-buf fd.

cheers,
  Gerd

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Gerd,

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Gerd Hoffmann
>Sent: Friday, May 19, 2017 4:57 PM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; linux-kernel@vger.kernel.org;
>zhenyuw@linux.intel.com; Alex Williamson <alex.williamson@redhat.com>; Lv,
>Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi
>A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>  Hi,
>
>> >We could also do it the other way around:  Instead of having the
>> >kernel returning
>>
>> >the plane description userspace could pass it in, and the kernel
>> >throws -EINVAL in
>>
>> >case it doesn't match due to things having changed meanwhile.
>>
>> Or just return a dmabuf  based on the current plane ?
>
>If gvt is able to hand out dma-bufs for inactive planes, then yes, we can do this.
>Have one ioctl where we pass in the plane id, get back a plane description struct,
>and another where we pass in the plane description struct and get back a dma-
>buf fd.
Or more simply just pass the plane id, because even the plane description did not match the current one we will eventually create a dmabuf based on current plane.

>
>cheers,
>  Gerd
>
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> Or more simply just pass the plane id, because even the plane description did not match the current one we will eventually create a dmabuf based on current plane.

That is the current behavior.

Works as long as we return the plane description too, so userspace knows
what it actually got.

cheers,
  Gerd

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Gerd Hoffmann]

  Hi,

> > +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
> > +               unsigned int ioctl, unsigned long arg)
> > +{
> > +       struct intel_vgpu *vgpu = filp->private_data;
> > +       int minsz;
> > +       struct intel_vgpu_dmabuf dmabuf;
> > +       int ret;
> > +       struct fd f;
> > +       f = fdget(dmabuf.fd);
> > +       minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
> > +       if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
> > +               return -EFAULT;
> > +       if (ioctl == INTEL_VGPU_QUERY_DMABUF)
> > +               ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
> > +       else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
> > +               ret = intel_gvt_ops->vgpu_generate_dmabuf(vgpu, &dmabuf);
> 
> Why do we need vendor specific ioctls here?  Aren't querying the
> current plane and getting an fd for that plane very generic concepts? 
> Is the resulting dmabuf Intel specific?

The dmabuf certainly isn't, and I think the ioctl interface can easily
be made pretty generic too.  The struct intel_vgpu_dmabuf hasn't much
intel-specific bits in there.  The only thing is the tiled bool, and I
think that can be replaced with a modifier code field (see
fourcc_mod_code() in drm/drm_fourcc.h).

Apparently the amd guys are working on vcpu support too, but using sriov
instead of mdev (saw this in the news only, have no more details).  They
can possibly support such an interface too.

cheers,
  Gerd

RE: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Chen, Xiaoguang]

Hi Gerd,

>-----Original Message-----
>From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
>Behalf Of Gerd Hoffmann
>Sent: Thursday, May 11, 2017 9:28 PM
>To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
>Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
>kernel@vger.kernel.org; zhenyuw@linux.intel.com; Alex Williamson
><alex.williamson@redhat.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
>dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
>Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
>
>  Hi,
>
>> While read the framebuffer region we have to tell the vendor driver which
>framebuffer we want to read? There are two framebuffers now in KVMGT that is
>primary and cursor.
>> There are two methods to implement this:
>> 1) write the plane id first and then read the framebuffer.
>> 2) create 2 vfio regions one for primary and one for cursor.
>
>(3) Place information for both planes into one vfio region.
>    Which allows to fetch both with a single read() syscall.
That works too. Then using the ioctl to get the dmabuf fd if needed. And plane id can be ioctl's parameter.

How about method 2 primary plane and cursor plane are different and should generate different dmabuf for each of them.

>
>The question is how you'll get the file descriptor then.  If the ioctl returns the
>dma-buf fd only you have a racy interface:  Things can change between read(vfio-
>region) and ioctl(need-dmabuf-fd).
You are right. So when creating the dmabuf we may have to decode the framebuffer and create the dmabuf using the latest framebuffer information and we must return the framebuffer information together with the dmabuf fd.

In the current implementation I saved the framebuffer information while user querying the framebuffer and generate the dmabuf using the saved information no error found yet but in theory there are sync problems.

>
>ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix the race,
>but then it is easier to go with ioctl only interface (simliar to the orginal one from
>dec last year) I think.
Yes. ioctl works for it.
But based on the mail last week. If I understand correctly Alex hope to query the framebuffer information by reading the vfio device region and then get the dmabuf fd using ioctl.

>
>cheers,
>  Gerd
>
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

[Alex Williamson]

On Fri, 12 May 2017 06:56:03 +0000
"Chen, Xiaoguang" <xiaoguang.chen@intel.com> wrote:

> Hi Gerd,
> 
> >-----Original Message-----
> >From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> >Behalf Of Gerd Hoffmann
> >Sent: Thursday, May 11, 2017 9:28 PM
> >To: Chen, Xiaoguang <xiaoguang.chen@intel.com>
> >Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gfx@lists.freedesktop.org; linux-
> >kernel@vger.kernel.org; zhenyuw@linux.intel.com; Alex Williamson
> ><alex.williamson@redhat.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com>; intel-gvt-
> >dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
> >
> >  Hi,
> >  
> >> While read the framebuffer region we have to tell the vendor driver which  
> >framebuffer we want to read? There are two framebuffers now in KVMGT that is
> >primary and cursor.  
> >> There are two methods to implement this:
> >> 1) write the plane id first and then read the framebuffer.
> >> 2) create 2 vfio regions one for primary and one for cursor.  
> >
> >(3) Place information for both planes into one vfio region.
> >    Which allows to fetch both with a single read() syscall.  
> That works too. Then using the ioctl to get the dmabuf fd if needed. And plane id can be ioctl's parameter.
> 
> How about method 2 primary plane and cursor plane are different and should generate different dmabuf for each of them.
> 
> >
> >The question is how you'll get the file descriptor then.  If the ioctl returns the
> >dma-buf fd only you have a racy interface:  Things can change between read(vfio-
> >region) and ioctl(need-dmabuf-fd).  
> You are right. So when creating the dmabuf we may have to decode the framebuffer and create the dmabuf using the latest framebuffer information and we must return the framebuffer information together with the dmabuf fd.
> 
> In the current implementation I saved the framebuffer information while user querying the framebuffer and generate the dmabuf using the saved information no error found yet but in theory there are sync problems.
> 
> >
> >ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix the race,
> >but then it is easier to go with ioctl only interface (simliar to the orginal one from
> >dec last year) I think.  
> Yes. ioctl works for it.
> But based on the mail last week. If I understand correctly Alex hope to query the framebuffer information by reading the vfio device region and then get the dmabuf fd using ioctl.

No, I was explaining that I had questioned whether we could use a vfio
region in place of a separate dmabuf fd.  We can't.  I have no
particular desire to use a vfio region just for querying framebuffer
info.  I prefer the dmabuf manager fd idea that Gerd suggested.  Thanks,

Alex