Hi
On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> On Tue, Oct 22, 2024 at 12:04:29PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <berra...@redhat.com
> >
> > wrote:
> >
> > > On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
> > > > Hi Roque
> > > >
> > > > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> > > > <roq...@google.com> wrote:
> > > > >
> > > > > From: Andrew Keesler <ankees...@google.com>
> > > > >
> > > > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
> > > Identification
> > > > > Data) is propagated by QEMU such that a virtual display presents
> > > legitimate
> > > > > metadata (e.g., name, serial number, preferred resolutions, etc.)
> to
> > > its
> > > > > connected guest.
> > > > >
> > > > > This change propagates an optional user-provided display name to
> > > > > QemuConsole. Future changes will update downstream devices to
> leverage
> > > this
> > > > > display name for various uses, the primary one being providing a
> > > custom EDID
> > > > > name to guests. Future changes will also update other displays
> (e.g.,
> > > spice)
> > > > > with a similar option to propagate a display name to downstream
> > > devices.
> > > > >
> > > > > Currently, every virtio-gpu virtual display has the same name:
> "QEMU
> > > > > Monitor". We hope to be able to inject the EDID name of virtual
> > > displays in
> > > > > order to test guest behavior that is specific to display names. We
> > > provide the
> > > > > ability to inject the display name from the display configuration
> as
> > > that most
> > > > > closely resembles how real displays work (hardware displays contain
> > > static EDID
> > > > > information that is provided to every connected host).
> > > > >
> > > > > It should also be noted that EDID names longer than 12 bytes will
> be
> > > truncated
> > > > > per spec (I think?).
> > > > >
> > > > > Signed-off-by: Andrew Keesler <ankees...@google.com>
> > > > > Signed-off-by: Roque Arcudia Hernandez <roq...@google.com>
> > > > > ---
> > > > > include/ui/console.h | 1 +
> > > > > ui/console-priv.h | 1 +
> > > > > ui/console.c | 8 ++++++++
> > > > > ui/vnc.c | 8 +++++++-
> > > > > 4 files changed, 17 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/ui/console.h b/include/ui/console.h
> > > > > index 5832d52a8a..74ab03ed72 100644
> > > > > --- a/include/ui/console.h
> > > > > +++ b/include/ui/console.h
> > > > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
> > > > > uint32_t qemu_console_get_head(QemuConsole *con);
> > > > > int qemu_console_get_width(QemuConsole *con, int fallback);
> > > > > int qemu_console_get_height(QemuConsole *con, int fallback);
> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name);
> > > > > /* Return the low-level window id for the console */
> > > > > int qemu_console_get_window_id(QemuConsole *con);
> > > > > /* Set the low-level window id for the console */
> > > > > diff --git a/ui/console-priv.h b/ui/console-priv.h
> > > > > index 43ceb8122f..9f2769843f 100644
> > > > > --- a/ui/console-priv.h
> > > > > +++ b/ui/console-priv.h
> > > > > @@ -18,6 +18,7 @@ struct QemuConsole {
> > > > > Object parent;
> > > > >
> > > > > int index;
> > > > > + const char *name;
> > > > > DisplayState *ds;
> > > > > DisplaySurface *surface;
> > > > > DisplayScanout scanout;
> > > > > diff --git a/ui/console.c b/ui/console.c
> > > > > index 5165f17125..f377fd8417 100644
> > > > > --- a/ui/console.c
> > > > > +++ b/ui/console.c
> > > > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole
> *con,
> > > int fallback)
> > > > > }
> > > > > }
> > > > >
> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name)
> > > > > +{
> > > > > + if (con == NULL) {
> > > > > + return;
> > > > > + }
> > > > > + con->name = name;
> > > > > +}
> > > > > +
> > > > > int qemu_invalidate_text_consoles(void)
> > > > > {
> > > > > QemuConsole *s;
> > > > > diff --git a/ui/vnc.c b/ui/vnc.c
> > > > > index 93a8dbd253..7d6acc5c2e 100644
> > > > > --- a/ui/vnc.c
> > > > > +++ b/ui/vnc.c
> > > > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
> > > > > },{
> > > > > .name = "power-control",
> > > > > .type = QEMU_OPT_BOOL,
> > > > > + },{
> > > > > + .name = "name",
> > > > > + .type = QEMU_OPT_STRING,
> > > > > },
> > > > > { /* end of list */ }
> > > > > },
> > > > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
> > > **errp)
> > > > > QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> > > > > g_autoptr(SocketAddressList) saddr_list = NULL;
> > > > > g_autoptr(SocketAddressList) wsaddr_list = NULL;
> > > > > - const char *share, *device_id;
> > > > > + const char *share, *device_id, *name;
> > > > > QemuConsole *con;
> > > > > bool password = false;
> > > > > bool reverse = false;
> > > > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
> > > **errp)
> > > > > }
> > > > > qkbd_state_set_delay(vd->kbd, key_delay_ms);
> > > > >
> > > > > + name = qemu_opt_get(opts, "name");
> > > > > + qemu_console_set_name(vd->dcl.con, name);
> > > >
> > > > Why not expose a "head_name" property in QemuGraphicConsole?
> > >
> > > QemuGraphicConsole isn't mapped to any CLI though, is it ?
> > >
> > >
> > No, it would be a bit tedious to do so with multi-head -devices.
> >
> >
> > > In QAPI we have DisplayOptions union for all the local displays,
> > > and as a user I think I'd expect 'name' to be settable from
> > > those.
> > >
> > >
> > DisplayOptions is meant for the UI display.. Here, the intent is really
> to
> > set the HW EDID name field.
>
> But it is also applicable to the backend, all of which have a
> name for the display set in the window titlebar. We should
> be looking at both sides IMHO.
>
Ok, if we consider both should be treated similarly / reflect each other.
>
> > Also DisplayOptions doesn't map to a specific console.
>
> It could be made to contain per-head information if we desired
> though, and would be more useful than just the name. There were
> some patches a while ago trying to express per-console placement
> of windows onto host monitor outputs, for example.
>
[RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors
https://patchew.org/QEMU/20240531185804.119557-1-dongwon....@intel.com/
>
> > > own CLI options we can expose.
> > >
> > > For runtime setting, we have a QMP "display-update" command, that
> > > currently just lets you change VNC listening address, but was intended
> > > to allow for any runtime display changes.
> > >
> > > > This way it should be possible to set the name with QMP qom-set.
> > >
> > > qom-set isn't a particularly nice interface, as things you can set
> > > from that are not introspectable and have no type information that
> > > can be queried.
> > >
> >
> > fwiw, it could be easily exposed to D-Bus, for ex:
> >
> > busctl --user set-property org.qemu /org/qemu/Display1/Console_1
> > org.qemu.Display1.Console HeadName s "First Monitor"
>
> That could be mapped to whatever interface we expose on the QEMU side,
> it doesn't have to be qom-set.
>
It seems to me the main problem is that consoles are dynamically created by
devices, and it's hard for the ui/display to map options to a specific
console.
The other issue is handling arrays with CLI in general...
--
Marc-André Lureau
