| Message ID | 20180913054745.6353-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: add vgpu edid support | expand |
On Thu, 13 Sep 2018 07:47:44 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: Some sort of commit log indicating the motivation for the change is always appreciated. > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/uapi/linux/vfio.h | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 1aa7b82e81..38b591e909 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -200,8 +200,11 @@ struct vfio_device_info { > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ > #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */ > +#define VFIO_DEVICE_FLAGS_EDID (1 << 5) /* Device supports edid */ > __u32 num_regions; /* Max region index + 1 */ > __u32 num_irqs; /* Max IRQ index + 1 */ > + __u32 edid_max_x; /* Max display width (zero == no limit) */ > + __u32 edid_max_y; /* Max display height (zero == no limit) */ > }; Hmm, not really what I was looking for, devices providing these values are only a very small subset of devices supported by vfio, so I was thinking a new flag bit would indicate the presence of a new __u32 cap_offset field and we'd define a capability something like: struct vfio_device_info_edid_cap { struct vfio_info_cap_header header; __u32 edid_max_x; __u32 edid_max_y; }; Therefore the capability is a generic expansion and the user would look for this specific edid capability within that. The protocol would be as we have today with region info where a call using the base vfio_device_info would return success regardless of argsz, but indicate capabilities are supported and return in argsz the size necessary to receive them. Another possible implementation would be via a vfio region, we already support device specific regions via capabilities with vfio_region_info, so we could have an edid region which could handle both input and output using a defined structure and protocol within the region. With an edid blob of up to 512 bytes now, that means the vendor driver would need to buffer writes to that section of the region until some sort of activation, possibly using another "register" within the field to trigger the link state and only processing the edid blob on link down to link up transition. So the virtual register space of the region might look like struct vfio_device_edid_region { __u32 max_x; /* read-only */ __u32 max_y; /* read-only */ __u32 link_state; /* read-write, 0=down, 1=up */ /* edid blob processed on 0->1 */ __u32 blob_size; /* read-write */ /* max size = region_size - end of blob_size */ __u8 blob[]; }; This is sort of the "we're defining our own hardware, so why not use a region as virtual register space for the device rather than throw a new ioctl at everything" approach. Thoughts? Thanks, Alex > #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7) > > @@ -602,6 +605,41 @@ struct vfio_device_ioeventfd { > > #define VFIO_DEVICE_IOEVENTFD _IO(VFIO_TYPE, VFIO_BASE + 16) > > +/** > + * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17, > + * struct vfio_device_set_gfx_edid) > + * > + * Set display link state and edid blob (for UP state). > + * > + * 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_device_set_gfx_edid { > + __u32 argsz; > + __u32 flags; > + /* in */ > + __u32 link_state; > +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 > +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 > + __u32 edid_size; > + __u8 edid_blob[512]; > +}; > + > +#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /**
Hi, > Another possible implementation would be via a vfio region, we already > support device specific regions via capabilities with vfio_region_info, > so we could have an edid region which could handle both input and > output using a defined structure and protocol within the region. With > an edid blob of up to 512 bytes now, that means the vendor driver would > need to buffer writes to that section of the region until some sort of > activation, possibly using another "register" within the field to > trigger the link state and only processing the edid blob on link down > to link up transition. Hmm, using a virtual register space makes things more complicated for no good reason. This is a configuration interface for qemu, not something we expose to the guest. So, I'm not a fan ... New revision of the vfio.h patch attached below, how does that look like? If it is ok I'll go continue with that next week (more verbose docs, update qemu code and test, ...) cheers, Gerd From 818f2ea4dd756d28908e58a32a2fdd0d197a28da Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann <kraxel@redhat.com> Date: Thu, 6 Sep 2018 16:17:17 +0200 Subject: [PATCH] vfio: add edid api for display (vgpu) devices. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/uapi/linux/vfio.h | 48 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 1aa7b82e81..901f279033 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -200,12 +200,25 @@ struct vfio_device_info { #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */ +#define VFIO_DEVICE_FLAGS_CAPS (1 << 5) /* cap_offset present */ __u32 num_regions; /* Max region index + 1 */ __u32 num_irqs; /* Max IRQ index + 1 */ + __u32 cap_offset; /* Offset within info struct of first cap */ }; #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7) /* + * FIXME: docs ... + */ +#define VFIO_DEVICE_INFO_CAP_EDID 1 + +struct vfio_device_info_edid_cap { + struct vfio_info_cap_header header; + __u32 max_x; /* Max display height (zero == no limit) */ + __u32 max_y; /* Max display height (zero == no limit) */ +}; + +/* * Vendor driver using Mediated device framework should provide device_api * attribute in supported type attribute groups. Device API string should be one * of the following corresponding to device flags in vfio_device_info structure. @@ -602,6 +615,41 @@ struct vfio_device_ioeventfd { #define VFIO_DEVICE_IOEVENTFD _IO(VFIO_TYPE, VFIO_BASE + 16) +/** + * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17, + * struct vfio_device_set_gfx_edid) + * + * Set display link state and edid blob (for UP state). + * + * 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_device_set_gfx_edid { + __u32 argsz; + __u32 flags; + /* in */ + __u32 link_state; +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 + __u32 edid_size; + __u8 edid_blob[512]; +}; + +#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17) + /* -------- API for Type1 VFIO IOMMU -------- */ /**
On Fri, 14 Sep 2018 14:25:52 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > > Another possible implementation would be via a vfio region, we already > > support device specific regions via capabilities with vfio_region_info, > > so we could have an edid region which could handle both input and > > output using a defined structure and protocol within the region. With > > an edid blob of up to 512 bytes now, that means the vendor driver would > > need to buffer writes to that section of the region until some sort of > > activation, possibly using another "register" within the field to > > trigger the link state and only processing the edid blob on link down > > to link up transition. > > Hmm, using a virtual register space makes things more complicated for no > good reason. This is a configuration interface for qemu, not something > we expose to the guest. So, I'm not a fan ... Ok, I'm curious what makes it more complicated though and why it matters that it's for QEMU vs exposed to the guest. From the user perspective, it's just a few pwrites rather than an ioctl. If we use the ioctl, I think we still need to improve the protocol, it's confusing that the user can specify both an EDID and a link state. Does the user need to toggle the link state when setting an EDID or does that happen automatically? How are link state vs EDID specified? Simply link_state=0 and edid_size=0 is reserved as not provided? > New revision of the vfio.h patch attached below, how does that look > like? If it is ok I'll go continue with that next week (more verbose > docs, update qemu code and test, ...) Yes, modulo the ioctl protocol questions above. Thanks, Alex > From 818f2ea4dd756d28908e58a32a2fdd0d197a28da Mon Sep 17 00:00:00 2001 > From: Gerd Hoffmann <kraxel@redhat.com> > Date: Thu, 6 Sep 2018 16:17:17 +0200 > Subject: [PATCH] vfio: add edid api for display (vgpu) devices. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/uapi/linux/vfio.h | 48 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 1aa7b82e81..901f279033 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -200,12 +200,25 @@ struct vfio_device_info { > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ > #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */ > +#define VFIO_DEVICE_FLAGS_CAPS (1 << 5) /* cap_offset present */ > __u32 num_regions; /* Max region index + 1 */ > __u32 num_irqs; /* Max IRQ index + 1 */ > + __u32 cap_offset; /* Offset within info struct of first cap */ > }; > #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7) > > /* > + * FIXME: docs ... > + */ > +#define VFIO_DEVICE_INFO_CAP_EDID 1 > + > +struct vfio_device_info_edid_cap { > + struct vfio_info_cap_header header; > + __u32 max_x; /* Max display height (zero == no limit) */ > + __u32 max_y; /* Max display height (zero == no limit) */ > +}; > + > +/* > * Vendor driver using Mediated device framework should provide device_api > * attribute in supported type attribute groups. Device API string should be one > * of the following corresponding to device flags in vfio_device_info structure. > @@ -602,6 +615,41 @@ struct vfio_device_ioeventfd { > > #define VFIO_DEVICE_IOEVENTFD _IO(VFIO_TYPE, VFIO_BASE + 16) > > +/** > + * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17, > + * struct vfio_device_set_gfx_edid) > + * > + * Set display link state and edid blob (for UP state). > + * > + * 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_device_set_gfx_edid { > + __u32 argsz; > + __u32 flags; > + /* in */ > + __u32 link_state; > +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 > +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 > + __u32 edid_size; > + __u8 edid_blob[512]; > +}; > + > +#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /**
On 2018.09.14 14:25:52 +0200, Gerd Hoffmann wrote: > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 1aa7b82e81..901f279033 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -200,12 +200,25 @@ struct vfio_device_info { > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ > #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */ > +#define VFIO_DEVICE_FLAGS_CAPS (1 << 5) /* cap_offset present */ > __u32 num_regions; /* Max region index + 1 */ > __u32 num_irqs; /* Max IRQ index + 1 */ > + __u32 cap_offset; /* Offset within info struct of first cap */ > }; > #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7) > > /* > + * FIXME: docs ... > + */ > +#define VFIO_DEVICE_INFO_CAP_EDID 1 > + > +struct vfio_device_info_edid_cap { > + struct vfio_info_cap_header header; > + __u32 max_x; /* Max display height (zero == no limit) */ > + __u32 max_y; /* Max display height (zero == no limit) */ > +}; As current virtual display for Intel vGPU is still emulating against real HW pipeline with same limitations, asked display developers that whether or not specific mode can work might still depend on current or future HW behavior. So could we add some hints on what kind of edid mode vfio device can operate? Some may support arbitrary modes, but some may only support standard modes.
> > +#define VFIO_DEVICE_INFO_CAP_EDID 1 > > + > > +struct vfio_device_info_edid_cap { > > + struct vfio_info_cap_header header; > > + __u32 max_x; /* Max display height (zero == no limit) */ > > + __u32 max_y; /* Max display height (zero == no limit) */ > > +}; > > As current virtual display for Intel vGPU is still emulating against real HW > pipeline with same limitations, asked display developers that whether or not > specific mode can work might still depend on current or future HW behavior. > So could we add some hints on what kind of edid mode vfio device can operate? > Some may support arbitrary modes, but some may only support standard modes. What kind of restrictions do we have here? Really to a fixed list of standard modes? Some testing (kaby lake) indicates y axis has no restrictions and x axis gets rounded up to the next multiple of 8 pixels (32 bytes), maybe to align scanlines with cachelines? Oh, and btw: Seems the resolution restriction (to 1024x768 for the smallest vgpu type) seems to not be enforced. Intentional? cheers, Gerd
On 2018.09.17 10:50:33 +0200, Gerd Hoffmann wrote: > > > +#define VFIO_DEVICE_INFO_CAP_EDID 1 > > > + > > > +struct vfio_device_info_edid_cap { > > > + struct vfio_info_cap_header header; > > > + __u32 max_x; /* Max display height (zero == no limit) */ > > > + __u32 max_y; /* Max display height (zero == no limit) */ > > > +}; > > > > As current virtual display for Intel vGPU is still emulating against real HW > > pipeline with same limitations, asked display developers that whether or not > > specific mode can work might still depend on current or future HW behavior. > > So could we add some hints on what kind of edid mode vfio device can operate? > > Some may support arbitrary modes, but some may only support standard modes. > > What kind of restrictions do we have here? Really to a fixed list of > standard modes? Restriction is still with HW differences, e.g for skl/kbl with ddi wrpll within min/max clock range which may generate any required frequency, but I've been told for bxt there're some gaps in clock range that could be generated. > > Some testing (kaby lake) indicates y axis has no restrictions and x axis > gets rounded up to the next multiple of 8 pixels (32 bytes), maybe to > align scanlines with cachelines? That should be plane stride requirement I think. > > Oh, and btw: Seems the resolution restriction (to 1024x768 for the > smallest vgpu type) seems to not be enforced. Intentional? > hmm, what do you mean here? Not enforce to have only one mode for vgpu type? Or can't change to other mode?
On Mon, Sep 17, 2018 at 05:15:50PM +0800, Zhenyu Wang wrote: > On 2018.09.17 10:50:33 +0200, Gerd Hoffmann wrote: > > > > +#define VFIO_DEVICE_INFO_CAP_EDID 1 > > > > + > > > > +struct vfio_device_info_edid_cap { > > > > + struct vfio_info_cap_header header; > > > > + __u32 max_x; /* Max display height (zero == no limit) */ > > > > + __u32 max_y; /* Max display height (zero == no limit) */ > > > > +}; > > > > > > As current virtual display for Intel vGPU is still emulating against real HW > > > pipeline with same limitations, asked display developers that whether or not > > > specific mode can work might still depend on current or future HW behavior. > > > So could we add some hints on what kind of edid mode vfio device can operate? > > > Some may support arbitrary modes, but some may only support standard modes. > > > > What kind of restrictions do we have here? Really to a fixed list of > > standard modes? > > Restriction is still with HW differences, e.g for skl/kbl with ddi wrpll > within min/max clock range which may generate any required frequency, but > I've been told for bxt there're some gaps in clock range that could be > generated. Ok, handling min/max clock (pixel clock I assume) should be easy. Ranges are more difficuilt ... But shouldn't the guest driver be able to cope with modes which can't be displayed? Monitors don't adjust edid to display driver capabilities either, so the non-working modes are probably simply dropped from the list of monitor modes ... > > Oh, and btw: Seems the resolution restriction (to 1024x768 for the > > smallest vgpu type) seems to not be enforced. Intentional? > > hmm, what do you mean here? Not enforce to have only one mode for vgpu type? > Or can't change to other mode? Using i915-GVTg_V5_8 type. Description says "resolution: 1024x768". The edid data lists 640x480, 800x600, 1024x768. I can set modes higher than 1024x768 though. Can create custom modes (using xrandr --newmode), attach them to the output (xrandr --addmode) and activate them (xrandr --mode). cheers, Gerd
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 1aa7b82e81..38b591e909 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -200,8 +200,11 @@ struct vfio_device_info { #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */ +#define VFIO_DEVICE_FLAGS_EDID (1 << 5) /* Device supports edid */ __u32 num_regions; /* Max region index + 1 */ __u32 num_irqs; /* Max IRQ index + 1 */ + __u32 edid_max_x; /* Max display width (zero == no limit) */ + __u32 edid_max_y; /* Max display height (zero == no limit) */ }; #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7) @@ -602,6 +605,41 @@ struct vfio_device_ioeventfd { #define VFIO_DEVICE_IOEVENTFD _IO(VFIO_TYPE, VFIO_BASE + 16) +/** + * VFIO_DEVICE_SET_GFX_EDID - _IOW(VFIO_TYPE, VFIO_BASE + 17, + * struct vfio_device_set_gfx_edid) + * + * Set display link state and edid blob (for UP state). + * + * 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_device_set_gfx_edid { + __u32 argsz; + __u32 flags; + /* in */ + __u32 link_state; +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 + __u32 edid_size; + __u8 edid_blob[512]; +}; + +#define VFIO_DEVICE_SET_GFX_EDID _IO(VFIO_TYPE, VFIO_BASE + 17) + /* -------- API for Type1 VFIO IOMMU -------- */ /**
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/uapi/linux/vfio.h | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)