Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
"Kasireddy, Vivek" writes:
> Hi Markus,
>
> Thank you for the review.
>
>> Vivek Kasireddy 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
>> > Cc: Gerd Hoffmann
>> > Cc: Daniel P. Berrangé
>> > Cc: Markus Armbruster
>> > Cc: Philippe Mathieu-Daudé
>> > Cc: Marc-André Lureau
>> > Cc: Thomas Huth
>> > Signed-off-by: Vivek Kasireddy
>> > ---
>> > 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
>> > 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.=]\n"
>>
>> Is "" a VC number?
>
> [Kasireddy, Vivek] Yes.
An "id" is commonly a name. Suggest connector..
>> > #endif
>> > #if defined(CONFIG_VNC)
>> > "-display vnc=[,]\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.`` :
``curses[,charset=]``
>>
>> 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!
RE: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
Hi Markus,
Thank you for the review.
> Vivek Kasireddy 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
> > Cc: Gerd Hoffmann
> > Cc: Daniel P. Berrangé
> > Cc: Markus Armbruster
> > Cc: Philippe Mathieu-Daudé
> > Cc: Marc-André Lureau
> > Cc: Thomas Huth
> > Signed-off-by: Vivek Kasireddy
> > ---
> > 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". 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.
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
>
> 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:
> > 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.=]\n"
>
> Is "" a VC number?
[Kasireddy, Vivek] Yes.
>
> > #endif
> > #if defined(CONFIG_VNC)
> > "-display vnc=[,]\n"
>
> 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.
>
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 945c550909..651aaaf174 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -37,6 +37,7 @@
> > #include "qapi/qapi-commands-misc.h"
> > #include "qemu/cutils.h"
> > #include "qemu/main-loop.h"
> > +#include "qemu/option.h"
> >
> > #include "ui/console.h"
> > #include "ui/gtk.h"
> > @@ -115,6 +116,7 @@
> > #endif
> >
> > #define HOTKEY_MODIFIERS(GDK_CONTROL_MASK | GDK_MOD1_MASK)
> > +#define MAX_NUM_ATTEMPTS 5
>
> Could use a comment, and maybe a more telling name (this one doesn't
> tell me what is being attempted).
[Kasireddy, Vivek] How about MAX_NUM_RETRIES?
gdk_monitor_get_model() can return NULL but if I wait for sometime
(few milliseconds) and try again it may give a valid value. So, I need a
macro to limit the number of attempts or retries.
>
> >
> > static const guint16 *keycode_map;
> > static size_t keycode_maplen;
> > @@ -126,6 +128,15 @@ struct VCChardev {
> > };
> > typedef struct
Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
Vivek Kasireddy 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
> Cc: Gerd Hoffmann
> Cc: Daniel P. Berrangé
> Cc: Markus Armbruster
> Cc: Philippe Mathieu-Daudé
> Cc: Marc-André Lureau
> Cc: Thomas Huth
> Signed-off-by: Vivek Kasireddy
> ---
> 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?
Ignorant question: what's a "physical monitor/connector name"?
Would you mind breaking the lines a bit earlier, between column 70 and
75?
> +# 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:
> 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.=]\n"
Is "" a VC number?
> #endif
> #if defined(CONFIG_VNC)
> "-display vnc=[,]\n"
Remainder of my review is on style only. Style suggestions are not
demands :)
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 945c550909..651aaaf174 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -37,6 +37,7 @@
> #include "qapi/qapi-commands-misc.h"
> #include "qemu/cutils.h"
> #include "qemu/main-loop.h"
> +#include "qemu/option.h"
>
> #include "ui/console.h"
> #include "ui/gtk.h"
> @@ -115,6 +116,7 @@
> #endif
>
> #define HOTKEY_MODIFIERS(GDK_CONTROL_MASK | GDK_MOD1_MASK)
> +#define MAX_NUM_ATTEMPTS 5
Could use a comment, and maybe a more telling name (this one doesn't
tell me what is being attempted).
>
> static const guint16 *keycode_map;
> static size_t keycode_maplen;
> @@ -126,6 +128,15 @@ struct VCChardev {
> };
> typedef struct VCChardev VCChardev;
>
> +struct gd_monitor_data {
> +GtkDisplayState *s;
> +GdkDisplay *dpy;
> +GdkMonitor *monitor;
> +QEMUTimer *hp_timer;
> +int attempt;
> +};
> +typedef struct gd_monitor_data gd_monitor_data;
We usually contract these like
typedef struct gd_monitor_data {
...
} gd_monitor_data;
> +
> #define TYPE_CHARDEV_VC "chardev-vc"
> DECLARE_INSTANCE_CHECKER(VCChardev, VC_CHARDEV,
> TYPE_CHARDEV_VC)
> @@ -1385,6 +1396,147 @@ static void gd_menu_untabify(GtkMenuItem *item, void
> *opaque)
> }
> }
>
> +static void gd_monitor_fullscreen(GdkDisplay *dpy, VirtualConsole *vc,
> + gint monitor_num)
> +{
> +GtkDisplayState *s = vc->s;
> +
> +if (!vc->window) {
> +gd_tab_window_create(vc);
> +}
> +gtk_window_fullscreen_on_monitor(GTK_WINDOW(vc->window),
> + gdk_display_get_default_screen(dpy),
> + monitor_num);
> +s->full_screen = TRUE;
> +gd_update_cursor(vc);
> +}
> +
> +static int gd_monitor_lookup(GdkDisplay *dpy, char *label)
> +{
> +GdkMonitor *monitor;
> +const char *monitor_name;
> +int i, total_monitors;
> +
> +total_monitors = gdk_display_get_n_monitors(dpy);
> +for (i = 0; i < total_monitors; i++) {
Suggest to format like this:
int
