drm/i915/gvt: Dma-buf support for GVT-g

PATCH Superseded Xiaoguang Chen 6 participants 14 messages May 18, 2017 – May 22, 2017

Subsystems (5)

DRM drivers DRM misc Intel i915 VFIO driver +1 more subsystems

lore

Share & tools

lore

Xiaoguang Chen

author

2017-05-18 09:50 UTC #0 lore raw

v2 cover drm/i915/gvt: Dma-buf support for GVT-g

v2->v1:
1) create a management fd for dma-buf operations.
2) alloc gem object's backing storage in gem obj's get_pages() callback.

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 user space. And user can use this dma-buf fd to do render or other operations.
User need to create a fd(for intel GVT-g dma-buf support it is a:dma-buf management fd) then user can use this fd to query the plane information or create a dma-buf. The life cycle of this fd is managed by GVT-g user do not need to care about that.

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

Xiaoguang Chen (5):
drm/i915/gvt: Extend the GVT-g architecture to support vfio device
region
drm/i915/gvt: OpRegion support for GVT-g
drm/i915/gvt: Frame buffer decoder support for GVT-g drm/i915/gvt: Dmabuf support for GVT-g
drm/i915/gvt: Adding interface so user space can get the dma-buf

 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     | 321 ++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h     |  44 +++
 drivers/gpu/drm/i915/gvt/fb_decoder.c | 487 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/fb_decoder.h | 166 ++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.c        |   2 +
 drivers/gpu/drm/i915/gvt/gvt.h        |   7 +
 drivers/gpu/drm/i915/gvt/kvmgt.c      | 207 ++++++++++++++-
 drivers/gpu/drm/i915/gvt/opregion.c   |  12 +-
 include/uapi/drm/i915_drm.h           |  29 ++
 include/uapi/linux/vfio.h             |  12 +
 13 files changed, 1287 insertions(+), 7 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

Signature · 1 line

1.9.1

Xiaoguang Chen

author

2017-05-18 09:50 UTC #1 lore raw

PATCH v2 1/5 drm/i915/gvt: Extend the GVT-g architecture to support vfio device region

Signed-off-by Xiaoguang Chen <[email protected]>

---
 drivers/gpu/drm/i915/gvt/kvmgt.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/gvt/kvmgt.c +18 -3

index 1ae0b40..3c6a02b 100644
@@ -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;

Signature · 1 line

1.9.1

Xiaoguang Chen

author

2017-05-18 09:50 UTC #2 lore raw

PATCH v2 2/5 drm/i915/gvt: OpRegion support for GVT-g

In reply to 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 <[email protected]>

Signed-off-by Xiaoguang Chen <[email protected]>

---
 drivers/gpu/drm/i915/gvt/kvmgt.c    | 97 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/opregion.c | 12 ++++-
 2 files changed, 107 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/gvt/kvmgt.c +97 -0

index 3c6a02b..389f072 100644
@@ -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;

drivers/gpu/drm/i915/gvt/opregion.c +10 -2

index 3117991..99591bc 100644
@@ -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) ||

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

He, Min

2017-05-18 15:43 UTC #3 lore raw

RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g

Xiaoguang,
I have some comments inline. Thanks.

Quoted 117 lines

