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

Alex Williamson alex.williamson at redhat.com
Wed Sep 19 19:52:19 UTC 2018


On Tue, 18 Sep 2018 15:38:12 +0200
Gerd Hoffmann <kraxel at redhat.com> wrote:

No empty commit logs please.  There must be something to say about the
goal or motivation beyond the subject.

> Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> ---
>  include/uapi/linux/vfio.h | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82e81..78e5a37d83 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -301,6 +301,45 @@ struct vfio_region_info_cap_type {
>  #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
>  #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
>  
> +#define VFIO_REGION_TYPE_PCI_GFX                (1)

nit, what's the PCI dependency?

> +#define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
> +
> +/**
> + * Set display link state and edid blob.
> + *
> + * 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).

Who is responsible for this notification, the user interacting with
this region or the driver providing the region when a new edid is
provided?  This comment needs to state the expected API as clearly as
possible.

> + *
> + * @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.

What signals that the user edid_blob update is complete?  Should the
size be written before or after the blob?  Is the user required to
update the entire blob in a single write or can it be written
incrementally?

It might also be worth defining access widths, I see that you use
memcpy to support any width in mbochs, but we could define only native
field accesses for discrete registers if it makes the implementation
easier.

> + *
> + * Returns 0 on success, error code (such as -EINVAL) on failure.

Left over from ioctl.

> + */
> +struct vfio_region_gfx_edid {
> +	/* device capability hints (read only) */
> +	__u32 max_xres;
> +	__u32 max_yres;
> +	__u32 __reserved1[6];

Is the plan to use the version field within vfio_info_cap_header to
make use of these reserved fields later, ie. version 2 might define a
field from this reserved block?

> +
> +	/* device state (read/write) */
> +	__u32 link_state;
> +#define VFIO_DEVICE_GFX_LINK_STATE_UP    1
> +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
> +	__u32 edid_size;
> +	__u32 __reserved2[6];
> +
> +	/* edid blob (read/write) */
> +	__u8 edid_blob[512];

It seems the placement of this blob is what makes us feel like we need
to introduce reserved fields for later use, but we could instead define
an edid_offset read-only field so that the blob is always at the end of
whatever discrete fields we define.  Perhaps then we wouldn't even need
a read-only vs read-write section, simply define it per virtual
register.

Overall, I prefer this approach rather than adding yet more ioctls for
every feature and extension we add, thanks for implementing it.  What's
your impression vs ioctls?  Thanks,

Alex


More information about the intel-gvt-dev mailing list