[PATCH v2 1/2] vfio: add edid api for display (vgpu) devices.
Gerd Hoffmann
kraxel at redhat.com
Thu Sep 20 08:11:06 UTC 2018
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 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?
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
More information about the intel-gvt-dev
mailing list