-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Xiaoguang Chen
Sent: Thursday, May 18, 2017 2:50 AM
To: [email protected]; [email protected]; intel- [email protected]; [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt- [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang <[email protected]>
Subject: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g

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 <[email protected]>

Signed-off-by Xiaoguang Chen <[email protected]>

---
 drivers/gpu/drm/i915/gvt/kvmgt.c    | 97
+++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/opregion.c | 12 ++++-
 2 files changed, 107 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/gvt/kvmgt.c +2 -0

index 3c6a02b..389f072 100644
@@ -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 {

drivers/gpu/drm/i915/gvt/kvmgt.c +69 -0

 	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)) {

drivers/gpu/drm/i915/gvt/kvmgt.c static void kvmgt_protect_table_del(struct · line 520

Quoted 10 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +9 -0

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

In current GVT code, we already have a function init_vgpu_opregion, which will copy the physical opregion into a per vgpu structure:
vgpu_opregion(vgpu)->va, why don’t you reuse this function and set the vgpu_opregion(vgpu)->va to the intel_vgpu_register_reg function?

Quoted 41 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +8 -0

+	if (ret) {
+		memunmap(base);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)

{

drivers/gpu/drm/i915/gvt/kvmgt.c +9 -0

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

drivers/gpu/drm/i915/gvt/kvmgt.c +0 -0

 	ret = 0;

drivers/gpu/drm/i915/gvt/opregion.c +2 -0

index 3117991..99591bc 100644
@@ -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;

drivers/gpu/drm/i915/gvt/opregion.c int intel_vgpu_emulate_opregion_request(struct · line 297

Quoted 10 lines

drivers/gpu/drm/i915/gvt/opregion.c +6 -2

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

As I said, you can call init_vgpu_opregion(), then vgpu_opregion(vgpu)->va will be valid and you don’t need to change here.

Quoted 13 lines

drivers/gpu/drm/i915/gvt/opregion.c +2 -0

 	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 */

drivers/gpu/drm/i915/gvt/opregion.c +0 -1

 	if ((vgpu_cfg_space(vgpu)[INTEL_GVT_PCI_SWSCI]
 				& SWSCI_SCI_TRIGGER) ||
--

1.9.1

Quoted 4 lines

_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chen, Xiaoguang

author

2017-05-19 02:20 UTC #4 lore raw

RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g

Hi min,

Quoted 135 lines

-----Original Message-----
From: He, Min
Sent: Thursday, May 18, 2017 11:44 PM
To: Chen, Xiaoguang <[email protected]>; [email protected]; [email protected]; intel- [email protected]; [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt- [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang <[email protected]>
Subject: RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g
Xiaoguang,
I have some comments inline. Thanks.
Quoted 119 lines
-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Xiaoguang Chen
Sent: Thursday, May 18, 2017 2:50 AM
To: [email protected]; [email protected]; intel- [email protected]; [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt- [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang <[email protected]>
Subject: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g
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 <[email protected]>
Signed-off-by Xiaoguang Chen <[email protected]>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c    | 97
+++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/opregion.c | 12 ++++-
 2 files changed, 107 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/gvt/kvmgt.c +2 -0
index 3c6a02b..389f072 100644
@@ -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,
drivers/gpu/drm/i915/gvt/kvmgt.c +71 -0
 	}
 }
 
+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 |

Quoted 8 lines

drivers/gpu/drm/i915/gvt/kvmgt.c static void kvmgt_protect_table_del(struct kvmgt_guest_info *info, · line 518
Quoted 4 lines
VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
drivers/gpu/drm/i915/gvt/kvmgt.c +3 -0
+			VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
+			&intel_vgpu_regops_opregion, OPREGION_SIZE,
+			VFIO_REGION_INFO_FLAG_READ, base);
In current GVT code, we already have a function init_vgpu_opregion, which will copy the physical opregion into a per vgpu structure:
vgpu_opregion(vgpu)->va, why don’t you reuse this function and set the vgpu_opregion(vgpu)->va to the intel_vgpu_register_reg function?

The reason is we do not the know the gpa of the OpRegion. For KVMGT the gpa of the OpRegion is allocated by seabios.

Quoted 68 lines

Quoted 42 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +8 -0

+	if (ret) {
+		memunmap(base);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int intel_vgpu_create(struct kobject *kobj, struct mdev_device

*mdev) {

drivers/gpu/drm/i915/gvt/kvmgt.c +9 -0

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

drivers/gpu/drm/i915/gvt/kvmgt.c +0 -0

 	ret = 0;

drivers/gpu/drm/i915/gvt/opregion.c +2 -0

index 3117991..99591bc 100644
@@ -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;

drivers/gpu/drm/i915/gvt/opregion.c int intel_vgpu_emulate_opregion_request(struct · line 297

Quoted 10 lines

drivers/gpu/drm/i915/gvt/opregion.c +6 -2

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

As I said, you can call init_vgpu_opregion(), then vgpu_opregion(vgpu)->va will be valid and you don’t need to change here.

Quoted 13 lines

drivers/gpu/drm/i915/gvt/opregion.c +2 -0

 	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 */

drivers/gpu/drm/i915/gvt/opregion.c +0 -1

 	if ((vgpu_cfg_space(vgpu)[INTEL_GVT_PCI_SWSCI]
 				& SWSCI_SCI_TRIGGER) ||
--

1.9.1

Quoted 4 lines

Quoted 4 lines
_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

He, Min

2017-05-22 02:28 UTC #5 lore raw

RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g

Quoted 158 lines

-----Original Message-----
From: Chen, Xiaoguang
Sent: Thursday, May 18, 2017 7:20 PM
To: He, Min <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Cc: Niu, Bing <[email protected]>
Subject: RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g
Hi min,
Quoted 135 lines
-----Original Message-----
From: He, Min
Sent: Thursday, May 18, 2017 11:44 PM
To: Chen, Xiaoguang <[email protected]>; [email protected]; [email protected]; intel- [email protected]; [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt- [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang <[email protected]>
Subject: RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g
Xiaoguang,
I have some comments inline. Thanks.
Quoted 119 lines
-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Xiaoguang Chen
Sent: Thursday, May 18, 2017 2:50 AM
To: [email protected]; [email protected]; intel- [email protected]; [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt- [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang <[email protected]>
Subject: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g
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 <[email protected]>
Signed-off-by Xiaoguang Chen <[email protected]>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c    | 97
+++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/opregion.c | 12 ++++-
 2 files changed, 107 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/gvt/kvmgt.c +2 -0
index 3c6a02b..389f072 100644
@@ -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,
drivers/gpu/drm/i915/gvt/kvmgt.c +71 -0
 	}
 }
 
+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 |
Quoted 8 lines
drivers/gpu/drm/i915/gvt/kvmgt.c static void kvmgt_protect_table_del(struct kvmgt_guest_info *info, · line 518
Quoted 4 lines
VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
drivers/gpu/drm/i915/gvt/kvmgt.c +3 -0
+			VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
+			&intel_vgpu_regops_opregion, OPREGION_SIZE,
+			VFIO_REGION_INFO_FLAG_READ, base);
In current GVT code, we already have a function init_vgpu_opregion, which will copy the physical opregion into a per vgpu structure:
vgpu_opregion(vgpu)->va, why don’t you reuse this function and set the vgpu_opregion(vgpu)->va to the intel_vgpu_register_reg function?

Quoted 2 lines

The reason is we do not the know the gpa of the OpRegion. For KVMGT the gpa of the OpRegion is allocated by seabios.

Maybe you can consolidate them into one function to remove duplicate code.

Xiaoguang Chen

author

2017-05-18 09:50 UTC #6 lore raw

PATCH v2 3/5 drm/i915/gvt: Frame buffer decoder support for GVT-g

In reply to Xiaoguang Chen

decode frambuffer attributes of primary, cursor and sprite plane

Signed-off-by Xiaoguang Chen <[email protected]>

---
 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 | 166 ++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h        |   1 +
 include/uapi/drm/i915_drm.h           |   6 +
 7 files changed, 665 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

drivers/gpu/drm/i915/gvt/Makefile +2 -1

index b123c20..192ca26 100644
@@ -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))

drivers/gpu/drm/i915/gvt/display.c +1 -1

index e0261fc..f5f63c5 100644
@@ -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;

drivers/gpu/drm/i915/gvt/display.h +2 -0

index d73de22..b46b868 100644
@@ -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

drivers/gpu/drm/i915/gvt/fb_decoder.c new file +487 -0

487 lines not on this page · raw message

drivers/gpu/drm/i915/gvt/fb_decoder.h new file +166 -0

166 lines not on this page · raw message

drivers/gpu/drm/i915/gvt/gvt.h +1 -0

index 930732e..c42266c 100644
@@ -46,6 +46,7 @@
 #include "sched_policy.h"
 #include "render.h"
 #include "cmd_parser.h"
+#include "fb_decoder.h"
 
 #define GVT_MAX_VGPU 8

include/uapi/drm/i915_drm.h +6 -0

index f24a80d..ece8e2b 100644
@@ -1439,6 +1439,12 @@ enum drm_i915_perf_record_type {
 	DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };
 
+/** Intel GVT-g specific definitions
+ */
+#define INTEL_GVT_PLANE_PRIMARY		1
+#define INTEL_GVT_PLANE_SPRITE		2
+#define INTEL_GVT_PLANE_CURSOR		3
+
 #if defined(__cplusplus)
 }
 #endif

Signature · 1 line

1.9.1

Xiaoguang Chen

author

2017-05-18 09:50 UTC #7 lore raw

PATCH v2 4/5 drm/i915/gvt: Dmabuf support for GVT-g

In reply to 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 <[email protected]>

---
 drivers/gpu/drm/i915/gvt/Makefile |   2 +-
 drivers/gpu/drm/i915/gvt/dmabuf.c | 321 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/dmabuf.h |  44 ++++++
 drivers/gpu/drm/i915/gvt/gvt.h    |   3 +
 include/uapi/drm/i915_drm.h       |  21 +++
 5 files changed, 390 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.c
 create mode 100644 drivers/gpu/drm/i915/gvt/dmabuf.h

drivers/gpu/drm/i915/gvt/Makefile +1 -1

index 192ca26..e480f7d 100644
@@ -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))

drivers/gpu/drm/i915/gvt/dmabuf.c new file +321 -0

321 lines not on this page · raw message

drivers/gpu/drm/i915/gvt/dmabuf.h new file +44 -0

44 lines not on this page · raw message

drivers/gpu/drm/i915/gvt/gvt.h +3 -0

index c42266c..a553120 100644
@@ -47,6 +47,7 @@
 #include "render.h"
 #include "cmd_parser.h"
 #include "fb_decoder.h"
+#include "dmabuf.h"
 
 #define GVT_MAX_VGPU 8

@@ -186,6 +187,8 @@ struct intel_vgpu {
 		atomic_t released;
 	} vdev;
 #endif
+	struct intel_vgpu_plane_info *plane_info;
+	struct drm_i915_gem_object *obj_dmabuf;
 };
 
 struct intel_gvt_gm {

include/uapi/drm/i915_drm.h +21 -0

index ece8e2b..cde4f8e 100644
@@ -1445,6 +1445,27 @@ enum drm_i915_perf_record_type {
 #define INTEL_GVT_PLANE_SPRITE		2
 #define INTEL_GVT_PLANE_CURSOR		3
 
+/**
+ * Ioctl to query plane info or create dma-buf
+ */
+#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;
+};
+
 #if defined(__cplusplus)
 }
 #endif

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2017-05-18 10:27 UTC #8 lore raw

Re: [PATCH v2 4/5] drm/i915/gvt: Dmabuf support for GVT-g

Quoted 60 lines · Xiaoguang Chen

drivers/gpu/drm/i915/gvt/dmabuf.c new file +60 -0

+#define GEN8_DECODE_PTE(pte) \
+	((dma_addr_t)(((((u64)pte) >> 12) & 0x7ffffffULL) << 12))
+
+static struct sg_table *intel_vgpu_gem_get_pages(
+		struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+	struct intel_gvt *gvt = dev_priv->gvt;
+	struct sg_table *st;
+	struct scatterlist *sg;
+	int i, ret;
+	gen8_pte_t __iomem *gtt_entries;
+	struct intel_vgpu *vgpu;
+	unsigned int fb_gma = 0, fb_size = 0;
+	bool found = false;
+
+	mutex_lock(&gvt->lock);
+	for_each_active_vgpu(gvt, vgpu, i) {
+		if (vgpu->obj_dmabuf == obj) {
+			fb_gma = vgpu->plane_info->start;
+			fb_size = vgpu->plane_info->size;
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&gvt->lock);
+
+	if (!found) {
+		gvt_vgpu_err("no vgpu found\n");
+		return NULL;
+	}
+
+	st = kmalloc(sizeof(*st), GFP_KERNEL);
+	if (!st) {
+		ret = -ENOMEM;
+		return ERR_PTR(ret);
+	}
+
+	ret = sg_alloc_table(st, fb_size, GFP_KERNEL);
+	if (ret) {
+		kfree(st);
+		return ERR_PTR(ret);
+	}
+	gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
+		(fb_gma >> PAGE_SHIFT);
+	for_each_sg(st->sgl, sg, fb_size, i) {
+		sg->offset = 0;
+		sg->length = PAGE_SIZE;
+		sg_dma_address(sg) =
+			GEN8_DECODE_PTE(readq(&gtt_entries[i]));
+		sg_dma_len(sg) = PAGE_SIZE;
+	}
+
+	return st;
+}
+
+static void intel_vgpu_gem_put_pages(struct drm_i915_gem_object *obj,
+		struct sg_table *pages)
+{
+	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);

drivers/gpu/drm/i915/gvt/dmabuf.c line 105

Quoted 10 lines

drivers/gpu/drm/i915/gvt/dmabuf.c new file +10 -0

+	struct intel_gvt *gvt = dev_priv->gvt;
+	int i;
+	struct intel_vgpu *vgpu;
+
+	mutex_lock(&gvt->lock);
+	for_each_active_vgpu(gvt, vgpu, i) {
+		if (vgpu->obj_dmabuf == obj) {
+			kfree(vgpu->plane_info);
+			break;
+		}

We have a union for private data in i915_gem_object that you can use to store the backpointer.

drivers/gpu/drm/i915/gvt/dmabuf.c line 112

Quoted 7 lines

drivers/gpu/drm/i915/gvt/dmabuf.c new file +7 -0

+	}
+	mutex_unlock(&gvt->lock);
+
+	sg_free_table(pages);
+	kfree(pages);
+
+	i915_gem_object_unpin_pages(obj);

That's confused and should be triggering at least 2 WARNs when the object is freed.

Quoted 35 lines

drivers/gpu/drm/i915/gvt/dmabuf.c new file +35 -0

+}
+
+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,
+};
+
+static struct drm_i915_gem_object *intel_vgpu_create_gem(struct drm_device *dev,
+		struct intel_vgpu *vgpu)
+{
+	struct drm_i915_private *pri = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct intel_vgpu_plane_info *info = vgpu->plane_info;
+
+	obj = i915_gem_object_alloc(pri);
+	if (obj == NULL)
+		return NULL;
+
+	drm_gem_private_object_init(dev, &obj->base,
+		info->size << PAGE_SHIFT);
+	i915_gem_object_init(obj, &intel_vgpu_gem_ops);
+
+	obj->base.read_domains = I915_GEM_DOMAIN_GTT;
+	obj->base.write_domain = 0;
+
+	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;

drivers/gpu/drm/i915/gvt/dmabuf.c line 157

Quoted 10 lines

drivers/gpu/drm/i915/gvt/dmabuf.c new file +10 -0

+		case PLANE_CTL_TILED_Y:
+			tiling_mode = I915_TILING_Y;
+			break;
+		default:
+			gvt_vgpu_err("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 : 0;
+	}

How good a first class object is this? If I understand this correctly, we have a valid DMA mapping for the object, so we can access it via GTT mmaps and rendering on the GPU. We just can't use CPU mmaps.

However, we should reject things like tiling changes, cache level changes, set-domain is also problematic.

Quoted 16 lines

drivers/gpu/drm/i915/gvt/dmabuf.c new file +15 -0

+
+	return obj;
+}
 
+int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args)
+{
+	struct dma_buf *dmabuf;
+	struct drm_i915_gem_object *obj;
+	struct drm_device *dev = &vgpu->gvt->dev_priv->drm;
+	struct intel_vgpu_dmabuf *gvt_dmabuf = args;
+	struct intel_vgpu_plane_info *info;
+	int ret;
+
+	info = intel_vgpu_get_plane_info(dev, vgpu, gvt_dmabuf->plane_id);
+	if (info == NULL)
+		return -EINVAL;

drivers/gpu/drm/i915/gvt/dmabuf.c line 289

Quoted 10 lines

drivers/gpu/drm/i915/gvt/dmabuf.c new file +10 -0

+
+	vgpu->plane_info = info;
+	obj = intel_vgpu_create_gem(dev, vgpu);
+	if (obj == NULL) {
+		gvt_vgpu_err("create gvt gem obj failed:%d\n", vgpu->id);
+		return -EINVAL;
+	}
+	vgpu->obj_dmabuf = obj;
+
+	ret = i915_gem_object_pin_pages(obj);

Why do you need to pin the pages? It has no relevance to the backing storage, that can be discard freely by the other end of vgpu. get_pages is just allocating an sg_table...
-chris

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Gerd Hoffmann

2017-05-19 11:28 UTC #9 lore raw

Re: [PATCH v2 4/5] drm/i915/gvt: Dmabuf support for GVT-g

In reply to Xiaoguang Chen

include/uapi/drm/i915_drm.h enum drm_i915_perf_record_type { · line 1452

Quoted 5 lines

include/uapi/drm/i915_drm.h +5 -0

+/**
+ * Ioctl to query plane info or create dma-buf
+ */
+#define INTEL_VGPU_QUERY_DMABUF		0
+#define INTEL_VGPU_GENERATE_DMABUF	1

This should use _IO* #defines.

include/uapi/drm/i915_drm.h enum drm_i915_perf_record_type { · line 1466

Quoted 13 lines

include/uapi/drm/i915_drm.h +13 -0

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

I'd suggest to drop tiled, and add drm_format_mod (for fourcc_mod_code()) instead.

You can also move intel_vgpu_plane_info to this place and just use it as sub-struct for intel_vgpu_dmabuf.

cheers,
Gerd

Xiaoguang Chen

author

2017-05-18 09:50 UTC #10 lore raw

PATCH v2 5/5 drm/i915/gvt: Adding interface so user space can get the dma-buf

In reply to Xiaoguang Chen

User space will try to create a management fd for the dma-buf operation. Using this management fd user can query the plane information and create a dma-buf fd if necessary.
GVT-g will handle the life cycle of the management fd and will align the life cycle of the fd with the vfio device.
User space should handle the life cycle of the created dma-buf fd close the dma-buf fd timely when finishing use.

Signed-off-by Xiaoguang Chen <[email protected]>

---
 drivers/gpu/drm/i915/gvt/gvt.c   |  2 +
 drivers/gpu/drm/i915/gvt/gvt.h   |  3 ++
 drivers/gpu/drm/i915/gvt/kvmgt.c | 89 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h      |  2 +
 include/uapi/linux/vfio.h        | 12 ++++++
 5 files changed, 108 insertions(+)

drivers/gpu/drm/i915/gvt/gvt.c +2 -0

index 2032917..48e04e6 100644
@@ -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_create_dmabuf = intel_vgpu_create_dmabuf,
 };
 
 /**

drivers/gpu/drm/i915/gvt/gvt.h +3 -0

index a553120..b7fdfd5 100644
@@ -185,6 +185,7 @@ struct intel_vgpu {
 		struct kvm *kvm;
 		struct work_struct release_work;
 		atomic_t released;
+		struct vfio_device *vfio_device;
 	} vdev;
 #endif
 	struct intel_vgpu_plane_info *plane_info;

@@ -469,6 +470,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_create_dmabuf)(struct intel_vgpu *, void *);
 };

drivers/gpu/drm/i915/gvt/kvmgt.c +89 -0

107 lines not on this page · raw message

include/uapi/drm/i915_drm.h +2 -0

index cde4f8e..9d28433 100644
@@ -1466,6 +1466,8 @@ struct intel_vgpu_dmabuf {
 	__u32 tiled;
 };
 
+#define INTEL_VGPU_DMABUF_MGR_FD	0
+
 #if defined(__cplusplus)
 }
 #endif

include/uapi/linux/vfio.h +12 -0

index ae46105..c81500b 100644
@@ -502,6 +502,18 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)
+ *
+ * Create a fd for a vfio device based on the input type
+ * Vendor driver should handle this ioctl to create a fd and manage the
+ * life cycle of this fd.
+ *
+ * Return: a fd if vendor support that type, -errno if not supported
+ */
+
+#define VFIO_DEVICE_GET_FD	_IO(VFIO_TYPE, VFIO_BASE + 14)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Alex Williamson

2017-05-19 16:33 UTC #11 lore raw

Re: [PATCH v2 5/5] drm/i915/gvt: Adding interface so user space can get the dma-buf

Quoted 72 lines · On Thu, 18 May 2017 17:50:05 +0800 Xiaoguang Chen <[email protected]>…

Signed-off-by Xiaoguang Chen <[email protected]>

---
 drivers/gpu/drm/i915/gvt/gvt.c   |  2 +
 drivers/gpu/drm/i915/gvt/gvt.h   |  3 ++
 drivers/gpu/drm/i915/gvt/kvmgt.c | 89 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h      |  2 +
 include/uapi/linux/vfio.h        | 12 ++++++
 5 files changed, 108 insertions(+)

drivers/gpu/drm/i915/gvt/gvt.c +2 -0

index 2032917..48e04e6 100644
@@ -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_create_dmabuf = intel_vgpu_create_dmabuf,
 };
 
 /**

drivers/gpu/drm/i915/gvt/gvt.h +3 -0

index a553120..b7fdfd5 100644
@@ -185,6 +185,7 @@ struct intel_vgpu {
 		struct kvm *kvm;
 		struct work_struct release_work;
 		atomic_t released;
+		struct vfio_device *vfio_device;
 	} vdev;
 #endif
 	struct intel_vgpu_plane_info *plane_info;

@@ -469,6 +470,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_create_dmabuf)(struct intel_vgpu *, void *);
 };

drivers/gpu/drm/i915/gvt/kvmgt.c +6 -0

index 389f072..9a663df 100644
@@ -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,66 @@ 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)
+{
+	return -EPERM;
+}

drivers/gpu/drm/i915/gvt/kvmgt.c static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu) · line 542

Quoted 10 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +10 -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);
+	else
+		gvt_vgpu_err("intel vgpu dmabuf mgr fd is in a wrong state\n");

You could do:

if (WARN_ON(!vgpu->vdev.vfio_device))

	return -EINVAL;

Quoted 13 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +13 -0

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

drivers/gpu/drm/i915/gvt/kvmgt.c static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu) · line 565

Quoted 10 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +10 -0

+	minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
+	if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+		return -EFAULT;
+
+	f = fdget(dmabuf.fd);
+
+	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_create_dmabuf(vgpu, &dmabuf);

Why are these still Intel specific?

Quoted 27 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +22 -0

+	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 +1320,34 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 	} else if (cmd == VFIO_DEVICE_RESET) {

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd, · line 1330

Quoted 10 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +8 -0

 		intel_gvt_ops->vgpu_reset(vgpu);
 		return 0;
+	} else if (cmd == VFIO_DEVICE_GET_FD) {
+		int fd;
+		u32 type;
+		struct vfio_device *device;
+
+		if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
+			return -EINVAL;
+		if (type != INTEL_VGPU_DMABUF_MGR_FD) {

Yet more unnecessary Intel specific code. Userspace doesn't want to handle a dmabuf differently between Intel, AMD, and NVIDIA, so if we have no reason to need vendor specific ioctl and parameters, then don't.

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd, · line 1331

Quoted 1 line

drivers/gpu/drm/i915/gvt/kvmgt.c +1 -0
+			gvt_vgpu_err("not supported fd type:%d\n", type);

Just return error, users can abuse any logging they can get to.

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd, · line 1337

Quoted 6 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +6 -0

+			return -EINVAL;
+		}
+
+		fd = anon_inode_getfd("intel-vgpu-dmabuf-mgr-fd",
+			&intel_vgpu_dmabuf_mgr_fd_ops,
+			vgpu, O_RDWR | O_CLOEXEC);

And this ordering is still wrong, do this last so you don't need to worry about getting the vfio_device reference below failing and calling release unnecessarily (though it seems this fd is leaked in that case anyway).

Quoted 39 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +13 -0

+		if (fd < 0) {
+			gvt_vgpu_err("create dmabuf mgr fd failed\n");
+			return -EINVAL;
+		}
+
+		device = vfio_device_get_from_dev(mdev_dev(mdev));
+		if (device == NULL) {
+			gvt_vgpu_err("kvmgt: vfio device is null\n");
+			return -EINVAL;
+		}
+		vgpu->vdev.vfio_device = device;
+
+		return fd;
 	}
 
 	return 0;

include/uapi/drm/i915_drm.h +2 -0

index cde4f8e..9d28433 100644
@@ -1466,6 +1466,8 @@ struct intel_vgpu_dmabuf {
 	__u32 tiled;
 };
 
+#define INTEL_VGPU_DMABUF_MGR_FD	0
+
 #if defined(__cplusplus)
 }
 #endif

include/uapi/linux/vfio.h +2 -0

index ae46105..c81500b 100644
@@ -502,6 +502,18 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 516

Quoted 10 lines

include/uapi/linux/vfio.h +10 -0

+ *
+ * Create a fd for a vfio device based on the input type
+ * Vendor driver should handle this ioctl to create a fd and manage the
+ * life cycle of this fd.
+ *
+ * Return: a fd if vendor support that type, -errno if not supported
+ */
+
+#define VFIO_DEVICE_GET_FD	_IO(VFIO_TYPE, VFIO_BASE + 14)
+

The known input types would need to be defined here too. Definition of the ioctls available on that type should also be documented here.

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 519

Quoted 3 lines

include/uapi/linux/vfio.h +0 -0

 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chen, Xiaoguang

author

2017-05-22 02:00 UTC #12 lore raw

RE: [PATCH v2 5/5] drm/i915/gvt: Adding interface so user space can get the dma-buf

Hi Alex,

Quoted 102 lines

-----Original Message-----
From: Alex Williamson [mailto:[email protected]]
Sent: Saturday, May 20, 2017 12:34 AM
To: Chen, Xiaoguang <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; Lv, Zhiyuan <[email protected]>; [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin <[email protected]>
Subject: Re: [PATCH v2 5/5] drm/i915/gvt: Adding interface so user space can get the dma-buf
Quoted 8 lines · On Thu, 18 May 2017 17:50:05 +0800 Xiaoguang Chen <[email protected]>…
User space will try to create a management fd for the dma-buf operation. Using this management fd user can query the plane information and create a dma-buf fd if necessary.
GVT-g will handle the life cycle of the management fd and will align the life cycle of the fd with the vfio device.
User space should handle the life cycle of the created dma-buf fd close the dma-buf fd timely when finishing use.
Quoted 5 lines
Signed-off-by Xiaoguang Chen <[email protected]>
---
 drivers/gpu/drm/i915/gvt/gvt.c   |  2 +
 drivers/gpu/drm/i915/gvt/gvt.h   |  3 ++
 drivers/gpu/drm/i915/gvt/kvmgt.c | 89
++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/gvt/kvmgt.c static int intel_vgpu_reg_init_opregion(struct
Quoted 52 lines
 include/uapi/drm/i915_drm.h      |  2 +
 include/uapi/linux/vfio.h        | 12 ++++++
 5 files changed, 108 insertions(+)
(quoted diff) +0 -0
b/drivers/gpu/drm/i915/gvt/gvt.c index 2032917..48e04e6 100644
drivers/gpu/drm/i915/gvt/gvt.c +2 -0
@@ -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_create_dmabuf = intel_vgpu_create_dmabuf,
 };
 
 /**
(quoted diff) +0 -0
b/drivers/gpu/drm/i915/gvt/gvt.h index a553120..b7fdfd5 100644
drivers/gpu/drm/i915/gvt/gvt.h +1 -0
@@ -185,6 +185,7 @@ struct intel_vgpu {
 		struct kvm *kvm;
 		struct work_struct release_work;
 		atomic_t released;
+		struct vfio_device *vfio_device;
 	} vdev;
 #endif
 	struct intel_vgpu_plane_info *plane_info; @@ -469,6 +470,8 @@ struct
intel_gvt_ops {
drivers/gpu/drm/i915/gvt/gvt.h +2 -0
 	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_create_dmabuf)(struct intel_vgpu *, void *);
 };
drivers/gpu/drm/i915/gvt/kvmgt.c +1 -0
index 389f072..9a663df 100644
@@ -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,66 @@ static int intel_vgpu_reg_init_opregion(struct
intel_vgpu *vgpu)
Quoted 8 lines
drivers/gpu/drm/i915/gvt/kvmgt.c +5 -0
 	return ret;
 }
 
+static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file,
+		struct vm_area_struct *vma)
+{
+	return -EPERM;
+}
drivers/gpu/drm/i915/gvt/kvmgt.c static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu) · line 542
Quoted 10 lines
drivers/gpu/drm/i915/gvt/kvmgt.c +10 -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);
+	else
+		gvt_vgpu_err("intel vgpu dmabuf mgr fd is in a wrong state\n");
You could do:

Quoted 2 lines

if (WARN_ON(!vgpu->vdev.vfio_device))
	return -EINVAL;

OK.

Quoted 17 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +16 -0

+
+	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;
+
+	minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
+	if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
+		return -EFAULT;
+

Quoted 8 lines

drivers/gpu/drm/i915/gvt/kvmgt.c static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu) · line 565
Quoted 6 lines
drivers/gpu/drm/i915/gvt/kvmgt.c +6 -0
+	f = fdget(dmabuf.fd);
+
+	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_create_dmabuf(vgpu, &dmabuf);
Why are these still Intel specific?

Ah, the v2 patch series were sent before our discussion in other thread. Will change it in v3.

Quoted 28 lines

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device

Quoted 27 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +22 -0

+	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 +1320,34 @@ static long intel_vgpu_ioctl(struct mdev_device

*mdev, unsigned int cmd,

Quoted 15 lines

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd, · line 1330
Quoted 11 lines
drivers/gpu/drm/i915/gvt/kvmgt.c +8 -0
 	} else if (cmd == VFIO_DEVICE_RESET) {
 		intel_gvt_ops->vgpu_reset(vgpu);
 		return 0;
+	} else if (cmd == VFIO_DEVICE_GET_FD) {
+		int fd;
+		u32 type;
+		struct vfio_device *device;
+
+		if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
+			return -EINVAL;
+		if (type != INTEL_VGPU_DMABUF_MGR_FD) {
Yet more unnecessary Intel specific code. Userspace doesn't want to handle a dmabuf differently between Intel, AMD, and NVIDIA, so if we have no reason to need vendor specific ioctl and parameters, then don't.

Yes. I will change in v3.

Quoted 4 lines

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd, · line 1331
Quoted 2 lines
drivers/gpu/drm/i915/gvt/kvmgt.c +1 -0
+			gvt_vgpu_err("not supported fd type:%d\n", type);
Just return error, users can abuse any logging they can get to.

OK.

Quoted 11 lines

drivers/gpu/drm/i915/gvt/kvmgt.c static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd, · line 1337
Quoted 7 lines
drivers/gpu/drm/i915/gvt/kvmgt.c +6 -0
+			return -EINVAL;
+		}
+
+		fd = anon_inode_getfd("intel-vgpu-dmabuf-mgr-fd",
+			&intel_vgpu_dmabuf_mgr_fd_ops,
+			vgpu, O_RDWR | O_CLOEXEC);
And this ordering is still wrong, do this last so you don't need to worry about getting the vfio_device reference below failing and calling release unnecessarily (though it seems this fd is leaked in that case anyway).

Will change in v3.

Quoted 38 lines

drivers/gpu/drm/i915/gvt/kvmgt.c +13 -0

+		if (fd < 0) {
+			gvt_vgpu_err("create dmabuf mgr fd failed\n");
+			return -EINVAL;
+		}
+
+		device = vfio_device_get_from_dev(mdev_dev(mdev));
+		if (device == NULL) {
+			gvt_vgpu_err("kvmgt: vfio device is null\n");
+			return -EINVAL;
+		}
+		vgpu->vdev.vfio_device = device;
+
+		return fd;
 	}
 
 	return 0;

include/uapi/drm/i915_drm.h +2 -0

index cde4f8e..9d28433 100644
@@ -1466,6 +1466,8 @@ struct intel_vgpu_dmabuf {
 	__u32 tiled;
 };
 
+#define INTEL_VGPU_DMABUF_MGR_FD	0
+
 #if defined(__cplusplus)
 }
 #endif

include/uapi/linux/vfio.h +0 -0

index ae46105..c81500b 100644
@@ -502,6 +502,18 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)

Quoted 16 lines

include/uapi/linux/vfio.h struct vfio_pci_hot_reset { · line 516
Quoted 13 lines
include/uapi/linux/vfio.h +13 -0
+/**
+ * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)
+ *
+ * Create a fd for a vfio device based on the input type
+ * Vendor driver should handle this ioctl to create a fd and manage
+the
+ * life cycle of this fd.
+ *
+ * Return: a fd if vendor support that type, -errno if not supported
+*/
+
+#define VFIO_DEVICE_GET_FD	_IO(VFIO_TYPE, VFIO_BASE + 14)
+
The known input types would need to be defined here too. Definition of the ioctls available on that type should also be documented here.

Yes. Will do it in v3.
Thanks.

Quoted 4 lines

Quoted 4 lines
include/uapi/linux/vfio.h +0 -0
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**

Patchwork

2017-05-18 13:52 UTC #13 lore raw

✓ Fi.CI.BAT: success for drm/i915/gvt: dma-buf support for GVT-g (rev2)

In reply to Xiaoguang Chen

== Series Details ==

Series: drm/i915/gvt: dma-buf support for GVT-g (rev2)
URL : https://patchwork.freedesktop.org/series/23686/
State : success

== Summary ==

Series 23686v2 drm/i915/gvt: dma-buf support for GVT-g
https://patchwork.freedesktop.org/api/1.0/series/23686/revisions/2/mbox/

Test kms_cursor_legacy:

        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-snb-2600) fdo#100215

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:443s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:434s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:594s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:516s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:493s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:490s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:425s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:415s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:418s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:498s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:463s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:458s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:578s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:469s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:497s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:439s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:407s

fi-kbl-7560u failed to collect. IGT log at Patchwork_4736/fi-kbl-7560u/igt.log

d89ce671546724cca610b42c95218532991a480c drm-tip: 2017y-05m-18d-12h-42m-48s UTC integration manifest d0916ef drm/i915/gvt: Adding interface so user space can get the dma-buf
d463b50 drm/i915/gvt: Dmabuf support for GVT-g
2993ade drm/i915/gvt: Frame buffer decoder support for GVT-g
cb45796 drm/i915/gvt: OpRegion support for GVT-g
e5666eb drm/i915/gvt: Extend the GVT-g architecture to support vfio device region

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4736/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx