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

[Victor Toso]

Hi,

On Mon, Mar 11, 2019 at 07:34:19AM -0400, Frediano Ziglio wrote:
> > Changes v1->v2
> > * Using reds_core_timer_* api, which calls the timeout function from the
> >   right context (Frediano);
> 
> Sorry, these functions suffers the same problem, you have to
> use the core interface of the DisplayChannel.
> 
> OT: I was thinking to add some extra checks on these thread
> conditions.

I checked but not fully :(

timer_start() calls:

    g_source_attach(timer->source, timer->context);

...and I thought timer->context was the right context here but
one can see that in timer_add()

    timer->context = iface->main_context;

> 
> > * 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);
> 
> Why not putting this in _init or constructed ?

You tell me, I'm not sure where it fits better in the server
arch. I thought that at the time we create the display is good
enough. Let me know if I should move it.

> 
> >      }
> >      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");
> 
> Why a warning?
> I think the code you wrote should initilise the timer once
> and should never be NULL.

It should never be NULL indeed.

> > +        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)
> 
> I had a second though on this patch. Let's assume I have only a monitor and
> this sequence:
> - monitor_config
> - update
> Your code will change potentially to:
> - update
> - monitor_config
> Now, the client will receive an update before the monitor
> configuration, potentially will ignore the update as updating
> an invalid area.
> If there are no other updates the client window could even
> remain black waiting for data to arrive.

I'm afraid I don't understand the situation above. AFAICS, we
only delay ongoing updates. I'm looking this as regression in
drm/qxl btw but this server-side fix looks sane enough to me,
that is, if 2-3 updates are happening in less than < 200ms, we
should only send the last one.

Cheers,
Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190311/b7212c22/attachment.sig>