[PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/uapi/linux/vfio.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 1aa7b82e81..38b591e909 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -200,8 +200,11 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
 #define VFIO_DEVICE_FLAGS_CCW	(1 << 4)	/* vfio-ccw device */
+#define VFIO_DEVICE_FLAGS_EDID	(1 << 5)	/* Device supports edid */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
+	__u32   edid_max_x;     /* Max display width (zero == no limit) */
+	__u32   edid_max_y;     /* Max display height (zero == no limit) */
 };
 #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
 
@@ -602,6 +605,41 @@ struct vfio_device_ioeventfd {
 
 #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
 
+/**
+ * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17,
+ *                                 struct vfio_device_set_gfx_edid)
+ *
+ * Set display link state and edid blob (for UP state).
+ *
+ * For the edid blob spec look here:
+ * https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
+ *
+ * The guest should be notified about edid changes, for example by
+ * setting the link status to down temporarely (emulate monitor
+ * hotplug).
+ *
+ * @link_state:
+ * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on.
+ * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off.
+ *
+ * @edid_size: Size of the edid data blob.
+ * @edid_blob: The actual edid data.
+ *
+ * Returns 0 on success, error code (such as -EINVAL) on failure.
+ */
+struct vfio_device_set_gfx_edid {
+	__u32 argsz;
+	__u32 flags;
+	/* in */
+	__u32 link_state;
+#define VFIO_DEVICE_GFX_LINK_STATE_UP    1
+#define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
+	__u32 edid_size;
+	__u8  edid_blob[512];
+};
+
+#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.9.3

[PATCH 2/2] vfio: add edid support to mbochs sample driver

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 samples/vfio-mdev/mbochs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 2535c3677c..6331871ff5 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -95,16 +95,24 @@ MODULE_PARM_DESC(mem, "megabytes available to " MBOCHS_NAME " devices");
 static const struct mbochs_type {
 	const char *name;
 	u32 mbytes;
+	u32 max_x;
+	u32 max_y;
 } mbochs_types[] = {
 	{
 		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_1,
 		.mbytes = 4,
+		.max_x  = 800,
+		.max_y  = 600,
 	}, {
 		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_2,
 		.mbytes = 16,
+		.max_x  = 1920,
+		.max_y  = 1440,
 	}, {
 		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_3,
 		.mbytes = 64,
+		.max_x  = 0,
+		.max_y  = 0,
 	},
 };
 
@@ -151,6 +159,7 @@ struct mdev_state {
 	u64 memsize;
 	struct page **pages;
 	pgoff_t pagecount;
+	u8 edid[512];
 
 	struct list_head dmabufs;
 	u32 active_id;
@@ -346,6 +355,11 @@ static void handle_mmio_read(struct mdev_state *mdev_state, u16 offset,
 	int index;
 
 	switch (offset) {
+	case 0x000 ... 0x3ff: /* edid block */
+		if (offset + count > sizeof(mdev_state->edid))
+			goto unhandled;
+		memcpy(buf, mdev_state->edid + offset, count);
+		break;
 	case 0x500 ... 0x515: /* bochs dispi interface */
 		if (count != 2)
 			goto unhandled;
@@ -983,9 +997,13 @@ static int mbochs_get_irq_info(struct mdev_device *mdev,
 static int mbochs_get_device_info(struct mdev_device *mdev,
 				  struct vfio_device_info *dev_info)
 {
+	struct mdev_state *mdev_state = mdev_get_drvdata(mdev);
+
 	dev_info->flags = VFIO_DEVICE_FLAGS_PCI;
 	dev_info->num_regions = VFIO_PCI_NUM_REGIONS;
 	dev_info->num_irqs = VFIO_PCI_NUM_IRQS;
+	dev_info->edid_max_x = mdev_state->type->max_x;
+	dev_info->edid_max_y = mdev_state->type->max_y;
 	return 0;
 }
 
@@ -1084,7 +1102,7 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd,
 			unsigned long arg)
 {
 	int ret = 0;
-	unsigned long minsz;
+	unsigned long minsz, outsz;
 	struct mdev_state *mdev_state;
 
 	mdev_state = mdev_get_drvdata(mdev);
@@ -1095,6 +1113,7 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		struct vfio_device_info info;
 
 		minsz = offsetofend(struct vfio_device_info, num_irqs);
+		outsz = offsetofend(struct vfio_device_info, edid_max_y);
 
 		if (copy_from_user(&info, (void __user *)arg, minsz))
 			return -EFAULT;
@@ -1108,7 +1127,9 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd,
 
 		memcpy(&mdev_state->dev_info, &info, sizeof(info));
 
-		if (copy_to_user((void __user *)arg, &info, minsz))
+		if (outsz > info.argsz)
+			outsz = info.argsz;
+		if (copy_to_user((void __user *)arg, &info, outsz))
 			return -EFAULT;
 
 		return 0;
@@ -1194,6 +1215,35 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd,
 		return mbochs_get_gfx_dmabuf(mdev, dmabuf_id);
 	}
 
+	case VFIO_DEVICE_SET_GFX_EDID:
+	{
+		struct vfio_device_set_gfx_edid *edid;
+
+		edid = kmalloc(sizeof(*edid), GFP_KERNEL);
+
+		minsz = offsetofend(struct vfio_device_set_gfx_edid,
+				    edid_blob);
+
+		if (copy_from_user(edid, (void __user *)arg, minsz)) {
+			kfree(edid);
+			return -EFAULT;
+		}
+
+		if (edid->argsz < minsz ||
+		    edid->edid_size > sizeof(mdev_state->edid)) {
+			kfree(edid);
+			return -EINVAL;
+		}
+
+		memset(mdev_state->edid, 0, sizeof(mdev_state->edid));
+		if (edid->link_state == VFIO_DEVICE_GFX_LINK_STATE_UP) {
+			memcpy(mdev_state->edid, edid->edid_blob, edid->edid_size);
+		}
+		kfree(edid);
+
+		return 0;
+	}
+
 	case VFIO_DEVICE_SET_IRQS:
 		return -EINVAL;
 
-- 
2.9.3

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Alex Williamson]

On Thu, 13 Sep 2018 07:47:44 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

Some sort of commit log indicating the motivation for the change is
always appreciated.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/uapi/linux/vfio.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..38b591e909 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -200,8 +200,11 @@ struct vfio_device_info {
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
>  #define VFIO_DEVICE_FLAGS_CCW	(1 << 4)	/* vfio-ccw device */
> +#define VFIO_DEVICE_FLAGS_EDID	(1 << 5)	/* Device supports edid */
>  	__u32	num_regions;	/* Max region index + 1 */
>  	__u32	num_irqs;	/* Max IRQ index + 1 */
> +	__u32   edid_max_x;     /* Max display width (zero == no limit) */
> +	__u32   edid_max_y;     /* Max display height (zero == no limit) */
>  };

Hmm, not really what I was looking for, devices providing these values
are only a very small subset of devices supported by vfio, so I was
thinking a new flag bit would indicate the presence of a new __u32
cap_offset field and we'd define a capability something like:

struct vfio_device_info_edid_cap {
	struct vfio_info_cap_header header;
	__u32 edid_max_x;
	__u32 edid_max_y;
};

Therefore the capability is a generic expansion and the user would look
for this specific edid capability within that.  The protocol would be
as we have today with region info where a call using the base
vfio_device_info would return success regardless of argsz, but indicate
capabilities are supported and return in argsz the size necessary to
receive them.

Another possible implementation would be via a vfio region, we already
support device specific regions via capabilities with vfio_region_info,
so we could have an edid region which could handle both input and
output using a defined structure and protocol within the region.  With
an edid blob of up to 512 bytes now, that means the vendor driver would
need to buffer writes to that section of the region until some sort of
activation, possibly using another "register" within the field to
trigger the link state and only processing the edid blob on link down
to link up transition.  So the virtual register space of the region
might look like

struct vfio_device_edid_region {
	__u32 max_x;	/* read-only */
	__u32 max_y;	/* read-only */
	__u32 link_state;	/* read-write, 0=down, 1=up */
				/* edid blob processed on 0->1 */
	__u32 blob_size;	/* read-write */
			/* max size = region_size - end of blob_size */
	__u8 blob[];
};

This is sort of the "we're defining our own hardware, so why not use a
region as virtual register space for the device rather than throw a new
ioctl at everything" approach.  Thoughts?  Thanks,

Alex

>  #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
>  
> @@ -602,6 +605,41 @@ struct vfio_device_ioeventfd {
>  
>  #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
>  
> +/**
> + * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17,
> + *                                 struct vfio_device_set_gfx_edid)
> + *
> + * Set display link state and edid blob (for UP state).
> + *
> + * For the edid blob spec look here:
> + * https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
> + *
> + * The guest should be notified about edid changes, for example by
> + * setting the link status to down temporarely (emulate monitor
> + * hotplug).
> + *
> + * @link_state:
> + * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on.
> + * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off.
> + *
> + * @edid_size: Size of the edid data blob.
> + * @edid_blob: The actual edid data.
> + *
> + * Returns 0 on success, error code (such as -EINVAL) on failure.
> + */
> +struct vfio_device_set_gfx_edid {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u32 link_state;
> +#define VFIO_DEVICE_GFX_LINK_STATE_UP    1
> +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
> +	__u32 edid_size;
> +	__u8  edid_blob[512];
> +};
> +
> +#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Gerd Hoffmann]

  Hi,

> Another possible implementation would be via a vfio region, we already
> support device specific regions via capabilities with vfio_region_info,
> so we could have an edid region which could handle both input and
> output using a defined structure and protocol within the region.  With
> an edid blob of up to 512 bytes now, that means the vendor driver would
> need to buffer writes to that section of the region until some sort of
> activation, possibly using another "register" within the field to
> trigger the link state and only processing the edid blob on link down
> to link up transition.

Hmm, using a virtual register space makes things more complicated for no
good reason.  This is a configuration interface for qemu, not something
we expose to the guest.  So, I'm not a fan ...

New revision of the vfio.h patch attached below, how does that look
like?  If it is ok I'll go continue with that next week (more verbose
docs, update qemu code and test, ...)

cheers,
  Gerd

From 818f2ea4dd756d28908e58a32a2fdd0d197a28da Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 6 Sep 2018 16:17:17 +0200
Subject: [PATCH] vfio: add edid api for display (vgpu) devices.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/uapi/linux/vfio.h | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 1aa7b82e81..901f279033 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -200,12 +200,25 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
 #define VFIO_DEVICE_FLAGS_CCW	(1 << 4)	/* vfio-ccw device */
+#define VFIO_DEVICE_FLAGS_CAPS	(1 << 5)	/* cap_offset present */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
+	__u32   cap_offset;	/* Offset within info struct of first cap */
 };
 #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
 
 /*
+ * FIXME: docs ...
+ */
+#define VFIO_DEVICE_INFO_CAP_EDID	1
+
+struct vfio_device_info_edid_cap {
+	struct vfio_info_cap_header header;
+	__u32   max_x;     /* Max display height (zero == no limit) */
+	__u32   max_y;     /* Max display height (zero == no limit) */
+};
+
+/*
  * Vendor driver using Mediated device framework should provide device_api
  * attribute in supported type attribute groups. Device API string should be one
  * of the following corresponding to device flags in vfio_device_info structure.
@@ -602,6 +615,41 @@ struct vfio_device_ioeventfd {
 
 #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
 
+/**
+ * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17,
+ *                                 struct vfio_device_set_gfx_edid)
+ *
+ * Set display link state and edid blob (for UP state).
+ *
+ * For the edid blob spec look here:
+ * https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
+ *
+ * The guest should be notified about edid changes, for example by
+ * setting the link status to down temporarely (emulate monitor
+ * hotplug).
+ *
+ * @link_state:
+ * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on.
+ * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off.
+ *
+ * @edid_size: Size of the edid data blob.
+ * @edid_blob: The actual edid data.
+ *
+ * Returns 0 on success, error code (such as -EINVAL) on failure.
+ */
+struct vfio_device_set_gfx_edid {
+	__u32 argsz;
+	__u32 flags;
+	/* in */
+	__u32 link_state;
+#define VFIO_DEVICE_GFX_LINK_STATE_UP    1
+#define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
+	__u32 edid_size;
+	__u8  edid_blob[512];
+};
+
+#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
-- 
2.9.3

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Alex Williamson]

On Fri, 14 Sep 2018 14:25:52 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > Another possible implementation would be via a vfio region, we already
> > support device specific regions via capabilities with vfio_region_info,
> > so we could have an edid region which could handle both input and
> > output using a defined structure and protocol within the region.  With
> > an edid blob of up to 512 bytes now, that means the vendor driver would
> > need to buffer writes to that section of the region until some sort of
> > activation, possibly using another "register" within the field to
> > trigger the link state and only processing the edid blob on link down
> > to link up transition.  
> 
> Hmm, using a virtual register space makes things more complicated for no
> good reason.  This is a configuration interface for qemu, not something
> we expose to the guest.  So, I'm not a fan ...

Ok, I'm curious what makes it more complicated though and why it
matters that it's for QEMU vs exposed to the guest.  From the user
perspective, it's just a few pwrites rather than an ioctl.  If we use
the ioctl, I think we still need to improve the protocol, it's
confusing that the user can specify both an EDID and a link state.
Does the user need to toggle the link state when setting an EDID or
does that happen automatically?  How are link state vs EDID specified?
Simply link_state=0 and edid_size=0 is reserved as not provided?

> New revision of the vfio.h patch attached below, how does that look
> like?  If it is ok I'll go continue with that next week (more verbose
> docs, update qemu code and test, ...)

