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@xxxxxxxxxx> wrote:

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

> Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> ---
>  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 -------- */
>  
>  /**