Sending again (replying all this time).
> 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.
That is correct - xres/yres are only set against the first head in the
current code. The wording in my commit message (and cover letter) was
misleading. I will fix that to say that "If no
virtio_gpu_base_conf.outputs are provided,
virtio_gpu_base_conf.xres/virtio_gpu_base_conf.yres will still be
respected _for the first head_ for backwards compatibility".
The only way I could get QEMU to fill in xres/yres for a 2nd and later
head was to trigger virtio_gpu_ops.ui_info via a VNC client.
> 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 ?
Maybe you might want to set xres/yres on an output, but not enable it
yet? I don't have any concrete examples of when you might want to do
that, maybe you have some examples?
I feel like I could see a user setting xres/yres on an output,
forgetting to set enabled on that output, and then being confused why
their head is blank. Because of this, my vote would be to default to
enabling an output when it has configuration. I am easily swayed by
your guidance, though.
> 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.
ACK - fixed for the next patch.
> 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.
ACK - fixed for the next patch.
> This is a backcompat problem, because xres/yres are mandatory if
> 'outputs' is present.
ACK - fixed for the next patch.
On Wed, Aug 27, 2025 at 9:46 AM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> 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 <ankees...@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 :|
>
>
