[RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Tina Zhang]

This series tries to send the vGPU display vblank event to user land.

Instead of delivering page flip events, we choose to post display vblank
event. Handling page flip events for both primary plane and cursor plane
may make user space quite busy, although we have the mask/unmask mechansim
for mitigation. Besides, there are some cases that guest app only uses
one framebuffer for both drawing and display. In such case, guest OS won't
do the plane page flip when the framebuffer is updated, thus the user
land won't be notified about the updated framebuffer.

With vGPU's vblank event working as a timer, userspace still needs to
query the framebuffer first, before getting a new dma-buf for it.

v3:
- Deliver display vblank event instead of page flip event. (Zhenyu)
v2:
- Use VFIO irq chain to get eventfds from userspace instead of adding
a new ABI. (Alex)
v1:
- https://patchwork.kernel.org/cover/10962341/


Tina Zhang (4):
  vfio: Define device specific irq type capability
  vfio: Introduce vGPU display irq type
  drm/i915/gvt: Register vGPU display vblank irq
  drm/i915/gvt: Deliver vGPU vblank event to userspace

 drivers/gpu/drm/i915/gvt/display.c   |  14 +-
 drivers/gpu/drm/i915/gvt/gvt.h       |   6 +
 drivers/gpu/drm/i915/gvt/hypercall.h |   1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 193 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/gvt/mpt.h       |  17 +++
 include/uapi/linux/vfio.h            |  22 ++-
 6 files changed, 241 insertions(+), 12 deletions(-)

-- 
2.17.1

[RFC PATCH v3 1/4] vfio: Define device specific irq type capability

[Tina Zhang]

Cap the number of irqs with fixed indexes and use capability chains
to chain device specific irqs.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 02bb7ad6e986..600784acc4ac 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -444,11 +444,27 @@ struct vfio_irq_info {
 #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
 #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
 #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
+#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps */
 	__u32	index;		/* IRQ index */
+	__u32	cap_offset;	/* Offset within info struct of first cap */
 	__u32	count;		/* Number of IRQs within this index */
 };
 #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
 
+/*
+ * The irq type capability allows irqs unique to a specific device or
+ * class of devices to be exposed.
+ *
+ * The structures below define version 1 of this capability.
+ */
+#define VFIO_IRQ_INFO_CAP_TYPE      3
+
+struct vfio_irq_info_cap_type {
+	struct vfio_info_cap_header header;
+	__u32 type;     /* global per bus driver */
+	__u32 subtype;  /* type specific */
+};
+
 /**
  * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
  *
@@ -550,7 +566,8 @@ enum {
 	VFIO_PCI_MSIX_IRQ_INDEX,
 	VFIO_PCI_ERR_IRQ_INDEX,
 	VFIO_PCI_REQ_IRQ_INDEX,
-	VFIO_PCI_NUM_IRQS
+	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use   */
+				/* device specific cap to define content */
 };
 
 /*
-- 
2.17.1

[RFC PATCH v3 2/4] vfio: Introduce vGPU display irq type

[Tina Zhang]

Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/uapi/linux/vfio.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 600784acc4ac..c3e9c821a5cb 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -465,6 +465,9 @@ struct vfio_irq_info_cap_type {
 	__u32 subtype;  /* type specific */
 };
 
