lkml.org 
[lkml]   [2018]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices.
On Tue, 18 Sep 2018 15:38:12 +0200
Gerd Hoffmann <kraxel@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@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

\
 
 \ /
  Last update: 2018-09-19 21:53    [W:0.051 / U:5.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site