Yes, modulo the ioctl protocol questions above.  Thanks,

Alex

> From 818f2ea4dd756d28908e58a32a2fdd0d197a28da Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Thu, 6 Sep 2018 16:17:17 +0200
> Subject: [PATCH] vfio: add edid api for display (vgpu) devices.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/uapi/linux/vfio.h | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..901f279033 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -200,12 +200,25 @@ struct vfio_device_info {
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
>  #define VFIO_DEVICE_FLAGS_CCW	(1 << 4)	/* vfio-ccw device */
> +#define VFIO_DEVICE_FLAGS_CAPS	(1 << 5)	/* cap_offset present */
>  	__u32	num_regions;	/* Max region index + 1 */
>  	__u32	num_irqs;	/* Max IRQ index + 1 */
> +	__u32   cap_offset;	/* Offset within info struct of first cap */
>  };
>  #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
>  
>  /*
> + * FIXME: docs ...
> + */
> +#define VFIO_DEVICE_INFO_CAP_EDID	1
> +
> +struct vfio_device_info_edid_cap {
> +	struct vfio_info_cap_header header;
> +	__u32   max_x;     /* Max display height (zero == no limit) */
> +	__u32   max_y;     /* Max display height (zero == no limit) */
> +};
> +
> +/*
>   * Vendor driver using Mediated device framework should provide device_api
>   * attribute in supported type attribute groups. Device API string should be one
>   * of the following corresponding to device flags in vfio_device_info structure.
> @@ -602,6 +615,41 @@ struct vfio_device_ioeventfd {
>  
>  #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
>  
> +/**
> + * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17,
> + *                                 struct vfio_device_set_gfx_edid)
> + *
> + * Set display link state and edid blob (for UP state).
> + *
> + * For the edid blob spec look here:
> + * https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
> + *
> + * The guest should be notified about edid changes, for example by
> + * setting the link status to down temporarely (emulate monitor
> + * hotplug).
> + *
> + * @link_state:
> + * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on.
> + * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off.
> + *
> + * @edid_size: Size of the edid data blob.
> + * @edid_blob: The actual edid data.
> + *
> + * Returns 0 on success, error code (such as -EINVAL) on failure.
> + */
> +struct vfio_device_set_gfx_edid {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u32 link_state;
> +#define VFIO_DEVICE_GFX_LINK_STATE_UP    1
> +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
> +	__u32 edid_size;
> +	__u8  edid_blob[512];
> +};
> +
> +#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Zhenyu Wang]

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