+#define VFIO_IRQ_TYPE_GFX				(1)
+#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
+
 /**
  * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
  *
-- 
2.17.1

[RFC PATCH v3 3/4] drm/i915/gvt: Register vGPU display vblank irq

[Tina Zhang]

Gvt-g emulates and injects the vGPU's display vblank interrupts in
kernel space. However the dma-buf based framebuffer consumer in the
user land (e.g. Qemu vfio/display) may also need to be notified by
this event.

Register the display irq as VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ to
each vGPU, so that the display interrupt event can be delivered to
userspace through eventfd.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.c   | 10 +++++--
 drivers/gpu/drm/i915/gvt/gvt.h       |  2 ++
 drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 43 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/mpt.h       | 17 +++++++++++
 5 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index e1c313da6c00..1a0a4ae4826e 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -506,16 +506,22 @@ void intel_vgpu_clean_display(struct intel_vgpu *vgpu)
 int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution)
 {
 	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
+	int ret;
 
 	intel_vgpu_init_i2c_edid(vgpu);
 
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv) ||
 	    IS_COFFEELAKE(dev_priv))
-		return setup_virtual_dp_monitor(vgpu, PORT_D, GVT_DP_D,
+		ret = setup_virtual_dp_monitor(vgpu, PORT_D, GVT_DP_D,
 						resolution);
 	else
-		return setup_virtual_dp_monitor(vgpu, PORT_B, GVT_DP_B,
+		ret = setup_virtual_dp_monitor(vgpu, PORT_B, GVT_DP_B,
 						resolution);
+
+	if (ret == 0)
+		intel_gvt_hypervisor_register_display_irq(vgpu);
+
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index f5a328b5290a..cd29ea28d7ed 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -201,6 +201,8 @@ struct intel_vgpu {
 		struct mdev_device *mdev;
 		struct vfio_region *region;
 		int num_regions;
+		struct vfio_irq *irq;
+		int num_irqs;
 		struct eventfd_ctx *intx_trigger;
 		struct eventfd_ctx *msi_trigger;
 
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index 4862fb12778e..be33f20f3bc1 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -68,6 +68,7 @@ struct intel_gvt_mpt {
 			     bool map);
 	int (*set_opregion)(void *vgpu);
 	int (*set_edid)(void *vgpu, int port_num);
+	int (*register_display_irq)(void *vgpu);
 	int (*get_vfio_device)(void *vgpu);
 	void (*put_vfio_device)(void *vgpu);
 	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index a68addf95c23..f222c9cd7a56 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -78,6 +78,13 @@ struct vfio_region {
 	void				*data;
 };
 
+struct vfio_irq {
+	u32	type;
+	u32	subtype;
+	u32	flags;
+	u32	count;
+};
+
 struct vfio_edid_region {
 	struct vfio_region_gfx_edid vfio_edid_regs;
 	void *edid_blob;
@@ -635,6 +642,37 @@ static int kvmgt_set_edid(void *p_vgpu, int port_num)
 	return ret;
 }
 
+static int intel_vgpu_register_irq(struct intel_vgpu *vgpu,
+		unsigned int type, unsigned int subtype, u32 count, u32 flags)
+{
+	struct vfio_irq *irq;
+
+	irq = krealloc(vgpu->vdev.irq,
+			(vgpu->vdev.num_irqs + 1) * sizeof(*irq),
+			GFP_KERNEL);
+	if (!irq)
+		return -ENOMEM;
+
+	vgpu->vdev.irq = irq;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].type = type;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].subtype = subtype;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].count = count;
+	vgpu->vdev.irq[vgpu->vdev.num_irqs].flags = flags;
+	vgpu->vdev.num_irqs++;
+	return 0;
+}
+
+static int kvmgt_register_display_irq(void *p_vgpu)
+{
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
+
+	intel_vgpu_register_irq(vgpu, VFIO_IRQ_TYPE_GFX,
+				VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ,
+				1,
+				VFIO_IRQ_INFO_MASKABLE | VFIO_IRQ_INFO_EVENTFD);
+	return 0;
+}
+
 static void kvmgt_put_vfio_device(void *vgpu)
 {
 	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
@@ -1838,6 +1876,10 @@ static void kvmgt_detach_vgpu(void *p_vgpu)
 	vgpu->vdev.num_regions = 0;
 	kfree(vgpu->vdev.region);
 	vgpu->vdev.region = NULL;
+
+	vgpu->vdev.num_irqs = 0;
+	kfree(vgpu->vdev.irq);
+	vgpu->vdev.irq = NULL;
 }
 
 static int kvmgt_inject_msi(unsigned long handle, u32 addr, u16 data)
@@ -2039,6 +2081,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
 	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
 	.set_opregion = kvmgt_set_opregion,
 	.set_edid = kvmgt_set_edid,
+	.register_display_irq = kvmgt_register_display_irq,
 	.get_vfio_device = kvmgt_get_vfio_device,
 	.put_vfio_device = kvmgt_put_vfio_device,
 	.is_valid_gfn = kvmgt_is_valid_gfn,
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 0f9440128123..abf4a69920d3 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -330,6 +330,23 @@ static inline int intel_gvt_hypervisor_set_edid(struct intel_vgpu *vgpu,
 	return intel_gvt_host.mpt->set_edid(vgpu, port_num);
 }
 
+/**
+ * intel_gvt_hypervisor_set_irq - register vgpu specific irq
+ * @vgpu: a vGPU
+ * @port_num: display port number
+ *
+ * Returns:
+ * Zero on success, negative error code if failed.
+ */
+static inline int intel_gvt_hypervisor_register_display_irq(
+						struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->register_display_irq)
+		return 0;
+
+	return intel_gvt_host.mpt->register_display_irq(vgpu);
+}
+
 /**
  * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
  * @vgpu: a vGPU
-- 
2.17.1

[RFC PATCH v3 4/4] drm/i915/gvt: Deliver vGPU vblank event to userspace

[Tina Zhang]

Deliver the display vblank event to the user land. Userspace can use
the irq mask/unmask mechanism to disable or enable the event delivery.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.c |   4 +
 drivers/gpu/drm/i915/gvt/gvt.h     |   4 +
 drivers/gpu/drm/i915/gvt/kvmgt.c   | 150 +++++++++++++++++++++++++++--
 3 files changed, 149 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
index 1a0a4ae4826e..e62313b5f8a6 100644
--- a/drivers/gpu/drm/i915/gvt/display.c
+++ b/drivers/gpu/drm/i915/gvt/display.c
@@ -412,6 +412,10 @@ static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
 
 	if (pipe_is_enabled(vgpu, pipe)) {
 		vgpu_vreg_t(vgpu, PIPE_FRMCOUNT_G4X(pipe))++;
+		if (vgpu->vdev.vblank_trigger &&
+		    !(vgpu->vdev.display_event_mask & DISPLAY_VBLANK_EVENT))
+			eventfd_signal(vgpu->vdev.vblank_trigger, 1);
+
 		intel_vgpu_trigger_virtual_event(vgpu, vblank_event[pipe]);
 	}
 }
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index cd29ea28d7ed..b3b476ee5acf 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -165,6 +165,8 @@ struct intel_vgpu_submission {
 	bool active;
 };
 
+#define DISPLAY_VBLANK_EVENT	(1 << 0)
+
 struct intel_vgpu {
 	struct intel_gvt *gvt;
 	struct mutex vgpu_lock;
@@ -205,6 +207,8 @@ struct intel_vgpu {
 		int num_irqs;
 		struct eventfd_ctx *intx_trigger;
 		struct eventfd_ctx *msi_trigger;
+		struct eventfd_ctx *vblank_trigger;
+		u32 display_event_mask;
 
 		/*
 		 * Two caches are used to avoid mapping duplicated pages (eg.
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index f222c9cd7a56..7a84222d7d2d 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1222,6 +1222,8 @@ static int intel_vgpu_get_irq_count(struct intel_vgpu *vgpu, int type)
 {
 	if (type == VFIO_PCI_INTX_IRQ_INDEX || type == VFIO_PCI_MSI_IRQ_INDEX)
 		return 1;
+	else if (type < VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs)
+		return vgpu->vdev.irq[type - VFIO_PCI_NUM_IRQS].count;
 
 	return 0;
 }
@@ -1269,7 +1271,60 @@ static int intel_vgpu_set_msi_trigger(struct intel_vgpu *vgpu,
 	return 0;
 }
 
-static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
+static int intel_vgu_set_display_irq_mask(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	if (start != 0 || count != 1)
+		return -EINVAL;
+
+	if (flags & VFIO_IRQ_SET_DATA_NONE)
+		vgpu->vdev.display_event_mask |= DISPLAY_VBLANK_EVENT;
+
+	return 0;
+}
+
+static int intel_vgu_set_display_irq_unmask(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	if (start != 0 || count != 1)
+		return -EINVAL;
+
+	if (flags & VFIO_IRQ_SET_DATA_NONE)
+		vgpu->vdev.display_event_mask &= ~DISPLAY_VBLANK_EVENT;
+
+	return 0;
+}
+
+static int intel_vgpu_set_display_event_trigger(struct intel_vgpu *vgpu,
+		unsigned int index, unsigned int start, unsigned int count,
+		u32 flags, void *data)
+{
+	struct eventfd_ctx *trigger;
+
+	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+		int fd = *(int *)data;
+
+		trigger = eventfd_ctx_fdget(fd);
+		if (IS_ERR(trigger)) {
+			gvt_vgpu_err("eventfd_ctx_fdget failed\n");
+			return PTR_ERR(trigger);
+		}
+		vgpu->vdev.vblank_trigger = trigger;
+		vgpu->vdev.display_event_mask = 0;
+	} else if ((flags & VFIO_IRQ_SET_DATA_NONE) && !count) {
+		trigger = vgpu->vdev.vblank_trigger;
+		if (trigger) {
+			eventfd_ctx_put(trigger);
+			vgpu->vdev.vblank_trigger = NULL;
+		}
+	}
+
+	return 0;
+}
+
+int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
 		unsigned int index, unsigned int start, unsigned int count,
 		void *data)
 {
@@ -1302,6 +1357,35 @@ static int intel_vgpu_set_irqs(struct intel_vgpu *vgpu, u32 flags,
 			break;
 		}
 		break;
+	default:
+	{
+		int i;
+
+		if (index >= VFIO_PCI_NUM_IRQS +
+					vgpu->vdev.num_irqs)
+			return -EINVAL;
+		index =
+			array_index_nospec(index,
+						VFIO_PCI_NUM_IRQS +
+						vgpu->vdev.num_irqs);
+
+		i = index - VFIO_PCI_NUM_IRQS;
+		if (vgpu->vdev.irq[i].type == VFIO_IRQ_TYPE_GFX &&
+		    vgpu->vdev.irq[i].subtype ==
+		    VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ) {
+			switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+			case VFIO_IRQ_SET_ACTION_MASK:
+				func = intel_vgu_set_display_irq_mask;
+				break;
+			case VFIO_IRQ_SET_ACTION_UNMASK:
+				func = intel_vgu_set_display_irq_unmask;
+				break;
+			case VFIO_IRQ_SET_ACTION_TRIGGER:
+				func = intel_vgpu_set_display_event_trigger;
+				break;
+			}
+		}
+	}
 	}
 
 	if (!func)
@@ -1333,7 +1417,7 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		info.flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.num_regions = VFIO_PCI_NUM_REGIONS +
 				vgpu->vdev.num_regions;
-		info.num_irqs = VFIO_PCI_NUM_IRQS;
+		info.num_irqs = VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs;
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
@@ -1493,32 +1577,79 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			-EFAULT : 0;
 	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
 		struct vfio_irq_info info;
+		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+		unsigned int i;
+		int ret;
 
 		minsz = offsetofend(struct vfio_irq_info, count);
 
 		if (copy_from_user(&info, (void __user *)arg, minsz))
 			return -EFAULT;
 
-		if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
+		if (info.argsz < minsz)
 			return -EINVAL;
 
 		switch (info.index) {
 		case VFIO_PCI_INTX_IRQ_INDEX:
 		case VFIO_PCI_MSI_IRQ_INDEX:
+			info.flags = VFIO_IRQ_INFO_EVENTFD;
 			break;
-		default:
+		case VFIO_PCI_MSIX_IRQ_INDEX:
+		case VFIO_PCI_ERR_IRQ_INDEX:
+		case VFIO_PCI_REQ_IRQ_INDEX:
 			return -EINVAL;
-		}
+		default:
+		{
+			struct vfio_irq_info_cap_type cap_type = {
+				.header.id = VFIO_IRQ_INFO_CAP_TYPE,
+				.header.version = 1 };
 
-		info.flags = VFIO_IRQ_INFO_EVENTFD;
+			if (info.index >= VFIO_PCI_NUM_IRQS +
+					vgpu->vdev.num_irqs)
+				return -EINVAL;
+			info.index =
+				array_index_nospec(info.index,
+						VFIO_PCI_NUM_IRQS +
+						vgpu->vdev.num_irqs);
+
+			i = info.index - VFIO_PCI_NUM_IRQS;
+
+			info.flags = vgpu->vdev.irq[i].flags;
+			cap_type.type = vgpu->vdev.irq[i].type;
+			cap_type.subtype = vgpu->vdev.irq[i].subtype;
+
+			ret = vfio_info_add_capability(&caps,
+						&cap_type.header,
+						sizeof(cap_type));
+			if (ret)
+				return ret;
+		}
+		}
 
 		info.count = intel_vgpu_get_irq_count(vgpu, info.index);
 
 		if (info.index == VFIO_PCI_INTX_IRQ_INDEX)
 			info.flags |= (VFIO_IRQ_INFO_MASKABLE |
 				       VFIO_IRQ_INFO_AUTOMASKED);
-		else
-			info.flags |= VFIO_IRQ_INFO_NORESIZE;
+
+		if (caps.size) {
+			info.flags |= VFIO_IRQ_INFO_FLAG_CAPS;
+			if (info.argsz < sizeof(info) + caps.size) {
+				info.argsz = sizeof(info) + caps.size;
+				info.cap_offset = 0;
+			} else {
+				vfio_info_cap_shift(&caps, sizeof(info));
+				if (copy_to_user((void __user *)arg +
+						  sizeof(info), caps.buf,
+						  caps.size)) {
+					kfree(caps.buf);
+					return -EFAULT;
+				}
+				info.cap_offset = sizeof(info);
+			}
+
+			kfree(caps.buf);
+		}
 
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
@@ -1537,7 +1668,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			int max = intel_vgpu_get_irq_count(vgpu, hdr.index);
 
 			ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
-						VFIO_PCI_NUM_IRQS, &data_size);
+					VFIO_PCI_NUM_IRQS + vgpu->vdev.num_irqs,
+								 &data_size);
 			if (ret) {
 				gvt_vgpu_err("intel:vfio_set_irqs_validate_and_prepare failed\n");
 				return -EINVAL;
-- 
2.17.1

Re: [RFC PATCH v3 1/4] vfio: Define device specific irq type capability

[Alex Williamson]

On Thu, 27 Jun 2019 11:37:59 +0800
Tina Zhang <tina.zhang@intel.com> wrote:

> Cap the number of irqs with fixed indexes and use capability chains
> to chain device specific irqs.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 02bb7ad6e986..600784acc4ac 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -444,11 +444,27 @@ struct vfio_irq_info {
>  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
>  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
>  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info supports caps */
>  	__u32	index;		/* IRQ index */
> +	__u32	cap_offset;	/* Offset within info struct of first cap */
>  	__u32	count;		/* Number of IRQs within this index */
>  };


