Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Markus Armbruster <armbru@redhat.com>]

"Kasireddy, Vivek" <vivek.kasireddy@intel.com> writes:

> Hi Markus,
>
> Thank you for the review.
>
>> Vivek Kasireddy <vivek.kasireddy@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.kim@intel.com>
>> > Cc: Gerd Hoffmann <kraxel@redhat.com>
>> > Cc: Daniel P. Berrangé <berrange@redhat.com>
>> > Cc: Markus Armbruster <armbru@redhat.com>
>> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Cc: Thomas Huth <thuth@redhat.com>
>> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@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 <armbru@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!