qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] Support per-head resolutions with virtio-gpu
@ 2025-07-22 19:08 Andrew Keesler
  2025-07-22 19:08 ` [PATCH 1/1] hw/display: " Andrew Keesler
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Keesler @ 2025-07-22 19:08 UTC (permalink / raw)
  To: berrange, marcandre.lureau; +Cc: qemu-devel, Andrew Keesler

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.

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.

Andrew Keesler (1):
  hw/display: Support per-head resolutions with virtio-gpu

 hw/display/virtio-gpu-base.c | 12 ++++++++++++
 qapi/virtio.json             |  6 +++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.50.0.727.gbf7dc18ff4-goog



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu
  2025-07-22 19:08 [PATCH 0/1] Support per-head resolutions with virtio-gpu Andrew Keesler
@ 2025-07-22 19:08 ` Andrew Keesler
  2025-08-27 13:46   ` Daniel P. Berrangé
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Keesler @ 2025-07-22 19:08 UTC (permalink / raw)
  To: berrange, marcandre.lureau; +Cc: qemu-devel, Andrew Keesler

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.

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.

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);
+        g->req_state[output_idx].width = node->value->xres;
+        g->req_state[output_idx].height = node->value->yres;
+    }
+
     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
+#
 # Since: 10.1
 ##
 
 { 'struct': 'VirtIOGPUOutput',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str', 'xres': 'uint16', 'yres': 'uint16' } }
 
 ##
 # @DummyVirtioForceArrays:
-- 
2.50.0.727.gbf7dc18ff4-goog



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu
  2025-07-22 19:08 ` [PATCH 1/1] hw/display: " Andrew Keesler
@ 2025-08-27 13:46   ` Daniel P. Berrangé
  2025-08-27 15:48     ` Andrew Keesler
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel P. Berrangé @ 2025-08-27 13:46 UTC (permalink / raw)
  To: Andrew Keesler; +Cc: marcandre.lureau, qemu-devel

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 :|



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu
  2025-08-27 13:46   ` Daniel P. Berrangé
@ 2025-08-27 15:48     ` Andrew Keesler
  2025-08-27 16:43       ` Daniel P. Berrangé
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Keesler @ 2025-08-27 15:48 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 7628 bytes --]

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é <berrange@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 <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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 10125 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu
  2025-08-27 15:48     ` Andrew Keesler
@ 2025-08-27 16:43       ` Daniel P. Berrangé
  2025-08-28 15:47         ` Andrew Keesler
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel P. Berrangé @ 2025-08-27 16:43 UTC (permalink / raw)
  To: Andrew Keesler; +Cc: marcandre.lureau, qemu-devel

On Wed, Aug 27, 2025 at 11:48:34AM -0400, Andrew Keesler wrote:
> 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?

No, I don't have an example.

> 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.

Ok, lets just document that setting xres/yres will cause the
output to be automatically enabled. We can always retrofit
an 'enabled' property at a later date if someone arrives with
a use case for setting res without enabling the output.

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 :|



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu
  2025-08-27 16:43       ` Daniel P. Berrangé
@ 2025-08-28 15:47         ` Andrew Keesler
  2025-08-28 15:50           ` Daniel P. Berrangé
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Keesler @ 2025-08-28 15:47 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

> > 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.

Doh, I ran into an error. Seems like if I don't set xres/yres, then it
gets defaulted to 1280x800.


https://gitlab.com/qemu-project/qemu/-/blob/master/include/hw/virtio/virtio-gpu.h#L175-176

So...could we just get away with documenting that if you set xres/yres
and outputs[0].xres/yres, then the later is going to overwrite the
former? I'm not sure how to detect if xres/yres has been set globally
to 1280x800, or if we are just getting the defaults.

What do you think?

On Wed, Aug 27, 2025 at 12:44 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Aug 27, 2025 at 11:48:34AM -0400, Andrew Keesler wrote:
> > 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?
>
> No, I don't have an example.
>
> > 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.
>
> Ok, lets just document that setting xres/yres will cause the
> output to be automatically enabled. We can always retrofit
> an 'enabled' property at a later date if someone arrives with
> a use case for setting res without enabling the output.
>
> 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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 4242 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] hw/display: Support per-head resolutions with virtio-gpu
  2025-08-28 15:47         ` Andrew Keesler
@ 2025-08-28 15:50           ` Daniel P. Berrangé
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2025-08-28 15:50 UTC (permalink / raw)
  To: Andrew Keesler; +Cc: marcandre.lureau, qemu-devel

On Thu, Aug 28, 2025 at 11:47:23AM -0400, Andrew Keesler wrote:
> > > 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.
> 
> Doh, I ran into an error. Seems like if I don't set xres/yres, then it
> gets defaulted to 1280x800.
> 
> 
> https://gitlab.com/qemu-project/qemu/-/blob/master/include/hw/virtio/virtio-gpu.h#L175-176
> 
> So...could we just get away with documenting that if you set xres/yres
> and outputs[0].xres/yres, then the later is going to overwrite the
> former? I'm not sure how to detect if xres/yres has been set globally
> to 1280x800, or if we are just getting the defaults.
> 
> What do you think?

Oh it probably isn't worth the hassle for a mere error report. Instead
just document that per-output takes priority if both are set.


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 :|



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-08-28 17:25 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-22 19:08 [PATCH 0/1] Support per-head resolutions with virtio-gpu Andrew Keesler
2025-07-22 19:08 ` [PATCH 1/1] hw/display: " Andrew Keesler
2025-08-27 13:46   ` Daniel P. Berrangé
2025-08-27 15:48     ` Andrew Keesler
2025-08-27 16:43       ` Daniel P. Berrangé
2025-08-28 15:47         ` Andrew Keesler
2025-08-28 15:50           ` Daniel P. Berrangé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).