This cannot be inserted into the middle of the structure, it breaks
compatibility with all existing userspace binaries.  I must be added to
the end of the structure.

>  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE + 9)
>  
> +/*
> + * The irq type capability allows irqs unique to a specific device or
> + * class of devices to be exposed.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_TYPE      3
> +
> +struct vfio_irq_info_cap_type {
> +	struct vfio_info_cap_header header;
> +	__u32 type;     /* global per bus driver */
> +	__u32 subtype;  /* type specific */
> +};
> +
>  /**
>   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
>   *
> @@ -550,7 +566,8 @@ enum {
>  	VFIO_PCI_MSIX_IRQ_INDEX,
>  	VFIO_PCI_ERR_IRQ_INDEX,
>  	VFIO_PCI_REQ_IRQ_INDEX,
> -	VFIO_PCI_NUM_IRQS
> +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5 use   */
> +				/* device specific cap to define content */
>  };
>  
>  /*

Re: [RFC PATCH v3 1/4] vfio: Define device specific irq type capability

[Gerd Hoffmann]

  Hi,

> +struct vfio_irq_info_cap_type {
> +	struct vfio_info_cap_header header;
> +	__u32 type;     /* global per bus driver */
> +	__u32 subtype;  /* type specific */

Do we really need both type and subtype?

