Re: [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jul 22, 2025 at 07:08:24PM +0000, Andrew Keesler wrote:
> In 454f4b0f, we started down the path of supporting separate
> configurations per display head (e.g., you have 2 heads - one with
> EDID name "AAA" and the other with EDID name "BBB").
> 
> In this change, we add resolution to this configuration surface (e.g.,
> you have 2 heads - one with resolution 111x222 and the other with
> resolution 333x444).
> 
>   -display vnc=localhost:0,id=aaa,display=vga,head=0 \
>   -display vnc=localhost:1,id=bbb,display=vga,head=1 \
>   -device '{"driver":"virtio-vga",
>             "max_outputs":2,
>             "id":"vga",
>             "outputs":[
>               {
>                  "name":"AAA",
>                  "xres":111,
>                  "yres":222
>               },
>               {
>                  "name":"BBB",
>                  "xres":333,
>                  "yres":444
>               }
>             ]}'
> 
> If no virtio_gpu_base_conf.outputs are provided, then
> virtio_gpu_base_conf.xres/virtio_gpu_base_conf.yres will still be
> respected, preserving backwards compatibility.

IIUC from the current code, xres/yres are only set against the
first head. The 2nd and later heads are left undefined by the
virtio-gpu-base code at least - I'm unclear if something else
in QEMU will fill in defaults later, or if they set to 0x0.

> Otherwise, if any virtio_gpu_base_conf.outputs are provided, then
> virtio_gpu_base_conf.outputs.xres/virtio_gpu_base_conf.outputs.yres
> will take precedence. In this case,
> virtio_gpu_base_conf.outputs.xres/virtio_gpu_base_conf.outputs.yres
> must be non-zero.

Makes sense.

> Signed-off-by: Andrew Keesler <ankeesler@google.com>
> ---
>  hw/display/virtio-gpu-base.c | 12 ++++++++++++
>  qapi/virtio.json             |  6 +++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index 7269477a1c..1fc879cc93 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -206,6 +206,10 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>                         node->value->name, EDID_NAME_MAX_LENGTH);
>              return false;
>          }
> +        if (node->value && !(node->value->xres && node->value->yres)) {
> +            error_setg(errp, "invalid resolution == 0");
> +            return false;
> +        }
>      }
>  
>      if (virtio_gpu_virgl_enabled(g->conf)) {
> @@ -233,6 +237,14 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>      g->req_state[0].width = g->conf.xres;
>      g->req_state[0].height = g->conf.yres;
>  
> +    for (output_idx = 0, node = g->conf.outputs;
> +         node && output_idx < g->conf.max_outputs;
> +         output_idx++, node = node->next) {
> +        g->enabled_output_bitmask |= (1 << output_idx);

This change means that all heads are enabled by default if the 'outputs'
property array is set, which is a semantic difference from the previous
behaviour before xres/yres are added as properties.

Is it relevant to set xres/yres on outputs, even when they are
not (yet) enabled ?  Perhaps we should have an explicit 'enabled: bool'
property in the 'outputs' struct ?

> +        g->req_state[output_idx].width = node->value->xres;
> +        g->req_state[output_idx].height = node->value->yres;
> +    }

For head 0, this is overwriting the defaults set a few lines
earlier.

I think we should probably report an error if xres / yres
are set at the global level and also set against any individual
output, so the two approaches are mutually exclusive.

>      g->hw_ops = &virtio_gpu_ops;
>      for (i = 0; i < g->conf.max_outputs; i++) {
>          g->scanout[i].con =
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 9d652fe4a8..36581690c7 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -970,11 +970,15 @@
>  #
>  # @name: the name of the output
>  #
> +# @xres: horizontal resolution of the output in pixels
> +#
> +# @yres: vertical resolution of the output in pixels
> +#

Sorry, we missed the boat for 10.1, so these two new properties
will require an explicit "Since 10.2" tag against them, separate
from the overall struct versrion below.

>  # Since: 10.1
>  ##
>  
>  { 'struct': 'VirtIOGPUOutput',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str', 'xres': 'uint16', 'yres': 'uint16' } }

This is a backcompat problem, because xres/yres are mandatory if
'outputs' is present.

$ ./qemu-system-x86_64 -device '{"driver":"virtio-vga","outputs":[{"name":"AAA"},{"name":"BBB"}],"max_outputs":2,"id":"v0"}'
qemu-system-x86_64: -device {"driver":"virtio-vga","outputs":[{"name":"AAA"},{"name":"BBB"}],"max_outputs":2,"id":"v0"}: Parameter 'outputs[0].xres' is missing

These need to be marked optional, to be compatible with existing use
of 'outputs' from the 10.1.0 release.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|