diff mbox

[v2,4/4,RfC] stdvga: add xres and yres properties.

Message ID 1489048710-22084-5-git-send-email-kraxel@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gerd Hoffmann March 9, 2017, 8:38 a.m. UTC 
Add two new registers to the qemu extended register range, carrying the
suggested width and height of the display device.  In case the xres and
yres properties are set, fill these new registers accordingly.

Additionally guest driver updates are needed to support the new
registers.  These changes will probably land in linux kernel 4.12.

TODO: add compat properties to turn off qemu-display-size for machine
      types older than 2.10

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga-pci.c    | 52 ++++++++++++++++++++++++++++++++++---------------
 hw/display/vga_int.h    |  6 ++++--
 hw/display/virtio-vga.c |  3 ++-
 3 files changed, 42 insertions(+), 19 deletions(-)

Comments

Hervé Poussineau March 9, 2017, 9:03 p.m. UTC | #1 
Le 09/03/2017 à 09:38, Gerd Hoffmann a écrit :
> Add two new registers to the qemu extended register range, carrying the
> suggested width and height of the display device.  In case the xres and
> yres properties are set, fill these new registers accordingly.
>
> Additionally guest driver updates are needed to support the new
> registers.  These changes will probably land in linux kernel 4.12.

Is it possible to provide a mecanism to tell guest that the preferred default size changed?
For example, the user can resize the VM window on host, and guest will auto-adapt to new size/resolution.
It is already working like this on VirtualBox with VM Additions.

Regards,

Hervé