cheers,
  Gerd

Re: [RFC PATCH v3 2/4] vfio: Introduce vGPU display irq type

[Gerd Hoffmann]

On Thu, Jun 27, 2019 at 11:38:00AM +0800, Tina Zhang wrote:
> Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  include/uapi/linux/vfio.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 600784acc4ac..c3e9c821a5cb 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -465,6 +465,9 @@ struct vfio_irq_info_cap_type {
>  	__u32 subtype;  /* type specific */
>  };
>  
> +#define VFIO_IRQ_TYPE_GFX				(1)
> +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)

VFIO_IRQ_TYPE_GFX_VBLANK ?

cheers,
  Gerd

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Gerd Hoffmann]

  Hi,

> Instead of delivering page flip events, we choose to post display vblank
> event. Handling page flip events for both primary plane and cursor plane
> may make user space quite busy, although we have the mask/unmask mechansim
> for mitigation. Besides, there are some cases that guest app only uses
> one framebuffer for both drawing and display. In such case, guest OS won't
> do the plane page flip when the framebuffer is updated, thus the user
> land won't be notified about the updated framebuffer.

What happens when the guest is idle and doesn't draw anything to the
framebuffer?

cheers,
  Gerd

RE: [RFC PATCH v3 1/4] vfio: Define device specific irq type capability

[Zhang, Tina]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, June 27, 2019 12:08 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; kraxel@redhat.com; zhenyuw@linux.intel.com; Lv,
> Zhiyuan <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian,
> Kevin <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>
> Subject: Re: [RFC PATCH v3 1/4] vfio: Define device specific irq type
> capability
> 
> On Thu, 27 Jun 2019 11:37:59 +0800
> Tina Zhang <tina.zhang@intel.com> wrote:
> 
> > Cap the number of irqs with fixed indexes and use capability chains to
> > chain device specific irqs.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 02bb7ad6e986..600784acc4ac 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -444,11 +444,27 @@ struct vfio_irq_info {
> >  #define VFIO_IRQ_INFO_MASKABLE		(1 << 1)
> >  #define VFIO_IRQ_INFO_AUTOMASKED	(1 << 2)
> >  #define VFIO_IRQ_INFO_NORESIZE		(1 << 3)
> > +#define VFIO_IRQ_INFO_FLAG_CAPS		(1 << 4) /* Info
> supports caps */
> >  	__u32	index;		/* IRQ index */
> > +	__u32	cap_offset;	/* Offset within info struct of first cap */
> >  	__u32	count;		/* Number of IRQs within this index */
> >  };
> 
> 
> This cannot be inserted into the middle of the structure, it breaks
> compatibility with all existing userspace binaries.  I must be added to the end
> of the structure.
Indeed. Thanks.

BR,
Tina
> 
> >  #define VFIO_DEVICE_GET_IRQ_INFO	_IO(VFIO_TYPE, VFIO_BASE +
> 9)
> >
> > +/*
> > + * The irq type capability allows irqs unique to a specific device or
> > + * class of devices to be exposed.
> > + *
> > + * The structures below define version 1 of this capability.
> > + */
> > +#define VFIO_IRQ_INFO_CAP_TYPE      3
> > +
> > +struct vfio_irq_info_cap_type {
> > +	struct vfio_info_cap_header header;
> > +	__u32 type;     /* global per bus driver */
> > +	__u32 subtype;  /* type specific */
> > +};
> > +
> >  /**
> >   * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct
> vfio_irq_set)
> >   *
> > @@ -550,7 +566,8 @@ enum {
> >  	VFIO_PCI_MSIX_IRQ_INDEX,
> >  	VFIO_PCI_ERR_IRQ_INDEX,
> >  	VFIO_PCI_REQ_IRQ_INDEX,
> > -	VFIO_PCI_NUM_IRQS
> > +	VFIO_PCI_NUM_IRQS = 5	/* Fixed user ABI, IRQ indexes >=5
> use   */
> > +				/* device specific cap to define content */
> >  };
> >
> >  /*

RE: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhang, Tina]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Thursday, June 27, 2019 2:23 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; zhenyuw@linux.intel.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; Yuan, Hang <hang.yuan@intel.com>;
> alex.williamson@redhat.com
> Subject: Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> userspace
> 
>   Hi,
> 
> > Instead of delivering page flip events, we choose to post display
> > vblank event. Handling page flip events for both primary plane and
> > cursor plane may make user space quite busy, although we have the
> > mask/unmask mechansim for mitigation. Besides, there are some cases
> > that guest app only uses one framebuffer for both drawing and display.
> > In such case, guest OS won't do the plane page flip when the
> > framebuffer is updated, thus the user land won't be notified about the
> updated framebuffer.
> 
> What happens when the guest is idle and doesn't draw anything to the
> framebuffer?
The vblank event will be delivered to userspace as well, unless guest OS disable the pipe.
Does it make sense to vfio/display?
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd

RE: [RFC PATCH v3 1/4] vfio: Define device specific irq type capability

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Thursday, June 27, 2019 2:20 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; zhenyuw@linux.intel.com; Yuan, Hang
> <hang.yuan@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 v3 1/4] vfio: Define device specific irq type
> capability
> 
>   Hi,
> 
> > +struct vfio_irq_info_cap_type {
> > +	struct vfio_info_cap_header header;
> > +	__u32 type;     /* global per bus driver */
> > +	__u32 subtype;  /* type specific */
> 
> Do we really need both type and subtype?
Then, if one device has several irqs, how can we identify them?
Thanks.

