Hey,
One comment, even if the commit log mentions a bug #, it does not seem
to be describing what the bug is/how it's fixed (maybe this can be
inferred from the traces, but I did not read them closely as the log
does not hint that a bug can be seen by looking at them).
Christophe
On Mon, Mar 11, 2019 at 10:02:27AM +0000, Victor Toso wrote:
> 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.
>
> 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>
> ---
>
> Changes v1->v2
> * Using reds_core_timer_* api, which calls the timeout function from the
> right context (Frediano);
> * Remove the SpiceTimer on finalize (Frediano)
>
> server/display-channel-private.h | 2 ++
> server/display-channel.c | 27 +++++++++++++++++++++++++--
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/server/display-channel-private.h b/server/display-channel-private.h
> index 58179531..848abe81 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
>
> int gl_draw_async_count;
>
> + SpiceTimer *monitors_config_update_timer;
> +
> /* 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 9bb7fa44..a2f95956 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
> display_channel_destroy_surfaces(self);
> image_cache_reset(&self->priv->image_cache);
>
> + if (self->priv->monitors_config_update_timer) {
> + RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> + reds_core_timer_remove(reds, self->priv->monitors_config_update_timer);
> + self->priv->monitors_config_update_timer = NULL;
> + }
> +
> if (spice_extra_checks) {
> unsigned int count;
> _Drawable *drawable;
> @@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
> NULL);
> if (display) {
> display_channel_set_stream_video(display, stream_video);
> + display->priv->monitors_config_update_timer = reds_core_timer_add(reds,
> + (SpiceTimerFunc) display_channel_push_monitors_config, display);
> }
> return display;
> }
> @@ -2435,6 +2443,11 @@ void display_channel_push_monitors_config(DisplayChannel *display)
> {
> DisplayChannelClient *dcc;
>
> + if (display->priv->monitors_config_update_timer) {
> + RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
> + reds_core_timer_cancel(reds, display->priv->monitors_config_update_timer);
> + }
> +
> FOREACH_DCC(display, dcc) {
> dcc_push_monitors_config(dcc);
> }
> @@ -2444,14 +2457,24 @@ void display_channel_update_monitors_config(DisplayChannel *display,
> QXLMonitorsConfig *config,
> uint16_t count, uint16_t max_allowed)
> {
> + RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
>
> - 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_timer) {
> + spice_warning("No timer for monitor_config updates");
> + display_channel_push_monitors_config(display);
> + return;
> + }
> +
> + /* Cancel previous timer, if it was set */
> + reds_core_timer_cancel(reds, display->priv->monitors_config_update_timer);
> + reds_core_timer_start(reds, display->priv->monitors_config_update_timer, 200);
> }
>
> void display_channel_set_monitors_config_to_primary(DisplayChannel *display)
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel