RfC: vfio: add vgpu edid support?

[Alex Williamson]

On Fri, 7 Sep 2018 16:05:15 +0200
Gerd Hoffmann <kraxel at redhat.com> wrote:

>   Hi,
> 
> I consider adding EDID support to qemu, for display configuration.
> 
> qemu patches are here:
>     https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid
> 
> linux kernel patches are here:
>     https://git.kraxel.org/cgit/linux/log/?h=edid
> 
> Current (experimental) patches have support for the stdvga and
> virtio-vga.
> 
> I think it would be quite useful for vgpu too.  Unlike emulated devices
> vgpu's do not have a paravirtual display configuration path, because the
> standard way to configure the display is to simply read the edid from
> the monitor.
> 
> Intel has two hard-coded edid blobs for that (depending on vgpu type).
> Not sure how nvidia handles this, but probably simliar.  With qemu
> passing a edid blob for the virtual display instead of using the
> hardcoded blob we could make the display configuration much more
> flexible.
> 
> Ideally qemu would also be able to update the edid blob at any time, and
> the vgpu will notify the guest about it (probably by emulating a monitor
> hotplug event).  The guest can react on qemu window resizing then and
> adapt automatically, simliar to how it works with qxl and virtio-gpu.
> 
> The guest and the vgpu should be able to handle "odd" non-standard
> display resolutions like this (coming from random window resizing):
> 
>    Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm
>                   1212 1515 1551 1636 hborder 0
>                    840  844  848  869 vborder 0
>                   -hsync -vsync 
>                   VertFreq: 74 Hz, HorFreq: 65171 Hz
> 
> RfC patch for the vfio interface is below.
> 
> Comments?
> 
> cheers,
>   Gerd
> 
> =================== cut here ===================
> 
> From 556299e8c6280b1c4061fdae15491a013c22be98 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel at redhat.com>
> Date: Thu, 6 Sep 2018 16:17:17 +0200
> Subject: [PATCH] [RfC] vfio: edid interface
> 
> Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> ---
>  include/uapi/linux/vfio.h | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..9ac7dfb7c1 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -602,6 +602,48 @@ struct vfio_device_ioeventfd {
>  
>  #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
>  
> +/**
> + * VFIO_DEVICE_GFX_EDID_CAPS - _IOW(VFIO_TYPE, VFIO_BASE + 17,
> + *                                  struct vfio_device_gfx_edid_caps)

We haven't been very consistent with the _IOW vs _IOR vs _IOWR and as a
comment it's obviously not enforced, only meant to convey the nature of
the ioctl, but _IOW is probably the one choice that least matches.  We
have other examples of using _IOR (ignoring that argsz is passed in)
and _IOWR, which is technically more accurate.  Looks like we missed
this for the other GFX ioctls as well.

I'd prefer to see "GET" in the ioctl name as well, perhaps
VFIO_DEVICE_GET_GFX_EDID_CAPS.  Is this really EDID when it's only the
resolution though?

> + *
> + * Get edid capabilities.
> + *
> + * Drivers must support either none or both GFX_EDID ioctls,
> + * so the CAPS ioctl can also be used to probe for edid support.
> + *
> + * max_xres, max_yres - maximum display resolution supported.
> + *                      value "0" means no restriction.
> + *
> + */
> +struct vfio_device_gfx_edid_caps {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* out */
> +	__u32 max_xres;
> +	__u32 max_yres;
> +};
> +
> +#define VFIO_DEVICE_GFX_EDID_CAPS _IO(VFIO_TYPE, VFIO_BASE + 17)
> +
> +/**
> + * VFIO_DEVICE_GFX_EDID_SET - _IOW(VFIO_TYPE, VFIO_BASE + 18,
> + *                                 struct vfio_device_gfx_edid_set)

Perhaps flip this around, VFIO_DEVICE_SET_GFX_EDID.

> + *
> + * Set edid blob.
> + *
> + * Should trigger monitor hotplug emulation, to notifiy the guest os
> + * that the edid has changed.
> + *
> + */
> +struct vfio_device_gfx_edid_set {
> +	__u32 argsz;
> +	__u32 flags;
> +	/* in */
> +	__u8  edid[256];

Is the format of this blob defined somewhere?  Can we provide a
reference here to that format?  Thanks,

Alex

> +};
> +
> +#define VFIO_DEVICE_GFX_EDID_SET _IO(VFIO_TYPE, VFIO_BASE + 18)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**