BR,
tina
> 
> 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 v3 2/4] vfio: Introduce vGPU display irq type

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Thursday, June 27, 2019 2:21 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; zhenyuw@linux.intel.com; Yuan, Hang
> <hang.yuan@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 v3 2/4] vfio: Introduce vGPU display irq type
> 
> On Thu, Jun 27, 2019 at 11:38:00AM +0800, Tina Zhang wrote:
> > Introduce vGPU specific irq type VFIO_IRQ_TYPE_GFX, and
> > VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ as the subtype for vGPU display
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  include/uapi/linux/vfio.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 600784acc4ac..c3e9c821a5cb 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -465,6 +465,9 @@ struct vfio_irq_info_cap_type {
> >  	__u32 subtype;  /* type specific */
> >  };
> >
> > +#define VFIO_IRQ_TYPE_GFX				(1)
> > +#define VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ		(1)
> 
> VFIO_IRQ_TYPE_GFX_VBLANK ?
VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ is proposed to cover all the display related events. In this version, we only use VFIO_IRQ_SUBTYPE_GFX_DISPLAY_IRQ to deliver vblank event. However, we might still want to reserve seat for other display event, like page flip event.
Thanks.

BR,
Tina
> 
> 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 v3 1/4] vfio: Define device specific irq type capability

[Gerd Hoffmann]

On Thu, Jun 27, 2019 at 08:55:21AM +0000, Zhang, Tina wrote:
> 
> 
> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> > Behalf Of Gerd Hoffmann
> > Sent: Thursday, June 27, 2019 2:20 PM
> > To: Zhang, Tina <tina.zhang@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; zhenyuw@linux.intel.com; Yuan, Hang
> > <hang.yuan@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 v3 1/4] vfio: Define device specific irq type
> > capability
> > 
> >   Hi,
> > 
> > > +struct vfio_irq_info_cap_type {
> > > +	struct vfio_info_cap_header header;
> > > +	__u32 type;     /* global per bus driver */
> > > +	__u32 subtype;  /* type specific */
> > 
> > Do we really need both type and subtype?
> Then, if one device has several irqs, how can we identify them?
> Thanks.

Just assign multiple types?

cheers,
  Gerd

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Gerd Hoffmann]

> >   Hi,
> > 
> > > Instead of delivering page flip events, we choose to post display
> > > vblank event. Handling page flip events for both primary plane and
> > > cursor plane may make user space quite busy, although we have the
> > > mask/unmask mechansim for mitigation. Besides, there are some cases
> > > that guest app only uses one framebuffer for both drawing and display.
> > > In such case, guest OS won't do the plane page flip when the
> > > framebuffer is updated, thus the user land won't be notified about the
> > updated framebuffer.
> > 
> > What happens when the guest is idle and doesn't draw anything to the
> > framebuffer?
> The vblank event will be delivered to userspace as well, unless guest OS disable the pipe.
> Does it make sense to vfio/display?

Getting notified only in case there are actual display updates would be
a nice optimization, assuming the hardware is able to do that.  If the
guest pageflips this is obviously trivial.  Not sure this is possible in
case the guest renders directly to the frontbuffer.

What exactly happens when the guest OS disables the pipe?  Is a vblank
event delivered at least once?  That would be very useful because it
will be possible for userspace to stop polling altogether without
missing the "guest disabled pipe" event.

cheers,
  Gerd

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 2315 bytes --]

On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > Instead of delivering page flip events, we choose to post display
> > > > vblank event. Handling page flip events for both primary plane and
> > > > cursor plane may make user space quite busy, although we have the
> > > > mask/unmask mechansim for mitigation. Besides, there are some cases
> > > > that guest app only uses one framebuffer for both drawing and display.
> > > > In such case, guest OS won't do the plane page flip when the
> > > > framebuffer is updated, thus the user land won't be notified about the
> > > updated framebuffer.
> > > 
> > > What happens when the guest is idle and doesn't draw anything to the
> > > framebuffer?
> > The vblank event will be delivered to userspace as well, unless guest OS disable the pipe.
> > Does it make sense to vfio/display?
> 
> Getting notified only in case there are actual display updates would be
> a nice optimization, assuming the hardware is able to do that.  If the
> guest pageflips this is obviously trivial.  Not sure this is possible in
> case the guest renders directly to the frontbuffer.
> 
> What exactly happens when the guest OS disables the pipe?  Is a vblank
> event delivered at least once?  That would be very useful because it
> will be possible for userspace to stop polling altogether without
> missing the "guest disabled pipe" event.
> 

It looks like purpose to use vblank here is to replace user space
polling totally by kernel event? Which just act as display update
event to replace user space timer to make it query and update
planes? Although in theory vblank is not appropriate for this which
doesn't align with plane update or possible front buffer rendering at
all, but looks it's just a compromise e.g not sending event for every
cursor position change, etc.

I think we need to define semantics for this event properly, e.g user
space purely depends on this event for display update, the opportunity
for issuing this event is controlled by driver when it's necessary for
update, etc. Definitely not named as vblank event or only issue at vblank,
that need to happen for other plane change too.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Gerd Hoffmann]

