Re: [Qemu-devel] [PATCH v2 2/3] vfio/display: add xres + yres properties

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 20 Feb 2019 09:47:52 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> This allows configure the display resolution which the vgpu should use.
> The information will be passed to the guest using EDID, so the mdev
> driver must support the vfio edid region for this to work.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/vfio/pci.h     |  2 ++
>  hw/vfio/display.c | 16 ++++++++++++++--
>  hw/vfio/pci.c     |  2 ++
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index b1ae4c0754..c11c3f1670 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -149,6 +149,8 @@ typedef struct VFIOPCIDevice {
>  #define VFIO_FEATURE_ENABLE_IGD_OPREGION \
>                                  (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT)
>      OnOffAuto display;
> +    uint32_t display_xres;
> +    uint32_t display_yres;
>      int32_t bootindex;
>      uint32_t igd_gms;
>      OffAutoPCIBAR msix_relo;
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index ed2eb19ea3..7b9b604a64 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -46,8 +46,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
>      qemu_edid_info edid = {
>          .maxx  = dpy->edid_regs->max_xres,
>          .maxy  = dpy->edid_regs->max_yres,
> -        .prefx = prefx,
> -        .prefy = prefy,
> +        .prefx = prefx ?: vdev->display_xres,
> +        .prefy = prefy ?: vdev->display_yres,
>      };
>  
>      dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
> @@ -117,6 +117,10 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>                                     VFIO_REGION_SUBTYPE_GFX_EDID,
>                                     &dpy->edid_info);
>      if (ret) {
> +        if (vdev->display_xres || vdev->display_yres) {
> +            warn_report("vfio: no edid support available, "
> +                        "xres and yres properties have no effect.");
> +        }

In order to get here the device needs to have a display option set to
'on' or 'auto' and that display needs to be backed by a dmabuf graphics
plane.  That means that QEMU is happy to run without any warning if a
user sets a resolution on a region backed display, or a device without
a display.  I think that QEMU should probably fail, not just warn, for
all cases where an option is not appropriate for a device.  Perhaps
EDID setup should set a feature bit or flag that we can test similar to
how and where we test for a stray ramfb option.

>          return;
>      }
>  
> @@ -128,6 +132,14 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>      pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
>      dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
>  
> +    /* if xres + yres properties are unset use the maximum resolution */
> +    if (!vdev->display_xres) {
> +        vdev->display_xres = dpy->edid_regs->max_xres;
> +    }
> +    if (!vdev->display_yres) {
> +        vdev->display_yres = dpy->edid_regs->max_yres;
> +    }
> +
>      vfio_display_edid_update(vdev, true, 0, 0);
>      return;
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index dd12f36391..edb8394038 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3182,6 +3182,8 @@ static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
>      DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
>                              display, ON_OFF_AUTO_OFF),
> +    DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
> +    DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),

This is actually quite fun, I started my VM with arbitrary numbers and
the Windows GUI honored it every time.  Probably very useful for
playing with odd screen sizes.  I also tried to break it using
1000000x1000000, but the display came up as 1920x1200, the maximum
resolution GVT-g supports for this type.  I don't see that QEMU is
bounding this though, do we depend on the mdev device to ignore it if
we pass values it cannot support?  Thanks,

Alex

>      DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
>                         intx.mmap_timeout, 1100),
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,