Re: [spice v2] display-channel: monitors_config: add 200ms delay

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Mon, Mar 11, 2019 at 12:33:20PM +0100, Christophe Fergeau wrote:
> 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).

Oh, well. There is several ways we can write a commit log, like
there is several ways on how we tell a story. Most of the times,
I try to explain what the change is about. I do refer to BZ that
they fix because it is helpful for maintainers and backporting
patches but I don't see why I need to duplicate BZ comments in
the commit log too.

This patch avoids sending multiple monitors_config message to the
client during short period of time and I tried to explain that.
I'm more than happy to understand how I can improve the commit
log but I did not understand how from your comment.

Victor

> 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

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]