On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > Instead of delivering page flip events, we choose to post display
> > > > > vblank event. Handling page flip events for both primary plane and
> > > > > cursor plane may make user space quite busy, although we have the
> > > > > mask/unmask mechansim for mitigation. Besides, there are some cases
> > > > > that guest app only uses one framebuffer for both drawing and display.
> > > > > In such case, guest OS won't do the plane page flip when the
> > > > > framebuffer is updated, thus the user land won't be notified about the
> > > > updated framebuffer.
> > > > 
> > > > What happens when the guest is idle and doesn't draw anything to the
> > > > framebuffer?
> > > The vblank event will be delivered to userspace as well, unless guest OS disable the pipe.
> > > Does it make sense to vfio/display?
> > 
> > Getting notified only in case there are actual display updates would be
> > a nice optimization, assuming the hardware is able to do that.  If the
> > guest pageflips this is obviously trivial.  Not sure this is possible in
> > case the guest renders directly to the frontbuffer.
> > 
> > What exactly happens when the guest OS disables the pipe?  Is a vblank
> > event delivered at least once?  That would be very useful because it
> > will be possible for userspace to stop polling altogether without
> > missing the "guest disabled pipe" event.
> > 
> 
> It looks like purpose to use vblank here is to replace user space
> polling totally by kernel event? Which just act as display update
> event to replace user space timer to make it query and update
> planes?

I think it makes sense to design it that way, so userspace will either
use the events (when supported by the driver) or a timer (fallback if
not) but not both.

> Although in theory vblank is not appropriate for this which
> doesn't align with plane update or possible front buffer rendering at
> all, but looks it's just a compromise e.g not sending event for every
> cursor position change, etc.
> 
> I think we need to define semantics for this event properly, e.g user
> space purely depends on this event for display update, the opportunity
> for issuing this event is controlled by driver when it's necessary for
> update, etc. Definitely not named as vblank event or only issue at vblank,
> that need to happen for other plane change too.

I think it should be "display update notification", i.e. userspace
should check for plane changes and update the display.

Most events will probably come from vblank (typically plane update are
actually committed at vblank time to avoid tearing, right?).  That is an
implementation detail though.

cheers,
  Gerd

Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 3366 bytes --]

On 2019.06.28 07:43:46 +0200, Gerd Hoffmann wrote:
> On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> > On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > > >   Hi,
> > > > > 
> > > > > > Instead of delivering page flip events, we choose to post display
> > > > > > vblank event. Handling page flip events for both primary plane and
> > > > > > cursor plane may make user space quite busy, although we have the
> > > > > > mask/unmask mechansim for mitigation. Besides, there are some cases
> > > > > > that guest app only uses one framebuffer for both drawing and display.
> > > > > > In such case, guest OS won't do the plane page flip when the
> > > > > > framebuffer is updated, thus the user land won't be notified about the
> > > > > updated framebuffer.
> > > > > 
> > > > > What happens when the guest is idle and doesn't draw anything to the
> > > > > framebuffer?
> > > > The vblank event will be delivered to userspace as well, unless guest OS disable the pipe.
> > > > Does it make sense to vfio/display?
> > > 
> > > Getting notified only in case there are actual display updates would be
> > > a nice optimization, assuming the hardware is able to do that.  If the
> > > guest pageflips this is obviously trivial.  Not sure this is possible in
> > > case the guest renders directly to the frontbuffer.
> > > 
> > > What exactly happens when the guest OS disables the pipe?  Is a vblank
> > > event delivered at least once?  That would be very useful because it
> > > will be possible for userspace to stop polling altogether without
> > > missing the "guest disabled pipe" event.
> > > 
> > 
> > It looks like purpose to use vblank here is to replace user space
> > polling totally by kernel event? Which just act as display update
> > event to replace user space timer to make it query and update
> > planes?
> 
> I think it makes sense to design it that way, so userspace will either
> use the events (when supported by the driver) or a timer (fallback if
> not) but not both.

Agree. It's more of a userspace choice.

> 
> > Although in theory vblank is not appropriate for this which
> > doesn't align with plane update or possible front buffer rendering at
> > all, but looks it's just a compromise e.g not sending event for every
> > cursor position change, etc.
> > 
> > I think we need to define semantics for this event properly, e.g user
> > space purely depends on this event for display update, the opportunity
> > for issuing this event is controlled by driver when it's necessary for
> > update, etc. Definitely not named as vblank event or only issue at vblank,
> > that need to happen for other plane change too.
> 
> I think it should be "display update notification", i.e. userspace
> should check for plane changes and update the display.
> 
> Most events will probably come from vblank (typically plane update are
> actually committed at vblank time to avoid tearing, right?).  That is an
> implementation detail though.
> 

Yeah, vblank should be a good time, although driver might also do
optimization e.g checking actual plane change between vblank to see if
there's any real change, etc. Also that will depend on driver
implementation.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to userspace

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Friday, June 28, 2019 1:44 PM
> To: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> alex.williamson@redhat.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Yuan,
> Hang <hang.yuan@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> Zhi A <zhi.a.wang@intel.com>
> Subject: Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> userspace
> 
> On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> > On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > > >   Hi,
> > > > >
> > > > > > Instead of delivering page flip events, we choose to post
> > > > > > display vblank event. Handling page flip events for both
> > > > > > primary plane and cursor plane may make user space quite busy,
> > > > > > although we have the mask/unmask mechansim for mitigation.
> > > > > > Besides, there are some cases that guest app only uses one
> framebuffer for both drawing and display.
> > > > > > In such case, guest OS won't do the plane page flip when the
> > > > > > framebuffer is updated, thus the user land won't be notified
> > > > > > about the
> > > > > updated framebuffer.
> > > > >
> > > > > What happens when the guest is idle and doesn't draw anything to
> > > > > the framebuffer?
> > > > The vblank event will be delivered to userspace as well, unless guest OS
> disable the pipe.
> > > > Does it make sense to vfio/display?
> > >
> > > Getting notified only in case there are actual display updates would
> > > be a nice optimization, assuming the hardware is able to do that.
> > > If the guest pageflips this is obviously trivial.  Not sure this is
> > > possible in case the guest renders directly to the frontbuffer.
> > >
> > > What exactly happens when the guest OS disables the pipe?  Is a
> > > vblank event delivered at least once?  That would be very useful
> > > because it will be possible for userspace to stop polling altogether
> > > without missing the "guest disabled pipe" event.
> > >
> >
> > It looks like purpose to use vblank here is to replace user space
> > polling totally by kernel event? Which just act as display update
> > event to replace user space timer to make it query and update planes?
> 
> I think it makes sense to design it that way, so userspace will either use the
> events (when supported by the driver) or a timer (fallback if
> not) but not both.
> 
> > Although in theory vblank is not appropriate for this which doesn't
> > align with plane update or possible front buffer rendering at all, but
> > looks it's just a compromise e.g not sending event for every cursor
> > position change, etc.
> >
> > I think we need to define semantics for this event properly, e.g user
> > space purely depends on this event for display update, the opportunity
> > for issuing this event is controlled by driver when it's necessary for
> > update, etc. Definitely not named as vblank event or only issue at
> > vblank, that need to happen for other plane change too.
> 
> I think it should be "display update notification", i.e. userspace should check
> for plane changes and update the display.
> 
> Most events will probably come from vblank (typically plane update are
> actually committed at vblank time to avoid tearing, right?).  That is an
Yes.
> implementation detail though.