>
> TODO: add compat properties to turn off qemu-display-size for machine
>       types older than 2.10
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga-pci.c    | 52 ++++++++++++++++++++++++++++++++++---------------
>  hw/display/vga_int.h    |  6 ++++--
>  hw/display/virtio-vga.c |  3 ++-
>  3 files changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
> index 8500362..10b1832 100644
> --- a/hw/display/vga-pci.c
> +++ b/hw/display/vga-pci.c
> @@ -31,23 +31,28 @@
>  #include "ui/pixel_ops.h"
>  #include "qemu/timer.h"
>  #include "hw/loader.h"
> +#include "sysemu/sysemu.h"
>
>  #define PCI_VGA_IOPORT_OFFSET 0x400
>  #define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
>  #define PCI_VGA_BOCHS_OFFSET  0x500
>  #define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
>  #define PCI_VGA_QEXT_OFFSET   0x600
> -#define PCI_VGA_QEXT_SIZE     (2 * 4)
> +#define PCI_VGA_QEXT_SIZE_V1  (2 * 4)
> +#define PCI_VGA_QEXT_SIZE_V2  (4 * 4)
>  #define PCI_VGA_MMIO_SIZE     0x1000
>
>  #define PCI_VGA_QEXT_REG_SIZE         (0 * 4)
>  #define PCI_VGA_QEXT_REG_BYTEORDER    (1 * 4)
>  #define  PCI_VGA_QEXT_LITTLE_ENDIAN   0x1e1e1e1e
>  #define  PCI_VGA_QEXT_BIG_ENDIAN      0xbebebebe
> +#define PCI_VGA_QEXT_REG_WIDTH        (2 * 4)
> +#define PCI_VGA_QEXT_REG_HEIGHT       (3 * 4)
>
>  enum vga_pci_flags {
> -    PCI_VGA_FLAG_ENABLE_MMIO = 1,
> -    PCI_VGA_FLAG_ENABLE_QEXT = 2,
> +    PCI_VGA_FLAG_ENABLE_MMIO    = 1,
> +    PCI_VGA_FLAG_ENABLE_QEXT    = 2,
> +    PCI_VGA_FLAG_ENABLE_QEXT_V2 = 3,
>  };
>
>  typedef struct PCIVGAState {
> @@ -157,10 +162,14 @@ static uint64_t pci_vga_qext_read(void *ptr, hwaddr addr, unsigned size)
>
>      switch (addr) {
>      case PCI_VGA_QEXT_REG_SIZE:
> -        return PCI_VGA_QEXT_SIZE;
> +        return s->qext_size;
>      case PCI_VGA_QEXT_REG_BYTEORDER:
>          return s->big_endian_fb ?
>              PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
> +    case PCI_VGA_QEXT_REG_WIDTH:
> +        return s->xres;
> +    case PCI_VGA_QEXT_REG_HEIGHT:
> +        return s->yres;
>      default:
>          return 0;
>      }
> @@ -207,8 +216,7 @@ static const MemoryRegionOps pci_vga_qext_ops = {
>
>  void pci_std_vga_mmio_region_init(VGACommonState *s,
>                                    MemoryRegion *parent,
> -                                  MemoryRegion *subs,
> -                                  bool qext)
> +                                  MemoryRegion *subs)
>  {
>      memory_region_init_io(&subs[0], NULL, &pci_vga_ioport_ops, s,
>                            "vga ioports remapped", PCI_VGA_IOPORT_SIZE);
> @@ -220,9 +228,9 @@ void pci_std_vga_mmio_region_init(VGACommonState *s,
>      memory_region_add_subregion(parent, PCI_VGA_BOCHS_OFFSET,
>                                  &subs[1]);
>
> -    if (qext) {
> +    if (s->qext_size) {
>          memory_region_init_io(&subs[2], NULL, &pci_vga_qext_ops, s,
> -                              "qemu extended regs", PCI_VGA_QEXT_SIZE);
> +                              "qemu extended regs", s->qext_size);
>          memory_region_add_subregion(parent, PCI_VGA_QEXT_OFFSET,
>                                      &subs[2]);
>      }
> @@ -232,7 +240,6 @@ static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
>  {
>      PCIVGAState *d = PCI_VGA(dev);
>      VGACommonState *s = &d->vga;
> -    bool qext = false;
>
>      /* vga + console init */
>      vga_common_init(s, OBJECT(dev), true);
> @@ -249,10 +256,15 @@ static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
>          memory_region_init(&d->mmio, NULL, "vga.mmio", 4096);
>
>          if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
> -            qext = true;
> -            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +            if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT_V2)) {
> +                s->qext_size = PCI_VGA_QEXT_SIZE_V2;
> +                pci_set_byte(&d->dev.config[PCI_REVISION_ID], 3);
> +            } else {
> +                s->qext_size = PCI_VGA_QEXT_SIZE_V1;
> +                pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +            }
>          }
> -        pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs, qext);
> +        pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs);
>
>          pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>      }
> @@ -274,7 +286,6 @@ static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
>  {
>      PCIVGAState *d = PCI_VGA(dev);
>      VGACommonState *s = &d->vga;
> -    bool qext = false;
>
>      /* vga + console init */
>      vga_common_init(s, OBJECT(dev), false);
> @@ -284,10 +295,15 @@ static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
>      memory_region_init(&d->mmio, OBJECT(dev), "vga.mmio", 4096);
>
>      if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
> -        qext = true;
> -        pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +        if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT_V2)) {
> +            s->qext_size = PCI_VGA_QEXT_SIZE_V2;
> +            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 3);
> +        } else {
> +            s->qext_size = PCI_VGA_QEXT_SIZE_V1;
> +            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +        }
>      }
> -    pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs, qext);
> +    pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs);
>
>      pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
>      pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> @@ -315,6 +331,10 @@ static Property vga_pci_common_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
>      DEFINE_PROP_BIT("qemu-extended-regs",
>                      PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
> +    DEFINE_PROP_BIT("qemu-display-size",
> +                    PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT_V2, true),
> +    DEFINE_PROP_UINT32("xres", PCIVGAState, vga.xres, 0),
> +    DEFINE_PROP_UINT32("yres", PCIVGAState, vga.yres, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index dd6c958..1e9b0ff 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -162,6 +162,9 @@ typedef struct VGACommonState {
>      bool full_update_gfx;
>      bool big_endian_fb;
>      bool default_endian_fb;
> +    uint32_t qext_size;
> +    uint32_t xres;
> +    uint32_t yres;
>      /* hardware mouse cursor support */
>      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
>      uint32_t hw_cursor_x;
> @@ -223,7 +226,6 @@ extern const MemoryRegionOps vga_mem_ops;
>  /* vga-pci.c */
>  void pci_std_vga_mmio_region_init(VGACommonState *s,
>                                    MemoryRegion *parent,
> -                                  MemoryRegion *subs,
> -                                  bool qext);
> +                                  MemoryRegion *subs);
>
>  #endif
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index f9b017d..8684dc0 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -153,8 +153,9 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>      }
>
>      /* add stdvga mmio regions */
> +    vga->qext_size = 2 * 4; /* PCI_VGA_QEXT_SIZE_V1 */
>      pci_std_vga_mmio_region_init(vga, &vpci_dev->modern_bar,
> -                                 vvga->vga_mrs, true);
> +                                 vvga->vga_mrs);
>
>      vga->con = g->scanout[0].con;
>      graphic_console_set_hwops(vga->con, &virtio_vga_ops, vvga);
>
Gerd Hoffmann March 13, 2017, 3:17 p.m. UTC | #2 
On Do, 2017-03-09 at 22:03 +0100, Hervé Poussineau wrote:
> Le 09/03/2017 à 09:38, Gerd Hoffmann a écrit :
> > Add two new registers to the qemu extended register range, carrying the
> > suggested width and height of the display device.  In case the xres and
> > yres properties are set, fill these new registers accordingly.
> >
> > Additionally guest driver updates are needed to support the new
> > registers.  These changes will probably land in linux kernel 4.12.
> 
> Is it possible to provide a mecanism to tell guest that the preferred
> default size changed?
> For example, the user can resize the VM window on host, and guest will
> auto-adapt to new size/resolution.
> It is already working like this on VirtualBox with VM Additions.

