| Message ID | 20180918133813.1845-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: add vgpu edid support | expand |
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
On Wed, Sep 19, 2018 at 01:52:19PM -0600, Alex Williamson wrote: > 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? None I guess, just copy&paste from the (pci) vendor cap above ... Will rename it. > > + * 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? My qemu test code first sets link-state = down, then updates edid, then sets link-state = up. I can document that as official update protocol. Drivers should be able to map it do emulation events pretty straight forward. Except maybe for the final down-up transition, drivers might have to delay this to make sure the guest has seen the up-down transition. > 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. Makes sense for the u32 registers (especially the writable ones). For mbochs the virtual hardware has no link state notification, so I can just store whatever I get, then check link-state when the guest attempts to read the edid. When actually implementing link-state notification it is probably useful to not have to deal with a half-written link-state field ... > > + */ > > +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? Yes. > > + /* 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. Yes, sounds sensible. > Perhaps then we wouldn't even need a read-only vs read-write section, > simply define it per virtual register. I'll look into this. > 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? I'd still prefer ioctls, but this is workable too. So if you like it better this way lets go for it. cheers, Gerd
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) +#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). + * + * @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_region_gfx_edid { + /* device capability hints (read only) */ + __u32 max_xres; + __u32 max_yres; + __u32 __reserved1[6]; + + /* 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]; +}; + /* * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped * which allows direct access to non-MSIX registers which happened to be within
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/uapi/linux/vfio.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)