Hi - gentle ping on my last email. Would love to hear any thoughts about moving this forward.
Thank you for your thoughts thus far. On Mon, Oct 28, 2024 at 3:25 PM Andrew Keesler <[email protected]> wrote: > Hi Daniel and Marc-André - please excuse my delay (I was traveling > last week). > > I see 2 primary takeaways here: > > 1. Updating the name field from the ServerInit RFB message to be > controlled by the 'name' VNC option > > 2. Updating where we store the display 'name' in memory > > For 1 - are we amenable to me updating this display name field to be > controlled by 'name' in a future patch? I'm still learning what the > QEMU community prefers with respect to patch sizes, but in general I > have been trying to keep the patches small. > > For 2 - have we landed on storing the per-display 'name' in > DisplayOptions? I can't quite tell if we've converged here. It also > seems like there is some more plumbing to install here before we > leverage DisplayOptions, so I am curious what the recommendation is > for this patch - is there a temporary solution we could pursue and do > refactors later to unlock the CLI functionality that has been brought > up? > > Correct me if I am wrong about any of this. > > On Tue, Oct 22, 2024 at 4:36 AM Marc-André Lureau < > [email protected]> wrote: > >> Hi >> >> On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <[email protected]> >> 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é < >>> [email protected]> >>> > 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 >>> > > > <[email protected]> wrote: >>> > > > > >>> > > > > From: Andrew Keesler <[email protected]> >>> > > > > >>> > > > > 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 <[email protected]> >>> > > > > Signed-off-by: Roque Arcudia Hernandez <[email protected]> >>> > > > > --- >>> > > > > 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/[email protected]/ >> >>> >>> > > 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 >> >