Well, sure, we can pimp the virtual hardware even more.  Adding such a
notification is pretty invasive though.  We'll have to add irq support
to stdvga, on both guest and host.

So the question is:  Is it worth the effort?  How far we want take it
and why?

For linux this would require a pretty recent guest kernel with the (to
be written) driver changes added.  If you have a recent guest kernel
anyway you can just switch to virtio-gpu where this works today (gtk ui,
or spice with guest agent active).  So I see little reason to put too
much effort into stdvga, for linux guests.

For non-linux guests (where no virtio-gpu driver is available) better
stdvga support might be more useful.  Do you have specific guests in
mind?  Looking at your email address I suspect that is the case ...

While being at it:  We might also consider adding hardware cursor
support.  Seems that is standard these days.  Wayland has no software
cursor support any more, therefore you have no mouse pointer when
running wayland on stdvga (again: on virtio-gpu it'll work just fine).

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index 8500362..10b1832 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -31,23 +31,28 @@ 
 #include "ui/pixel_ops.h"
 #include "qemu/timer.h"
 #include "hw/loader.h"
+#include "sysemu/sysemu.h"
 
 #define PCI_VGA_IOPORT_OFFSET 0x400
 #define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
 #define PCI_VGA_BOCHS_OFFSET  0x500
 #define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
 #define PCI_VGA_QEXT_OFFSET   0x600
-#define PCI_VGA_QEXT_SIZE     (2 * 4)
+#define PCI_VGA_QEXT_SIZE_V1  (2 * 4)
+#define PCI_VGA_QEXT_SIZE_V2  (4 * 4)
 #define PCI_VGA_MMIO_SIZE     0x1000
 
 #define PCI_VGA_QEXT_REG_SIZE         (0 * 4)
 #define PCI_VGA_QEXT_REG_BYTEORDER    (1 * 4)
 #define  PCI_VGA_QEXT_LITTLE_ENDIAN   0x1e1e1e1e
 #define  PCI_VGA_QEXT_BIG_ENDIAN      0xbebebebe
+#define PCI_VGA_QEXT_REG_WIDTH        (2 * 4)
+#define PCI_VGA_QEXT_REG_HEIGHT       (3 * 4)
 
 enum vga_pci_flags {
-    PCI_VGA_FLAG_ENABLE_MMIO = 1,
-    PCI_VGA_FLAG_ENABLE_QEXT = 2,
+    PCI_VGA_FLAG_ENABLE_MMIO    = 1,
+    PCI_VGA_FLAG_ENABLE_QEXT    = 2,
+    PCI_VGA_FLAG_ENABLE_QEXT_V2 = 3,
 };
 
 typedef struct PCIVGAState {
@@ -157,10 +162,14 @@  static uint64_t pci_vga_qext_read(void *ptr, hwaddr addr, unsigned size)
 
     switch (addr) {
     case PCI_VGA_QEXT_REG_SIZE:
-        return PCI_VGA_QEXT_SIZE;
+        return s->qext_size;
     case PCI_VGA_QEXT_REG_BYTEORDER:
         return s->big_endian_fb ?
             PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
+    case PCI_VGA_QEXT_REG_WIDTH:
+        return s->xres;
+    case PCI_VGA_QEXT_REG_HEIGHT:
+        return s->yres;
     default:
         return 0;
     }
@@ -207,8 +216,7 @@  static const MemoryRegionOps pci_vga_qext_ops = {
 
 void pci_std_vga_mmio_region_init(VGACommonState *s,
                                   MemoryRegion *parent,
-                                  MemoryRegion *subs,
-                                  bool qext)
+                                  MemoryRegion *subs)
 {
     memory_region_init_io(&subs[0], NULL, &pci_vga_ioport_ops, s,
                           "vga ioports remapped", PCI_VGA_IOPORT_SIZE);
@@ -220,9 +228,9 @@  void pci_std_vga_mmio_region_init(VGACommonState *s,
     memory_region_add_subregion(parent, PCI_VGA_BOCHS_OFFSET,
                                 &subs[1]);
 
-    if (qext) {
+    if (s->qext_size) {
         memory_region_init_io(&subs[2], NULL, &pci_vga_qext_ops, s,
-                              "qemu extended regs", PCI_VGA_QEXT_SIZE);
+                              "qemu extended regs", s->qext_size);
         memory_region_add_subregion(parent, PCI_VGA_QEXT_OFFSET,
                                     &subs[2]);
     }
@@ -232,7 +240,6 @@  static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
 {
     PCIVGAState *d = PCI_VGA(dev);
     VGACommonState *s = &d->vga;
-    bool qext = false;
 
     /* vga + console init */
     vga_common_init(s, OBJECT(dev), true);
@@ -249,10 +256,15 @@  static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
         memory_region_init(&d->mmio, NULL, "vga.mmio", 4096);
 
         if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
-            qext = true;
-            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
+            if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT_V2)) {
+                s->qext_size = PCI_VGA_QEXT_SIZE_V2;
+                pci_set_byte(&d->dev.config[PCI_REVISION_ID], 3);
+            } else {
+                s->qext_size = PCI_VGA_QEXT_SIZE_V1;
+                pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
+            }
         }
-        pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs, qext);
+        pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs);
 
         pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
     }