On 2018.09.14 14:25:52 +0200, Gerd Hoffmann wrote:
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..901f279033 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -200,12 +200,25 @@ struct vfio_device_info {
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
>  #define VFIO_DEVICE_FLAGS_CCW	(1 << 4)	/* vfio-ccw device */
> +#define VFIO_DEVICE_FLAGS_CAPS	(1 << 5)	/* cap_offset present */
>  	__u32	num_regions;	/* Max region index + 1 */
>  	__u32	num_irqs;	/* Max IRQ index + 1 */
> +	__u32   cap_offset;	/* Offset within info struct of first cap */
>  };
>  #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
>  
>  /*
> + * FIXME: docs ...
> + */
> +#define VFIO_DEVICE_INFO_CAP_EDID	1
> +
> +struct vfio_device_info_edid_cap {
> +	struct vfio_info_cap_header header;
> +	__u32   max_x;     /* Max display height (zero == no limit) */
> +	__u32   max_y;     /* Max display height (zero == no limit) */
> +};

As current virtual display for Intel vGPU is still emulating against real HW
pipeline with same limitations, asked display developers that whether or not
specific mode can work might still depend on current or future HW behavior.
So could we add some hints on what kind of edid mode vfio device can operate?
Some may support arbitrary modes, but some may only support standard modes.