We have two WIP branches: one is for vblank event delivery and the other one is for page flip event delivery. 
Repo:
- QEMU: https://github.com/intel/Igvtg-qemu.git
- Kernel: https://github.com/intel/gvt-linux.git
Two branches: topic/userspace_direct_flip_page_event and topic/userspace_direct_flip_vblank_event

With GTK UI, the user experience is bad on branch topic/userspace_direct_flip_page_event, as most of the CPU efforts are spent on event handling in user space.
However, with the DRM UI both of the two branches have good user experience, as the event handling in DRM UI is pretty simple.

Thanks.

BR,
Tina

> 
> 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 v3 0/4] Deliver vGPU display vblank event to userspace

[Zhang, Tina]

Hi,

How about GVT-g supports both vblank and page flip events. Then QEMU UI can mask/unmask the events to decide which kind of event is expected.
For DRM UI, it can decide to mask vblank event and use page flip events. We tried DRM UI with page flip events, the performance is great, even in the case of front buffer rendering. For the UI like GTK, vblank event is better. 

Besides, with the irq mask/unmask mechanism, userspace can dynamically choose between the UI timer and the events. 

Thanks.

BR,
Tina

> -----Original Message-----
> From: Zhang, Tina
> Sent: Friday, June 28, 2019 4:45 PM
> To: 'Gerd Hoffmann' <kraxel@redhat.com>; Zhenyu Wang
> <zhenyuw@linux.intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; alex.williamson@redhat.com; Lv, Zhiyuan
> <zhiyuan.lv@intel.com>; Yuan, Hang <hang.yuan@intel.com>; intel-gvt-
> dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: RE: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> userspace
> 
> 
> 
> > -----Original Message-----
> > From: intel-gvt-dev
> > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of Gerd
> > Hoffmann
> > Sent: Friday, June 28, 2019 1:44 PM
> > To: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > alex.williamson@redhat.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Yuan,
> > Hang <hang.yuan@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> > Zhi A <zhi.a.wang@intel.com>
> > Subject: Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> > userspace
> >
> > On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> > > On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > > > >   Hi,
> > > > > >
> > > > > > > Instead of delivering page flip events, we choose to post
> > > > > > > display vblank event. Handling page flip events for both
> > > > > > > primary plane and cursor plane may make user space quite
> > > > > > > busy, although we have the mask/unmask mechansim for
> mitigation.
> > > > > > > Besides, there are some cases that guest app only uses one
> > framebuffer for both drawing and display.
> > > > > > > In such case, guest OS won't do the plane page flip when the
> > > > > > > framebuffer is updated, thus the user land won't be notified
> > > > > > > about the
> > > > > > updated framebuffer.
> > > > > >
> > > > > > What happens when the guest is idle and doesn't draw anything
> > > > > > to the framebuffer?
> > > > > The vblank event will be delivered to userspace as well, unless
> > > > > guest OS
> > disable the pipe.
> > > > > Does it make sense to vfio/display?
> > > >
> > > > Getting notified only in case there are actual display updates
> > > > would be a nice optimization, assuming the hardware is able to do that.
> > > > If the guest pageflips this is obviously trivial.  Not sure this
> > > > is possible in case the guest renders directly to the frontbuffer.
> > > >
> > > > What exactly happens when the guest OS disables the pipe?  Is a
> > > > vblank event delivered at least once?  That would be very useful
> > > > because it will be possible for userspace to stop polling
> > > > altogether without missing the "guest disabled pipe" event.
> > > >
> > >
> > > It looks like purpose to use vblank here is to replace user space
> > > polling totally by kernel event? Which just act as display update
> > > event to replace user space timer to make it query and update planes?
> >
> > I think it makes sense to design it that way, so userspace will either
> > use the events (when supported by the driver) or a timer (fallback if
> > not) but not both.
> >
> > > Although in theory vblank is not appropriate for this which doesn't
> > > align with plane update or possible front buffer rendering at all,
> > > but looks it's just a compromise e.g not sending event for every
> > > cursor position change, etc.
> > >
> > > I think we need to define semantics for this event properly, e.g
> > > user space purely depends on this event for display update, the
> > > opportunity for issuing this event is controlled by driver when it's
> > > necessary for update, etc. Definitely not named as vblank event or
> > > only issue at vblank, that need to happen for other plane change too.
> >
> > I think it should be "display update notification", i.e. userspace
> > should check for plane changes and update the display.
> >
> > Most events will probably come from vblank (typically plane update are
> > actually committed at vblank time to avoid tearing, right?).  That is
> > an
> Yes.
> > implementation detail though.
> 
> We have two WIP branches: one is for vblank event delivery and the other
> one is for page flip event delivery.
> Repo:
> - QEMU: https://github.com/intel/Igvtg-qemu.git
> - Kernel: https://github.com/intel/gvt-linux.git
> Two branches: topic/userspace_direct_flip_page_event and
> topic/userspace_direct_flip_vblank_event
> 
> With GTK UI, the user experience is bad on branch
> topic/userspace_direct_flip_page_event, as most of the CPU efforts are
> spent on event handling in user space.
> However, with the DRM UI both of the two branches have good user
> experience, as the event handling in DRM UI is pretty simple.
> 
> Thanks.
> 
> BR,
> Tina
> 
> >
> > 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 v3 0/4] Deliver vGPU display vblank event to userspace

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 6476 bytes --]

