"Kasireddy, Vivek" <vivek.kasire...@intel.com> writes: > Hi Markus, > > Thank you for the review. > >> Vivek Kasireddy <vivek.kasire...@intel.com> writes: >> >> > The new parameter named "connector" can be used to assign physical >> > monitors/connectors to individual GFX VCs such that when the monitor >> > is connected or hotplugged, the associated GTK window would be >> > fullscreened on it. If the monitor is disconnected or unplugged, >> > the associated GTK window would be destroyed and a relevant >> > disconnect event would be sent to the Guest. >> > >> > Usage: -device >> > virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080... >> > -display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1..... >> > >> > Cc: Dongwon Kim <dongwon....@intel.com> >> > Cc: Gerd Hoffmann <kra...@redhat.com> >> > Cc: Daniel P. Berrangé <berra...@redhat.com> >> > Cc: Markus Armbruster <arm...@redhat.com> >> > Cc: Philippe Mathieu-Daudé <f4...@amsat.org> >> > Cc: Marc-André Lureau <marcandre.lur...@redhat.com> >> > Cc: Thomas Huth <th...@redhat.com> >> > Signed-off-by: Vivek Kasireddy <vivek.kasire...@intel.com> >> > --- >> > qapi/ui.json | 9 ++- >> > qemu-options.hx | 1 + >> > ui/gtk.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 177 insertions(+), 1 deletion(-) >> > >> > diff --git a/qapi/ui.json b/qapi/ui.json >> > index 286c5731d1..86787a4c95 100644 >> > --- a/qapi/ui.json >> > +++ b/qapi/ui.json >> > @@ -1199,13 +1199,20 @@ >> > # interfaces (e.g. VGA and virtual console character >> > devices) >> > # by default. >> > # Since 7.1 >> > +# @connector: List of physical monitor/connector names where the GTK >> > windows >> > +# containing the respective graphics virtual consoles (VCs) >> > are >> > +# to be placed. If a mapping exists for a VC, it will be >> > +# fullscreened on that specific monitor or else it would >> > not be >> > +# displayed anywhere and would appear disconnected to the >> > guest. >> >> Let's see whether I understand this... We have VCs numbered 0, 1, ... >> VC #i is mapped to the i-th element of @connector, counting from zero. >> Correct? > > [Kasireddy, Vivek] Yes, correct. > >> Ignorant question: what's a "physical monitor/connector name"? > > [Kasireddy, Vivek] IIUC, while the HDMI/DP protocols refer to a receiver (RX) > as a "sink", the DRM Graphics subsystem in the kernel uses the term > "connector" > and the GTK library prefers the term "monitor".
Awesome. > All of these terms can be > used interchangeably but I chose the term connector for the new parameter > after debating between connector, monitor, output, etc. Okay. > The connector name (e.g. DP-1, HDMI-A-2, etc) uniquely identifies a connector > or a monitor on any given system: > https://elixir.bootlin.com/linux/v6.0-rc6/source/include/drm/drm_connector.h#L1328 > If you are on Intel hardware, you can find more info related to connectors by > doing: > cat /sys/kernel/debug/dri/0/i915_display_info Thanks! >> Would you mind breaking the lines a bit earlier, between column 70 and >> 75? > > [Kasireddy, Vivek] Np, will do that. > >> >> > +# Since 7.2 >> > # >> > # Since: 2.12 >> > ## >> > { 'struct' : 'DisplayGTK', >> > 'data' : { '*grab-on-hover' : 'bool', >> > '*zoom-to-fit' : 'bool', >> > - '*show-tabs' : 'bool' } } >> > + '*show-tabs' : 'bool', >> > + '*connector' : ['str'] } } >> > >> > ## >> > # @DisplayEGLHeadless: Elsewhere in ui.json, names of list-valued members use plural: channels, clients, keys, addresses. Let's name this one connectors for consistency. With that, QAPI schema Acked-by: Markus Armbruster <arm...@redhat.com> >> > diff --git a/qemu-options.hx b/qemu-options.hx >> > index 31c04f7eea..576b65ef9f 100644 >> > --- a/qemu-options.hx >> > +++ b/qemu-options.hx >> > @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display, >> > #if defined(CONFIG_GTK) >> > "-display >> > gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n" >> > " >> > [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n" >> > + " [,connector.<id of VC>=<connector name>]\n" >> >> Is "<id of VC>" a VC number? > > [Kasireddy, Vivek] Yes. An "id" is commonly a name. Suggest connector.<index>. >> > #endif >> > #if defined(CONFIG_VNC) >> > "-display vnc=<display>[,<optargs>]\n" A bit of documentation is missing: ``show-cursor=on|off`` : Force showing the mouse cursor ``window-close=on|off`` : Allow to quit qemu with window close button + ``connector.<index>`` : <explanation> ``curses[,charset=<encoding>]`` >> >> Remainder of my review is on style only. Style suggestions are not >> demands :) > > [Kasireddy, Vivek] No problem; most of your suggestions are sensible > and I'll include them all in v2. Thanks!