On Tue, 11 Sep 2018 06:58:29 +0200
Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:
> > > Subject: [PATCH] [RfC] vfio: edid interface
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > ---
> > > include/uapi/linux/vfio.h | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 42 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index 1aa7b82e81..9ac7dfb7c1 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -602,6 +602,48 @@ struct vfio_device_ioeventfd {
> > >
> > > #define VFIO_DEVICE_IOEVENTFD _IO(VFIO_TYPE, VFIO_BASE + 16)
> > >
> > > +/**
> > > + * VFIO_DEVICE_GFX_EDID_CAPS - _IOW(VFIO_TYPE, VFIO_BASE + 17,
> > > + * struct vfio_device_gfx_edid_caps)
> >
> > We haven't been very consistent with the _IOW vs _IOR vs _IOWR and as a
> > comment it's obviously not enforced, only meant to convey the nature of
> > the ioctl, but _IOW is probably the one choice that least matches. We
> > have other examples of using _IOR (ignoring that argsz is passed in)
> > and _IOWR, which is technically more accurate. Looks like we missed
> > this for the other GFX ioctls as well.
>
> Should be IORW (and IOW for SET), I'll fix.
>
> > I'd prefer to see "GET" in the ioctl name as well, perhaps
> > VFIO_DEVICE_GET_GFX_EDID_CAPS. Is this really EDID when it's only the
> > resolution though?
>
> It is supposed to return the capabilities of the device, such as
> 1024x768 resolution limit of the smallest intel vgpu type, so the edid
> blob generated by qemu will not contain resolutions higher than that.
We use get-info elsewhere in the vfio API for these sorts of things. In
fact, what's the value in a new ioctl vs adding a capability into the
existing VFIO_DEVICE_GET_INFO, much like we did for
VFIO_DEVICE_GET_REGION_INFO to describe sparse mmaps and vendor
specific types. We don't have a spare reserved field for the cap
offset, but we have argsz and flags to work around that. Just as you
have here, presence of the capability within the vfio_device_info would
imply the EDID set ioctl is available.
> > > + * VFIO_DEVICE_GFX_EDID_SET - _IOW(VFIO_TYPE, VFIO_BASE + 18,
> > > + * struct vfio_device_gfx_edid_set)
> >
> > Perhaps flip this around, VFIO_DEVICE_SET_GFX_EDID.
>
> I kind of like my naming, as the ioctls two edid ioctls will have the
> same prefix then (VFIO_DEVICE_GFX_EDID_*). Matter of taste though. And
> the other GFX ioctls don't match the scheme (VFIO_DEVICE_GFX_* prefix)
> either ...
The existing GFX ioctls match the scheme <class>_<action>_<object>:
VFIO_DEVICE_QUERY_GFX_PLANE
VFIO_DEVICE_GET_GFX_DMABUF
We're actually fairly consistent with this, some drifting into just
<class>_<action> when the action is operating on the instance of the
class:
VFIO_GET_API_VERSION
VFIO_CHECK_EXTENSION
VFIO_SET_IOMMU
VFIO_GROUP_GET_STATUS
VFIO_GROUP_SET_CONTAINER
VFIO_GROUP_UNSET_CONTAINER
VFIO_GROUP_GET_DEVICE_FD
VFIO_DEVICE_GET_INFO
VFIO_DEVICE_GET_REGION_INFO
VFIO_DEVICE_GET_IRQ_INFO
VFIO_DEVICE_SET_IRQS
VFIO_DEVICE_RESET
VFIO_DEVICE_GET_PCI_HOT_RESET_INFO
VFIO_DEVICE_PCI_HOT_RESET
VFIO_DEVICE_IOEVENTFD
VFIO_IOMMU_GET_INFO
VFIO_IOMMU_MAP_DMA
VFIO_IOMMU_UNMAP_DMA
VFIO_IOMMU_ENABLE
VFIO_IOMMU_DISABLE
VFIO_IOMMU_SPAPR_TCE_GET_INFO
VFIO_EEH_PE_OP
VFIO_IOMMU_SPAPR_REGISTER_MEMORY
VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY
VFIO_IOMMU_SPAPR_TCE_CREATE
VFIO_IOMMU_SPAPR_TCE_REMOVE
In the above proposal we have:
VFIO_DEVICE_GFX_EDID_SET <class>_<object>_<action>
VFIO_DEVICE_GFX_EDID_CAPS <class>_<object>_<noun>
Unless we're redefining the <class> to VFIO_DEVICE_GFX_EDID, but then
we're self inconsistent with GFX_PLANE and GFX_DMABUF, so I don't
consider these inline with the existing API.
> > > + * Set edid blob.
> > > + *
> > > + * Should trigger monitor hotplug emulation, to notifiy the guest os
> > > + * that the edid has changed.
> > > + *
> > > + */
> > > +struct vfio_device_gfx_edid_set {
> > > + __u32 argsz;
> > > + __u32 flags;
> > > + /* in */
> > > + __u8 edid[256];
> >
> > Is the format of this blob defined somewhere? Can we provide a
> > reference here to that format? Thanks,
>
> Spec seems not to be public. Wikipedia has this:
>
> https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
>
> Googling finds you pdfs of older revisions of the spec, but I've used
> the Wikipedia page to write the generator.
Ok, I'd like to see some sort of reference link, even if wikipedia, in a
comment for the ioctl so callers know the format of this blob. Thanks,
Alex