@@ -274,7 +286,6 @@  static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
 {
     PCIVGAState *d = PCI_VGA(dev);
     VGACommonState *s = &d->vga;
-    bool qext = false;
 
     /* vga + console init */
     vga_common_init(s, OBJECT(dev), false);
@@ -284,10 +295,15 @@  static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
     memory_region_init(&d->mmio, OBJECT(dev), "vga.mmio", 4096);
 
     if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
-        qext = true;
-        pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
+        if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT_V2)) {
+            s->qext_size = PCI_VGA_QEXT_SIZE_V2;
+            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 3);
+        } else {
+            s->qext_size = PCI_VGA_QEXT_SIZE_V1;
+            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
+        }
     }
-    pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs, qext);
+    pci_std_vga_mmio_region_init(s, &d->mmio, d->mrs);
 
     pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
     pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
@@ -315,6 +331,10 @@  static Property vga_pci_common_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
     DEFINE_PROP_BIT("qemu-extended-regs",
                     PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
+    DEFINE_PROP_BIT("qemu-display-size",
+                    PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT_V2, true),
+    DEFINE_PROP_UINT32("xres", PCIVGAState, vga.xres, 0),
+    DEFINE_PROP_UINT32("yres", PCIVGAState, vga.yres, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index dd6c958..1e9b0ff 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -162,6 +162,9 @@  typedef struct VGACommonState {
     bool full_update_gfx;
     bool big_endian_fb;
     bool default_endian_fb;
+    uint32_t qext_size;
+    uint32_t xres;
+    uint32_t yres;
     /* hardware mouse cursor support */
     uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
     uint32_t hw_cursor_x;
@@ -223,7 +226,6 @@  extern const MemoryRegionOps vga_mem_ops;
 /* vga-pci.c */
 void pci_std_vga_mmio_region_init(VGACommonState *s,
                                   MemoryRegion *parent,
-                                  MemoryRegion *subs,
-                                  bool qext);
+                                  MemoryRegion *subs);
 
 #endif
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index f9b017d..8684dc0 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -153,8 +153,9 @@  static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     }
 
     /* add stdvga mmio regions */
+    vga->qext_size = 2 * 4; /* PCI_VGA_QEXT_SIZE_V1 */
     pci_std_vga_mmio_region_init(vga, &vpci_dev->modern_bar,
-                                 vvga->vga_mrs, true);
+                                 vvga->vga_mrs);
 
     vga->con = g->scanout[0].con;
     graphic_console_set_hwops(vga->con, &virtio_vga_ops, vvga);