On 2019.06.28 12:43:47 +0000, Zhang, Tina wrote:
> Hi,
> 
> How about GVT-g supports both vblank and page flip events. Then QEMU UI can mask/unmask the events to decide which kind of event is expected.
> For DRM UI, it can decide to mask vblank event and use page flip events. We tried DRM UI with page flip events, the performance is great, even in the case of front buffer rendering. For the UI like GTK, vblank event is better. 
> 
> Besides, with the irq mask/unmask mechanism, userspace can dynamically choose between the UI timer and the events. 
> 

The idea is to deliver an event to tell userspace that "guest has
display update, you need to refresh your UI". How driver
implementation uses either vblank or page flip to determine that is
driver specific, as same routine of vfio gfx interface will be used to
refresh on guest display.

If userspace doesn't set irq for vfio gfx display, it would just use
own timer.

> 
> > -----Original Message-----
> > From: Zhang, Tina
> > Sent: Friday, June 28, 2019 4:45 PM
> > To: 'Gerd Hoffmann' <kraxel@redhat.com>; Zhenyu Wang
> > <zhenyuw@linux.intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; alex.williamson@redhat.com; Lv, Zhiyuan
> > <zhiyuan.lv@intel.com>; Yuan, Hang <hang.yuan@intel.com>; intel-gvt-
> > dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
> > Subject: RE: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> > userspace
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: intel-gvt-dev
> > > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of Gerd
> > > Hoffmann
> > > Sent: Friday, June 28, 2019 1:44 PM
> > > To: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > Cc: Tian, Kevin <kevin.tian@intel.com>; kvm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Zhang, Tina <tina.zhang@intel.com>;
> > > alex.williamson@redhat.com; Lv, Zhiyuan <zhiyuan.lv@intel.com>; Yuan,
> > > Hang <hang.yuan@intel.com>; intel-gvt-dev@lists.freedesktop.org; Wang,
> > > Zhi A <zhi.a.wang@intel.com>
> > > Subject: Re: [RFC PATCH v3 0/4] Deliver vGPU display vblank event to
> > > userspace
> > >
> > > On Fri, Jun 28, 2019 at 11:21:49AM +0800, Zhenyu Wang wrote:
> > > > On 2019.06.27 12:31:33 +0200, Gerd Hoffmann wrote:
> > > > > > >   Hi,
> > > > > > >
> > > > > > > > Instead of delivering page flip events, we choose to post
> > > > > > > > display vblank event. Handling page flip events for both
> > > > > > > > primary plane and cursor plane may make user space quite
> > > > > > > > busy, although we have the mask/unmask mechansim for
> > mitigation.
> > > > > > > > Besides, there are some cases that guest app only uses one
> > > framebuffer for both drawing and display.
> > > > > > > > In such case, guest OS won't do the plane page flip when the
> > > > > > > > framebuffer is updated, thus the user land won't be notified
> > > > > > > > about the
> > > > > > > updated framebuffer.
> > > > > > >
> > > > > > > What happens when the guest is idle and doesn't draw anything
> > > > > > > to the framebuffer?
> > > > > > The vblank event will be delivered to userspace as well, unless
> > > > > > guest OS
> > > disable the pipe.
> > > > > > Does it make sense to vfio/display?
> > > > >
> > > > > Getting notified only in case there are actual display updates
> > > > > would be a nice optimization, assuming the hardware is able to do that.
> > > > > If the guest pageflips this is obviously trivial.  Not sure this
> > > > > is possible in case the guest renders directly to the frontbuffer.
> > > > >
> > > > > What exactly happens when the guest OS disables the pipe?  Is a
> > > > > vblank event delivered at least once?  That would be very useful
> > > > > because it will be possible for userspace to stop polling
> > > > > altogether without missing the "guest disabled pipe" event.
> > > > >
> > > >
> > > > It looks like purpose to use vblank here is to replace user space
> > > > polling totally by kernel event? Which just act as display update
> > > > event to replace user space timer to make it query and update planes?
> > >
> > > I think it makes sense to design it that way, so userspace will either
> > > use the events (when supported by the driver) or a timer (fallback if
> > > not) but not both.
> > >
> > > > Although in theory vblank is not appropriate for this which doesn't
> > > > align with plane update or possible front buffer rendering at all,
> > > > but looks it's just a compromise e.g not sending event for every
> > > > cursor position change, etc.
> > > >
> > > > I think we need to define semantics for this event properly, e.g
> > > > user space purely depends on this event for display update, the
> > > > opportunity for issuing this event is controlled by driver when it's
> > > > necessary for update, etc. Definitely not named as vblank event or
> > > > only issue at vblank, that need to happen for other plane change too.
> > >
> > > I think it should be "display update notification", i.e. userspace
> > > should check for plane changes and update the display.
> > >
> > > Most events will probably come from vblank (typically plane update are
> > > actually committed at vblank time to avoid tearing, right?).  That is
> > > an
> > Yes.
> > > implementation detail though.
> > 
> > We have two WIP branches: one is for vblank event delivery and the other
> > one is for page flip event delivery.
> > Repo:
> > - QEMU: https://github.com/intel/Igvtg-qemu.git
> > - Kernel: https://github.com/intel/gvt-linux.git
> > Two branches: topic/userspace_direct_flip_page_event and
> > topic/userspace_direct_flip_vblank_event
> > 
> > With GTK UI, the user experience is bad on branch
> > topic/userspace_direct_flip_page_event, as most of the CPU efforts are
> > spent on event handling in user space.
> > However, with the DRM UI both of the two branches have good user
> > experience, as the event handling in DRM UI is pretty simple.
> > 
> > Thanks.
> > 
> > BR,
> > Tina
> > 
> > >
> > > cheers,
> > >   Gerd
> > >
> > > _______________________________________________
> > > intel-gvt-dev mailing list
> > > intel-gvt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]