| Message ID | 20180907140515.3i234v5a4ikrq6ob@sirius.home.kraxel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RfC: vfio: add vgpu edid support? | expand |
> -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On > Behalf Of Gerd Hoffmann > Sent: Friday, September 7, 2018 10:05 PM > To: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; Kirti Wankhede > <kwankhede@nvidia.com>; Alex Williamson <alex.williamson@redhat.com> > Subject: RfC: vfio: add vgpu edid support? > > Hi, > > I consider adding EDID support to qemu, for display configuration. > > qemu patches are here: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid > > linux kernel patches are here: > https://git.kraxel.org/cgit/linux/log/?h=edid > > Current (experimental) patches have support for the stdvga and virtio-vga. > > I think it would be quite useful for vgpu too. Unlike emulated devices vgpu's do > not have a paravirtual display configuration path, because the standard way to > configure the display is to simply read the edid from the monitor. > > Intel has two hard-coded edid blobs for that (depending on vgpu type). > Not sure how nvidia handles this, but probably simliar. With qemu passing a > edid blob for the virtual display instead of using the hardcoded blob we could > make the display configuration much more flexible. Sounds interesting. People are asking us to add more display modes. With this proposal, we could provide any required mode in a flexible way. But the hard-coded resolution might still be needed. As it is used for vgpu resources allocation during vgpu creation before running a qemu. And a vgpu cannot support a resolution bigger than the one used for its resources allocation. So some logic might be needed to make sure to only allow qemu to pass an edid with a smaller resolution than the one (a.k.a the hard-coded resolution) used for populating the vgpu. Thanks. BR, Tina > > Ideally qemu would also be able to update the edid blob at any time, and the > vgpu will notify the guest about it (probably by emulating a monitor hotplug > event). The guest can react on qemu window resizing then and adapt > automatically, simliar to how it works with qxl and virtio-gpu. > > The guest and the vgpu should be able to handle "odd" non-standard display > resolutions like this (coming from random window resizing): > > Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm > 1212 1515 1551 1636 hborder 0 > 840 844 848 869 vborder 0 > -hsync -vsync > VertFreq: 74 Hz, HorFreq: 65171 Hz > > RfC patch for the vfio interface is below. > > Comments? > > cheers, > Gerd > > =================== cut here =================== > > From 556299e8c6280b1c4061fdae15491a013c22be98 Mon Sep 17 00:00:00 > 2001 > From: Gerd Hoffmann <kraxel@redhat.com> > Date: Thu, 6 Sep 2018 16:17:17 +0200 > Subject: [PATCH] [RfC] vfio: edid interface > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > 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) > + * > + * Get edid capabilities. > + * > + * Drivers must support either none or both GFX_EDID ioctls, > + * so the CAPS ioctl can also be used to probe for edid support. > + * > + * max_xres, max_yres - maximum display resolution supported. > + * value "0" means no restriction. > + * > + */ > +struct vfio_device_gfx_edid_caps { > + __u32 argsz; > + __u32 flags; > + /* out */ > + __u32 max_xres; > + __u32 max_yres; > +}; > + > +#define VFIO_DEVICE_GFX_EDID_CAPS _IO(VFIO_TYPE, VFIO_BASE + 17) > + > +/** > + * VFIO_DEVICE_GFX_EDID_SET - _IOW(VFIO_TYPE, VFIO_BASE + 18, > + * struct vfio_device_gfx_edid_set) > + * > + * 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]; > +}; > + > +#define VFIO_DEVICE_GFX_EDID_SET _IO(VFIO_TYPE, VFIO_BASE + 18) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /** > -- > 2.9.3
On 2018.09.07 16:05:15 +0200, Gerd Hoffmann wrote: > Hi, > > I consider adding EDID support to qemu, for display configuration. > > qemu patches are here: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid > > linux kernel patches are here: > https://git.kraxel.org/cgit/linux/log/?h=edid > > Current (experimental) patches have support for the stdvga and > virtio-vga. > > I think it would be quite useful for vgpu too. Unlike emulated devices > vgpu's do not have a paravirtual display configuration path, because the > standard way to configure the display is to simply read the edid from > the monitor. > > Intel has two hard-coded edid blobs for that (depending on vgpu type). > Not sure how nvidia handles this, but probably simliar. With qemu > passing a edid blob for the virtual display instead of using the > hardcoded blob we could make the display configuration much more > flexible. > > Ideally qemu would also be able to update the edid blob at any time, and > the vgpu will notify the guest about it (probably by emulating a monitor > hotplug event). The guest can react on qemu window resizing then and > adapt automatically, simliar to how it works with qxl and virtio-gpu. What's the frequency for those edid update during resizing? Looks it's possible to bring hotplug storm that I'm not sure if all guest drivers for gvt has proper handling. > > The guest and the vgpu should be able to handle "odd" non-standard > display resolutions like this (coming from random window resizing): > > Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm > 1212 1515 1551 1636 hborder 0 > 840 844 848 869 vborder 0 > -hsync -vsync > VertFreq: 74 Hz, HorFreq: 65171 Hz > Some odd timing might not be supported, e.g can't get a sane PLL setting calculation for clock required. As gvt exposes a virtual display pipeline following hw definition, so guest driver still depend on sane output, I think that may just go wrong with arbitrary mode.. > +/** > + * VFIO_DEVICE_GFX_EDID_SET - _IOW(VFIO_TYPE, VFIO_BASE + 18, > + * struct vfio_device_gfx_edid_set) > + * > + * 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]; > +}; > + I assume you have defined return value for the set too right? In case for driver can't handle or invalid edid blob, etc. > +#define VFIO_DEVICE_GFX_EDID_SET _IO(VFIO_TYPE, VFIO_BASE + 18) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /** > -- > 2.9.3 > > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Hi, > > Intel has two hard-coded edid blobs for that (depending on vgpu type). > > Not sure how nvidia handles this, but probably simliar. With qemu passing a > > edid blob for the virtual display instead of using the hardcoded blob we could > > make the display configuration much more flexible. > > Sounds interesting. People are asking us to add more display modes. With this > proposal, we could provide any required mode in a flexible way. Yes, exactly. > But the hard-coded resolution might still be needed. As it is used for vgpu > resources allocation during vgpu creation before running a qemu. And a > vgpu cannot support a resolution bigger than the one used for its resources > allocation. The CAPS ioctl below should handle that: > > +/** > > + * VFIO_DEVICE_GFX_EDID_CAPS - _IOW(VFIO_TYPE, VFIO_BASE + 17, > > + * struct vfio_device_gfx_edid_caps) > > + * > > + * Get edid capabilities. > > + * > > + * Drivers must support either none or both GFX_EDID ioctls, > > + * so the CAPS ioctl can also be used to probe for edid support. > > + * > > + * max_xres, max_yres - maximum display resolution supported. > > + * value "0" means no restriction. > > + * > > + */ > > +struct vfio_device_gfx_edid_caps { > > + __u32 argsz; > > + __u32 flags; > > + /* out */ > > + __u32 max_xres; > > + __u32 max_yres; > > +}; cheers, Gerd
Hi, > > Ideally qemu would also be able to update the edid blob at any time, and > > the vgpu will notify the guest about it (probably by emulating a monitor > > hotplug event). The guest can react on qemu window resizing then and > > adapt automatically, simliar to how it works with qxl and virtio-gpu. > > What's the frequency for those edid update during resizing? Looks it's > possible to bring hotplug storm that I'm not sure if all guest drivers > for gvt has proper handling. At most once per second. The window resize typically creates a flood of the these events. qemu doesn't forward them to the guest as-is, but instead sets up a timer, firing one second later. The next event coming in just rearms the timer. When the timer goes off the guest is actually notified. > > The guest and the vgpu should be able to handle "odd" non-standard > > display resolutions like this (coming from random window resizing): > > > > Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm > > 1212 1515 1551 1636 hborder 0 > > 840 844 848 869 vborder 0 > > -hsync -vsync > > VertFreq: 74 Hz, HorFreq: 65171 Hz > > > > Some odd timing might not be supported, e.g can't get a sane PLL setting > calculation for clock required. As gvt exposes a virtual display pipeline > following hw definition, so guest driver still depend on sane output, I > think that may just go wrong with arbitrary mode.. I've suspected that might become an issue. What kind of restrictions exist for the clock? > > +struct vfio_device_gfx_edid_set { > > + __u32 argsz; > > + __u32 flags; > > + /* in */ > > + __u8 edid[256]; > > +}; > > + > > I assume you have defined return value for the set too right? In case > for driver can't handle or invalid edid blob, etc. Just return -EINVAL then, as usual (but I can update the comment explicitly saying so). cheers, Gerd
On Fri, 7 Sep 2018 16:05:15 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > I consider adding EDID support to qemu, for display configuration. > > qemu patches are here: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid > > linux kernel patches are here: > https://git.kraxel.org/cgit/linux/log/?h=edid > > Current (experimental) patches have support for the stdvga and > virtio-vga. > > I think it would be quite useful for vgpu too. Unlike emulated devices > vgpu's do not have a paravirtual display configuration path, because the > standard way to configure the display is to simply read the edid from > the monitor. > > Intel has two hard-coded edid blobs for that (depending on vgpu type). > Not sure how nvidia handles this, but probably simliar. With qemu > passing a edid blob for the virtual display instead of using the > hardcoded blob we could make the display configuration much more > flexible. > > Ideally qemu would also be able to update the edid blob at any time, and > the vgpu will notify the guest about it (probably by emulating a monitor > hotplug event). The guest can react on qemu window resizing then and > adapt automatically, simliar to how it works with qxl and virtio-gpu. > > The guest and the vgpu should be able to handle "odd" non-standard > display resolutions like this (coming from random window resizing): > > Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm > 1212 1515 1551 1636 hborder 0 > 840 844 848 869 vborder 0 > -hsync -vsync > VertFreq: 74 Hz, HorFreq: 65171 Hz > > RfC patch for the vfio interface is below. > > Comments? > > cheers, > Gerd > > =================== cut here =================== > > From 556299e8c6280b1c4061fdae15491a013c22be98 Mon Sep 17 00:00:00 2001 > From: Gerd Hoffmann <kraxel@redhat.com> > Date: Thu, 6 Sep 2018 16:17:17 +0200 > Subject: [PATCH] [RfC] vfio: edid interface > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > 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. 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? > + * > + * Get edid capabilities. > + * > + * Drivers must support either none or both GFX_EDID ioctls, > + * so the CAPS ioctl can also be used to probe for edid support. > + * > + * max_xres, max_yres - maximum display resolution supported. > + * value "0" means no restriction. > + * > + */ > +struct vfio_device_gfx_edid_caps { > + __u32 argsz; > + __u32 flags; > + /* out */ > + __u32 max_xres; > + __u32 max_yres; > +}; > + > +#define VFIO_DEVICE_GFX_EDID_CAPS _IO(VFIO_TYPE, VFIO_BASE + 17) > + > +/** > + * 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. > + * > + * 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, Alex > +}; > + > +#define VFIO_DEVICE_GFX_EDID_SET _IO(VFIO_TYPE, VFIO_BASE + 18) > + > /* -------- API for Type1 VFIO IOMMU -------- */ > > /**
On 2018.09.10 12:43:04 +0200, Gerd Hoffmann wrote: > Hi, > > > > Ideally qemu would also be able to update the edid blob at any time, and > > > the vgpu will notify the guest about it (probably by emulating a monitor > > > hotplug event). The guest can react on qemu window resizing then and > > > adapt automatically, simliar to how it works with qxl and virtio-gpu. > > > > What's the frequency for those edid update during resizing? Looks it's > > possible to bring hotplug storm that I'm not sure if all guest drivers > > for gvt has proper handling. > > At most once per second. > > The window resize typically creates a flood of the these events. qemu > doesn't forward them to the guest as-is, but instead sets up a timer, > firing one second later. The next event coming in just rearms the > timer. When the timer goes off the guest is actually notified. > Then that should be fine. Current intel display driver has 5 hotplug irq threshold within one second. > > > The guest and the vgpu should be able to handle "odd" non-standard > > > display resolutions like this (coming from random window resizing): > > > > > > Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm > > > 1212 1515 1551 1636 hborder 0 > > > 840 844 848 869 vborder 0 > > > -hsync -vsync > > > VertFreq: 74 Hz, HorFreq: 65171 Hz > > > > > > > Some odd timing might not be supported, e.g can't get a sane PLL setting > > calculation for clock required. As gvt exposes a virtual display pipeline > > following hw definition, so guest driver still depend on sane output, I > > think that may just go wrong with arbitrary mode.. > > I've suspected that might become an issue. > What kind of restrictions exist for the clock? > I need to double check this with our display developer on current supported HW. > > > +struct vfio_device_gfx_edid_set { > > > + __u32 argsz; > > > + __u32 flags; > > > + /* in */ > > > + __u8 edid[256]; > > > +}; > > > + > > > > I assume you have defined return value for the set too right? In case > > for driver can't handle or invalid edid blob, etc. > > Just return -EINVAL then, as usual (but I can update the comment > explicitly saying so). > thanks
> > Subject: [PATCH] [RfC] vfio: edid interface > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > 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. > > + * 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 ... > > + * 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. cheers, Gerd
> -----Original Message----- > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Monday, September 10, 2018 6:35 PM > To: Zhang, Tina <tina.zhang@intel.com> > Cc: intel-gvt-dev@lists.freedesktop.org; kvm@vger.kernel.org; Kirti Wankhede > <kwankhede@nvidia.com>; Alex Williamson <alex.williamson@redhat.com> > Subject: Re: vfio: add vgpu edid support? > > Hi, > > > > Intel has two hard-coded edid blobs for that (depending on vgpu type). > > > Not sure how nvidia handles this, but probably simliar. With qemu > > > passing a edid blob for the virtual display instead of using the > > > hardcoded blob we could make the display configuration much more flexible. > > > > Sounds interesting. People are asking us to add more display modes. > > With this proposal, we could provide any required mode in a flexible way. > > Yes, exactly. > > > But the hard-coded resolution might still be needed. As it is used for > > vgpu resources allocation during vgpu creation before running a qemu. > > And a vgpu cannot support a resolution bigger than the one used for > > its resources allocation. > > The CAPS ioctl below should handle that: > > > > +/** > > > + * VFIO_DEVICE_GFX_EDID_CAPS - _IOW(VFIO_TYPE, VFIO_BASE + 17, > > > + * struct vfio_device_gfx_edid_caps) > > > + * > > > + * Get edid capabilities. > > > + * > > > + * Drivers must support either none or both GFX_EDID ioctls, > > > + * so the CAPS ioctl can also be used to probe for edid support. > > > + * > > > + * max_xres, max_yres - maximum display resolution supported. > > > + * value "0" means no restriction. > > > + * > > > + */ > > > +struct vfio_device_gfx_edid_caps { > > > + __u32 argsz; > > > + __u32 flags; > > > + /* out */ > > > + __u32 max_xres; > > > + __u32 max_yres; > > > +}; Yeah, that's workable. And I also find in qemu_edid_generate(), the generated edid has display port as the video input type. That's compatible with gvt-g hard-coded edid. So, do you plan to use VFIO_DEVICE_GFX_EDID_SET dynamically? To change the display resolution during guest running? Could you elaborate more on what the use case would be? Besides, I think the proposed "VFIO_DEVICE_GFX_EDID_SET" can also provide host a capability to trigger display monitor hot-plug event to guest, when host wants to temporarily turn off vfio/display. Qemu can pass a special edid to vendor driver to tell vendor driver to plug the guest display monitor out. Thanks. BR, Tina > > cheers, > Gerd
Hi, > So, do you plan to use VFIO_DEVICE_GFX_EDID_SET dynamically? To change > the display resolution during guest running? Yes. > Could you elaborate more on what the use case would be? When the gtk or sdl window is resized by the user qemu will notify the guest about the new window size. GraphicHwOps->ui_info is the notification callback. https://git.kraxel.org/cgit/qemu/commit/?h=sirius/edid&id=8334ef0362e80a9994d4c35d12c0e929568daeff > Besides, I think the proposed "VFIO_DEVICE_GFX_EDID_SET" can also provide > host a capability to trigger display monitor hot-plug event to guest, when host > wants to temporarily turn off vfio/display. That makes sense indeed. > Qemu can pass a special edid to vendor driver to tell vendor driver to > plug the guest display monitor out. I'd add a field to vfio_device_gfx_edid_set instead, or use a flags bit. cheers, Gerd
On 2018.09.11 13:20:08 +0200, Gerd Hoffmann wrote: > > Besides, I think the proposed "VFIO_DEVICE_GFX_EDID_SET" can also provide > > host a capability to trigger display monitor hot-plug event to guest, when host > > wants to temporarily turn off vfio/display. > > That makes sense indeed. > For virtual display, I don't think doing DPMS off alike operation has practical usage, as host can control how/whether to show vgpu display. vgpu guest can apply display off on virtual display, which host can handle through vfio gfx plane ioctl. Even with my own hack to show vgpu display directly through DRM/KMS, it's still host's policy to do any monitor off if required. And turning vgpu display off it's different situation virtual display driver needs to handle e.g emulate dis-connected state, etc.
On 2018.09.12 07:09:59 +0200, Gerd Hoffmann wrote: > On Wed, Sep 12, 2018 at 11:53:16AM +0800, Zhenyu Wang wrote: > > On 2018.09.11 13:20:08 +0200, Gerd Hoffmann wrote: > > > > Besides, I think the proposed "VFIO_DEVICE_GFX_EDID_SET" can also provide > > > > host a capability to trigger display monitor hot-plug event to guest, when host > > > > wants to temporarily turn off vfio/display. > > > > > > That makes sense indeed. > > > > > > > For virtual display, I don't think doing DPMS off alike operation has > > practical usage, as host can control how/whether to show vgpu display. > > vgpu guest can apply display off on virtual display, which host can > > handle through vfio gfx plane ioctl. > > It is not DPMS. > It also is not display control. > I see, if it doesn't mean display control, that's fine to me. > It is about emulating link status, to give the guest hints how to > configure the display. Setting display_status to "disconnected" would > be the same as the user powering off the monitor in the physical world. > hmm, just doubt what kind of EDID can give that hint?
On Wed, Sep 12, 2018 at 11:53:16AM +0800, Zhenyu Wang wrote: > On 2018.09.11 13:20:08 +0200, Gerd Hoffmann wrote: > > > Besides, I think the proposed "VFIO_DEVICE_GFX_EDID_SET" can also provide > > > host a capability to trigger display monitor hot-plug event to guest, when host > > > wants to temporarily turn off vfio/display. > > > > That makes sense indeed. > > > > For virtual display, I don't think doing DPMS off alike operation has > practical usage, as host can control how/whether to show vgpu display. > vgpu guest can apply display off on virtual display, which host can > handle through vfio gfx plane ioctl. It is not DPMS. It also is not display control. It is about emulating link status, to give the guest hints how to configure the display. Setting display_status to "disconnected" would be the same as the user powering off the monitor in the physical world. cheers, Gerd
> > It is about emulating link status, to give the guest hints how to > > configure the display. Setting display_status to "disconnected" would > > be the same as the user powering off the monitor in the physical world. > > > > hmm, just doubt what kind of EDID can give that hint? New field in set_edid ioctl struct. cheers, Gerd
On Tue, 11 Sep 2018 06:58:29 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > > > Subject: [PATCH] [RfC] vfio: edid interface > > > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > --- > > > 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
=================== cut here =================== From 556299e8c6280b1c4061fdae15491a013c22be98 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann <kraxel@redhat.com> Date: Thu, 6 Sep 2018 16:17:17 +0200 Subject: [PATCH] [RfC] vfio: edid interface Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- 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) + * + * Get edid capabilities. + * + * Drivers must support either none or both GFX_EDID ioctls, + * so the CAPS ioctl can also be used to probe for edid support. + * + * max_xres, max_yres - maximum display resolution supported. + * value "0" means no restriction. + * + */ +struct vfio_device_gfx_edid_caps { + __u32 argsz; + __u32 flags; + /* out */ + __u32 max_xres; + __u32 max_yres; +}; + +#define VFIO_DEVICE_GFX_EDID_CAPS _IO(VFIO_TYPE, VFIO_BASE + 17) + +/** + * VFIO_DEVICE_GFX_EDID_SET - _IOW(VFIO_TYPE, VFIO_BASE + 18, + * struct vfio_device_gfx_edid_set) + * + * 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]; +}; + +#define VFIO_DEVICE_GFX_EDID_SET _IO(VFIO_TYPE, VFIO_BASE + 18) + /* -------- API for Type1 VFIO IOMMU -------- */ /**
Hi, I consider adding EDID support to qemu, for display configuration. qemu patches are here: https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid linux kernel patches are here: https://git.kraxel.org/cgit/linux/log/?h=edid Current (experimental) patches have support for the stdvga and virtio-vga. I think it would be quite useful for vgpu too. Unlike emulated devices vgpu's do not have a paravirtual display configuration path, because the standard way to configure the display is to simply read the edid from the monitor. Intel has two hard-coded edid blobs for that (depending on vgpu type). Not sure how nvidia handles this, but probably simliar. With qemu passing a edid blob for the virtual display instead of using the hardcoded blob we could make the display configuration much more flexible. Ideally qemu would also be able to update the edid blob at any time, and the vgpu will notify the guest about it (probably by emulating a monitor hotplug event). The guest can react on qemu window resizing then and adapt automatically, simliar to how it works with qxl and virtio-gpu. The guest and the vgpu should be able to handle "odd" non-standard display resolutions like this (coming from random window resizing): Detailed mode: Clock 106.620 MHz, 477 mm x 330 mm 1212 1515 1551 1636 hborder 0 840 844 848 869 vborder 0 -hsync -vsync VertFreq: 74 Hz, HorFreq: 65171 Hz RfC patch for the vfio interface is below. Comments? cheers, Gerd