-- 
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: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Gerd Hoffmann]

> > +#define VFIO_DEVICE_INFO_CAP_EDID	1
> > +
> > +struct vfio_device_info_edid_cap {
> > +	struct vfio_info_cap_header header;
> > +	__u32   max_x;     /* Max display height (zero == no limit) */
> > +	__u32   max_y;     /* Max display height (zero == no limit) */
> > +};
> 
> As current virtual display for Intel vGPU is still emulating against real HW
> pipeline with same limitations, asked display developers that whether or not
> specific mode can work might still depend on current or future HW behavior.
> So could we add some hints on what kind of edid mode vfio device can operate?
> Some may support arbitrary modes, but some may only support standard modes.

What kind of restrictions do we have here?  Really to a fixed list of
standard modes?

Some testing (kaby lake) indicates y axis has no restrictions and x axis
gets rounded up to the next multiple of 8 pixels (32 bytes), maybe to
align scanlines with cachelines?

Oh, and btw:  Seems the resolution restriction (to 1024x768 for the
smallest vgpu type) seems to not be enforced.  Intentional?

cheers,
  Gerd

Re: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Zhenyu Wang]

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

On 2018.09.17 10:50:33 +0200, Gerd Hoffmann wrote:
> > > +#define VFIO_DEVICE_INFO_CAP_EDID	1
> > > +
> > > +struct vfio_device_info_edid_cap {
> > > +	struct vfio_info_cap_header header;
> > > +	__u32   max_x;     /* Max display height (zero == no limit) */
> > > +	__u32   max_y;     /* Max display height (zero == no limit) */
> > > +};
> > 
> > As current virtual display for Intel vGPU is still emulating against real HW
> > pipeline with same limitations, asked display developers that whether or not
> > specific mode can work might still depend on current or future HW behavior.
> > So could we add some hints on what kind of edid mode vfio device can operate?
> > Some may support arbitrary modes, but some may only support standard modes.
> 
> What kind of restrictions do we have here?  Really to a fixed list of
> standard modes?

