>
> From: Victor Toso <me@xxxxxxxxxxxxxx>
>
> The device driver might take a few interactions to reconfigure all
> displays but in each change it can send it down to spice-server. The
> client is not interested in temporary states, only the final monitor
> config can be helpful.
>
> This patch adds a 200ms delay to wait for more changes from guest
> device. Tested only with QXL in Fedora 29.
>
Which kind of guest? Windows or Linux?
> Simple example below, with 4 displays enabled but removing the display 1
> (starting from 0), spice server receives:
>
> | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors config
> | count:3 max:4
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0
> | 1024x740
>
> Sends to the client #1
>
> | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors config
> | count:3 max:4
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0
> | 1024x740
>
> Sends to the client #2
>
> | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
> | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors config
> | count:1 max:4
> | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
> | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors config
> | count:3 max:4
> | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 1024x740
>
> Sends to the client #3
>
> | 16:50:14.973:
> | display-channel.c:2462:display_channel_update_monitors_config: Will
> | update monitors in 200ms ~~
> | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors config
> | count:4 max:4
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0
> | 1024x740
>
> Sends to the client #4 and final
>
> With this patch, only the last one is sent.
> Resolves: rhbz#1673072
> Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> ---
> server/display-channel-private.h | 2 ++
> server/display-channel.c | 16 +++++++++++++---
> server/display-channel.h | 2 +-
> 3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/server/display-channel-private.h
> b/server/display-channel-private.h
> index 58179531..f1a4314c 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
>
> int gl_draw_async_count;
>
> + guint monitors_config_update_delay_id;
> +
> /* TODO: some day unify this, make it more runtime.. */
> stat_info_t add_stat;
> stat_info_t exclude_stat;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index e68ed10f..7e2584ec 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2430,13 +2430,17 @@ gboolean
> display_channel_validate_surface(DisplayChannel *display, uint32_t surf
> return TRUE;
> }
>
> -void display_channel_push_monitors_config(DisplayChannel *display)
> +gboolean display_channel_push_monitors_config(gpointer userdata)
> {
> DisplayChannelClient *dcc;
> + DisplayChannel *display = userdata;
> +
> + display->priv->monitors_config_update_delay_id = 0;
>
> FOREACH_DCC(display, dcc) {
> dcc_push_monitors_config(dcc);
> }
> + return G_SOURCE_REMOVE;
> }
>
> void display_channel_update_monitors_config(DisplayChannel *display,
> @@ -2444,13 +2448,19 @@ void
> display_channel_update_monitors_config(DisplayChannel *display,
> uint16_t count, uint16_t
> max_allowed)
> {
>
> - if (display->priv->monitors_config)
> + if (display->priv->monitors_config) {
> monitors_config_unref(display->priv->monitors_config);
> + }
>
> display->priv->monitors_config =
> monitors_config_new(config->heads, count, max_allowed);
>
> - display_channel_push_monitors_config(display);
> + if (display->priv->monitors_config_update_delay_id != 0) {
> + g_source_remove(display->priv->monitors_config_update_delay_id);
> + }
> +
> + display->priv->monitors_config_update_delay_id =
> + g_timeout_add (200, display_channel_push_monitors_config, display);
Don't use g_timeout_add, it adds the source to the main context so the function
won't be executed in DisplayChannel thread causing thread race conditions.
I don't see the part of code that remove the source in case DisplayChannel
is destroyed.
> }
>
> void display_channel_set_monitors_config_to_primary(DisplayChannel *display)
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 948018cf..1d64afce 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -149,7 +149,7 @@ void display_channel_gl_draw_done
> (DisplayCha
> void display_channel_update_monitors_config(DisplayChannel *display,
> QXLMonitorsConfig *config,
> uint16_t count, uint16_t
> max_allowed);
> void display_channel_set_monitors_config_to_primary(DisplayChannel
> *display);
> -void display_channel_push_monitors_config(DisplayChannel *display);
> +gboolean display_channel_push_monitors_config(gpointer display);
>
> gboolean display_channel_validate_surface(DisplayChannel *display, uint32_t
> surface_id);
> gboolean display_channel_surface_has_canvas(DisplayChannel *display,
> uint32_t surface_id);
Otherwise the patch seems good.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel