On Fri, 14 Sep 2018 14:25:52 +0200
Gerd Hoffmann <kraxel@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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 -------- */
>
> /**