Restriction is still with HW differences, e.g for skl/kbl with ddi wrpll
within min/max clock range which may generate any required frequency, but
I've been told for bxt there're some gaps in clock range that could be
generated.

> 
> Some testing (kaby lake) indicates y axis has no restrictions and x axis
> gets rounded up to the next multiple of 8 pixels (32 bytes), maybe to
> align scanlines with cachelines?

That should be plane stride requirement I think.

> 
> Oh, and btw:  Seems the resolution restriction (to 1024x768 for the
> smallest vgpu type) seems to not be enforced.  Intentional?
> 

hmm, what do you mean here? Not enforce to have only one mode for vgpu type?
Or can't change to other mode?

-- 
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: [PATCH 1/2] vfio: add edid api for display (vgpu) devices.

[Gerd Hoffmann]

On Mon, Sep 17, 2018 at 05:15:50PM +0800, Zhenyu Wang wrote:
> On 2018.09.17 10:50:33 +0200, Gerd Hoffmann wrote:
> > > > +#define VFIO_DEVICE_INFO_CAP_EDID	1
> > > > +
> > > > +struct vfio_device_info_edid_cap {
> > > > +	struct vfio_info_cap_header header;
> > > > +	__u32   max_x;     /* Max display height (zero == no limit) */
> > > > +	__u32   max_y;     /* Max display height (zero == no limit) */
> > > > +};
> > > 
> > > As current virtual display for Intel vGPU is still emulating against real HW
> > > pipeline with same limitations, asked display developers that whether or not
> > > specific mode can work might still depend on current or future HW behavior.
> > > So could we add some hints on what kind of edid mode vfio device can operate?
> > > Some may support arbitrary modes, but some may only support standard modes.
> > 
> > What kind of restrictions do we have here?  Really to a fixed list of
> > standard modes?
> 
> Restriction is still with HW differences, e.g for skl/kbl with ddi wrpll
> within min/max clock range which may generate any required frequency, but
> I've been told for bxt there're some gaps in clock range that could be
> generated.

Ok, handling min/max clock (pixel clock I assume) should be easy.
Ranges are more difficuilt ...

But shouldn't the guest driver be able to cope with modes which can't be
displayed?  Monitors don't adjust edid to display driver capabilities
either, so the non-working modes are probably simply dropped from the
list of monitor modes ...

> > Oh, and btw:  Seems the resolution restriction (to 1024x768 for the
> > smallest vgpu type) seems to not be enforced.  Intentional?
> 
> hmm, what do you mean here? Not enforce to have only one mode for vgpu type?
> Or can't change to other mode?

Using i915-GVTg_V5_8 type.  Description says "resolution: 1024x768".
The edid data lists 640x480, 800x600, 1024x768.

I can set modes higher than 1024x768 though.  Can create custom modes
(using xrandr --newmode), attach them to the output (xrandr --addmode)
and activate them (xrandr --mode).

cheers,
  Gerd