[Spice-devel] [PATCH 04/18] worker: move dcc_add_drawable*

Frediano Ziglio fziglio at redhat.com
Fri Nov 20 08:04:53 PST 2015


> 
> On Fri, Nov 20, 2015 at 12:17 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > ---
> >  server/dcc.c             |  82 +++++++++++++++++++++++++++
> >  server/dcc.h             |  17 ++++++
> >  server/display-channel.c |  65 +++++++++++++++++----
> >  server/display-channel.h |  19 +------
> >  server/red_worker.c      | 144
> >  +----------------------------------------------
> >  5 files changed, 158 insertions(+), 169 deletions(-)
> >
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 8cc2c0a..a17fa7d 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -93,6 +93,88 @@ void dcc_push_surface_image(DisplayChannelClient *dcc,
> > int surface_id)
> >      red_channel_client_push(RED_CHANNEL_CLIENT(dcc));
> >  }
> >
> > +static void add_drawable_surface_images(DisplayChannelClient *dcc,
> > Drawable *drawable)
> > +{
> > +    DisplayChannel *display = DCC_TO_DC(dcc);
> > +    int x;
> > +
> > +    for (x = 0; x < 3; ++x) {
> > +        int surface_id;
> > +
> > +        surface_id = drawable->surface_deps[x];
> > +        if (surface_id != -1) {
> > +            if (dcc->surface_client_created[surface_id] == TRUE) {
> > +                continue;
> > +            }
> > +            dcc_create_surface(dcc, surface_id);
> > +            display_channel_current_flush(display, surface_id);
> > +            dcc_push_surface_image(dcc, surface_id);
> > +        }
> > +    }
> > +
> > +    if (dcc->surface_client_created[drawable->surface_id] == TRUE) {
> > +        return;
> > +    }
> > +
> > +    dcc_create_surface(dcc, drawable->surface_id);
> > +    display_channel_current_flush(display, drawable->surface_id);
> > +    dcc_push_surface_image(dcc, drawable->surface_id);
> > +}
> > +
> > +DrawablePipeItem *drawable_pipe_item_ref(DrawablePipeItem *dpi)
> > +{
> > +    dpi->refs++;
> > +    return dpi;
> > +}
> > +
> > +void drawable_pipe_item_unref(DrawablePipeItem *dpi)
> > +{
> > +    DisplayChannel *display = DCC_TO_DC(dpi->dcc);
> > +
> > +    if (--dpi->refs != 0)
> > +        return;
> > +
> > +    spice_return_if_fail(!ring_item_is_linked(&dpi->dpi_pipe_item.link));
> > +    spice_return_if_fail(!ring_item_is_linked(&dpi->base));
> > +    display_channel_drawable_unref(display, dpi->drawable);
> > +    free(dpi);
> > +}
> > +
> > +static DrawablePipeItem *drawable_pipe_item_new(DisplayChannelClient *dcc,
> > Drawable *drawable)
> > +{
> > +    DrawablePipeItem *dpi;
> > +
> > +    dpi = spice_malloc0(sizeof(*dpi));
> > +    dpi->drawable = drawable;
> > +    dpi->dcc = dcc;
> > +    ring_item_init(&dpi->base);
> > +    ring_add(&drawable->pipes, &dpi->base);
> > +    red_channel_pipe_item_init(RED_CHANNEL_CLIENT(dcc)->channel,
> > +                               &dpi->dpi_pipe_item, PIPE_ITEM_TYPE_DRAW);
> > +    dpi->refs++;
> > +    drawable->refs++;
> > +    return dpi;
> > +}
> > +
> > +void dcc_add_drawable(DisplayChannelClient *dcc, Drawable *drawable, bool
> > to_tail)
> > +{
> > +    DrawablePipeItem *dpi = drawable_pipe_item_new(dcc, drawable);
> > +
> > +    add_drawable_surface_images(dcc, drawable);
> > +    if (to_tail)
> > +        red_channel_client_pipe_add_tail(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item);
> > +    else
> > +        red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item);
> > +}
> 
> Not related to this patch but would be _way_ more clear if we have
> dcc_append_drawable() and dcc_prepend_drawable().
> 

Actually was more or less (with different names) like that.

> > +
> > +void dcc_add_drawable_after(DisplayChannelClient *dcc, Drawable *drawable,
> > PipeItem *pos)
> > +{
> > +    DrawablePipeItem *dpi = drawable_pipe_item_new(dcc, drawable);
> > +
> > +    add_drawable_surface_images(dcc, drawable);
> > +    red_channel_client_pipe_add_after(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item, pos);
> > +}
> > +
> >  static void dcc_init_stream_agents(DisplayChannelClient *dcc)
> >  {
> >      int i;
> > diff --git a/server/dcc.h b/server/dcc.h
> > index c62c3c9..f7c01a1 100644
> > --- a/server/dcc.h
> > +++ b/server/dcc.h
> > @@ -135,6 +135,17 @@ typedef struct ImageItem {
> >      uint8_t data[0];
> >  } ImageItem;
> >
> > +typedef struct DrawablePipeItem {
> > +    RingItem base;  /* link for a list of pipe items held by Drawable */
> > +    PipeItem dpi_pipe_item; /* link for the client's pipe itself */
> > +    Drawable *drawable;
> > +    DisplayChannelClient *dcc;
> > +    uint8_t refs;
> > +} DrawablePipeItem;
> > +
> > +void                       drawable_pipe_item_unref
> > (DrawablePipeItem *dpi);
> > +DrawablePipeItem*          drawable_pipe_item_ref
> > (DrawablePipeItem *dpi);
> > +
> >  DisplayChannelClient*      dcc_new
> >  (DisplayChannel *display,
> >                                                                        RedClient
> >                                                                        *client,
> >                                                                        RedsStream
> >                                                                        *stream,
> > @@ -169,6 +180,12 @@ void                       dcc_palette_cache_palette
> > (DisplayCha
> >                                                                        uint8_t
> >                                                                        *flags);
> >  int                        dcc_pixmap_cache_unlocked_add
> >  (DisplayChannelClient *dcc,
> >                                                                        uint64_t
> >                                                                        id,
> >                                                                        uint32_t
> >                                                                        size,
> >                                                                        int
> >                                                                        lossy);
> > +void                       dcc_add_drawable
> > (DisplayChannelClient *dcc,
> > +
> > Drawable
> > *drawable,
> > +                                                                      bool
> > to_tail);
> > +void                       dcc_add_drawable_after
> > (DisplayChannelClient *dcc,
> > +
> > Drawable
> > *drawable,
> > +
> > PipeItem
> > *pos);
> >
> >  typedef struct compress_send_data_t {
> >      void*    comp_buf;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index d455a0d..ecf6f7d 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -325,6 +325,50 @@ static void
> > streams_update_visible_region(DisplayChannel *display, Drawable *dra
> >      }
> >  }
> >
> > +static void pipes_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> > +{
> > +    DisplayChannelClient *dcc;
> > +    RingItem *dcc_ring_item, *next;
> > +
> > +    spice_warn_if(!ring_is_empty(&drawable->pipes));
> > +    FOREACH_DCC(display, dcc_ring_item, next, dcc) {
> > +        dcc_add_drawable(dcc, drawable, FALSE);
> > +    }
> > +}
> > +
> > +static void pipes_add_drawable_after(DisplayChannel *display,
> > +                                     Drawable *drawable, Drawable
> > *pos_after)
> > +{
> > +    DrawablePipeItem *dpi_pos_after;
> > +    RingItem *dpi_link, *dpi_next;
> > +    DisplayChannelClient *dcc;
> > +    int num_other_linked = 0;
> > +
> > +    DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next,
> > dpi_pos_after) {
> > +        num_other_linked++;
> > +        dcc_add_drawable_after(dpi_pos_after->dcc, drawable,
> > &dpi_pos_after->dpi_pipe_item);
> > +    }
> > +    if (num_other_linked == 0) {
> > +        pipes_add_drawable(display, drawable);
> > +        return;
> > +    }
> > +    if (num_other_linked != display->common.base.clients_num) {
> > +        RingItem *item, *next;
> > +        spice_debug("TODO: not O(n^2)");
> > +        FOREACH_DCC(display, item, next, dcc) {
> > +            int sent = 0;
> > +            DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next,
> > dpi_pos_after) {
> > +                if (dpi_pos_after->dcc == dcc) {
> > +                    sent = 1;
> > +                    break;
> > +                }
> > +            }
> > +            if (!sent) {
> > +                dcc_add_drawable(dcc, drawable, FALSE);
> > +            }
> > +        }
> > +    }
> > +}
> >
> >  static void current_add_drawable(DisplayChannel *display,
> >                                   Drawable *drawable, RingItem *pos)
> > @@ -366,9 +410,9 @@ static int current_add_equal(DisplayChannel *display,
> > DrawItem *item, TreeItem *
> >          other_drawable->refs++;
> >          current_remove_drawable(display, other_drawable);
> >          if (add_after) {
> > -            red_pipes_add_drawable_after(display, drawable,
> > other_drawable);
> > +            pipes_add_drawable_after(display, drawable, other_drawable);
> >          } else {
> > -            red_pipes_add_drawable(display, drawable);
> > +            pipes_add_drawable(display, drawable);
> >          }
> >          red_pipes_remove_drawable(other_drawable);
> >          display_channel_drawable_unref(display, other_drawable);
> > @@ -396,7 +440,7 @@ static int current_add_equal(DisplayChannel *display,
> > DrawItem *item, TreeItem *
> >                                          common.base.channel_link);
> >                  dpi = SPICE_CONTAINEROF(dpi_ring_item, DrawablePipeItem,
> >                  base);
> >                  while (worker_ring_item && (!dpi || dcc != dpi->dcc)) {
> > -                    dcc_add_drawable(dcc, drawable);
> > +                    dcc_add_drawable(dcc, drawable, FALSE);
> >                      worker_ring_item =
> >                      ring_next(&RED_CHANNEL(display)->clients,
> >                                                   worker_ring_item);
> >                      dcc = SPICE_CONTAINEROF(worker_ring_item,
> >                      DisplayChannelClient,
> > @@ -423,7 +467,7 @@ static int current_add_equal(DisplayChannel *display,
> > DrawItem *item, TreeItem *
> >              current_add_drawable(display, drawable,
> >              &other->siblings_link);
> >              red_pipes_remove_drawable(other_drawable);
> >              current_remove_drawable(display, other_drawable);
> > -            red_pipes_add_drawable(display, drawable);
> > +            pipes_add_drawable(display, drawable);
> >              return TRUE;
> >          }
> >          break;
> > @@ -787,25 +831,26 @@ void display_channel_print_stats(DisplayChannel
> > *display)
> >  #endif
> >  }
> >
> > -int display_channel_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> > +void display_channel_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> >  {
> > -    int ret = FALSE, surface_id = drawable->surface_id;
> > +    int success = FALSE, surface_id = drawable->surface_id;
> >      RedDrawable *red_drawable = drawable->red_drawable;
> >      Ring *ring = &display->surfaces[surface_id].current;
> >
> >      if (has_shadow(red_drawable)) {
> > -        ret = current_add_with_shadow(display, ring, drawable);
> > +        success = current_add_with_shadow(display, ring, drawable);
> >      } else {
> >          drawable->streamable = drawable_can_stream(display, drawable);
> > -        ret = current_add(display, ring, drawable);
> > +        success = current_add(display, ring, drawable);
> >      }
> >
> > +    if (success)
> > +        pipes_add_drawable(display, drawable);
> > +
> >  #ifdef RED_WORKER_STAT
> >      if ((++display->add_count % 100) == 0)
> >          display_channel_print_stats(display);
> >  #endif
> > -
> > -    return ret;
> >  }
> >
> >  int display_channel_wait_for_migrate_data(DisplayChannel *display)
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index bad61d1..233c391 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -113,19 +113,6 @@ enum {
> >      PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> >  };
> >
> > -typedef struct DrawablePipeItem {
> > -    RingItem base;  /* link for a list of pipe items held by Drawable */
> > -    PipeItem dpi_pipe_item; /* link for the client's pipe itself */
> > -    Drawable *drawable;
> > -    DisplayChannelClient *dcc;
> > -    uint8_t refs;
> > -} DrawablePipeItem;
> > -
> > -DrawablePipeItem*          drawable_pipe_item_new
> > (DisplayChannelClient *dcc,
> > -
> > Drawable
> > *drawable);
> > -void                       drawable_pipe_item_unref
> > (DrawablePipeItem *dpi);
> > -DrawablePipeItem*          drawable_pipe_item_ref
> > (DrawablePipeItem *dpi);
> > -
> >  typedef struct MonitorsConfig {
> >      int refs;
> >      int count;
> > @@ -270,7 +257,7 @@ void
> > display_channel_surface_unref
> > (DisplayCha
> >                                                                        uint32_t
> >                                                                        surface_id);
> >  bool                       display_channel_surface_has_canvas
> >  (DisplayChannel *display,
> >                                                                        uint32_t
> >                                                                        surface_id);
> > -int                        display_channel_add_drawable
> > (DisplayChannel *display,
> > +void                       display_channel_add_drawable
> > (DisplayChannel *display,
> >                                                                        Drawable
> >                                                                        *drawable);
> >  void                       display_channel_current_flush
> >  (DisplayChannel *display,
> >                                                                        int
> >                                                                        surface_id);
> > @@ -394,12 +381,8 @@ static inline void region_add_clip_rects(QRegion *rgn,
> > SpiceClipRects *data)
> >      }
> >  }
> >
> > -void red_pipes_add_drawable(DisplayChannel *display, Drawable *drawable);
> >  void current_remove_drawable(DisplayChannel *display, Drawable *item);
> > -void red_pipes_add_drawable_after(DisplayChannel *display,
> > -                                  Drawable *drawable, Drawable
> > *pos_after);
> >  void red_pipes_remove_drawable(Drawable *drawable);
> > -void dcc_add_drawable(DisplayChannelClient *dcc, Drawable *drawable);
> >  void current_remove(DisplayChannel *display, TreeItem *item);
> >  void detach_streams_behind(DisplayChannel *display, QRegion *region,
> >  Drawable *drawable);
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index f6dfe28..b8dfbb9 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -191,44 +191,6 @@ static void red_create_surface(DisplayChannel
> > *display, uint32_t surface_id, uin
> >                                 uint32_t height, int32_t stride, uint32_t
> >                                 format,
> >                                 void *line_0, int data_is_valid, int
> >                                 send_client);
> >
> > -/* fixme: move to display channel */
> > -DrawablePipeItem *drawable_pipe_item_new(DisplayChannelClient *dcc,
> > -                                         Drawable *drawable)
> > -{
> > -    DrawablePipeItem *dpi;
> > -
> > -    dpi = spice_malloc0(sizeof(*dpi));
> > -    dpi->drawable = drawable;
> > -    dpi->dcc = dcc;
> > -    ring_item_init(&dpi->base);
> > -    ring_add(&drawable->pipes, &dpi->base);
> > -    red_channel_pipe_item_init(RED_CHANNEL_CLIENT(dcc)->channel,
> > -                               &dpi->dpi_pipe_item, PIPE_ITEM_TYPE_DRAW);
> > -    dpi->refs++;
> > -    drawable->refs++;
> > -    return dpi;
> > -}
> > -
> > -DrawablePipeItem *drawable_pipe_item_ref(DrawablePipeItem *dpi)
> > -{
> > -    dpi->refs++;
> > -    return dpi;
> > -}
> > -
> > -void drawable_pipe_item_unref(DrawablePipeItem *dpi)
> > -{
> > -    DisplayChannel *display = DCC_TO_DC(dpi->dcc);
> > -
> > -    if (--dpi->refs != 0) {
> > -        return;
> > -    }
> > -
> > -    spice_return_if_fail(!ring_item_is_linked(&dpi->dpi_pipe_item.link));
> > -    spice_return_if_fail(!ring_item_is_linked(&dpi->base));
> > -    display_channel_drawable_unref(display, dpi->drawable);
> > -    free(dpi);
> > -}
> > -
> >  QXLInstance* red_worker_get_qxl(RedWorker *worker)
> >  {
> >      spice_return_val_if_fail(worker != NULL, NULL);
> > @@ -252,35 +214,6 @@ static inline int validate_surface(DisplayChannel
> > *display, uint32_t surface_id)
> >  }
> >
> >
> > -static inline void red_handle_drawable_surfaces_client_synced(
> > -                        DisplayChannelClient *dcc, Drawable *drawable)
> > -{
> > -    DisplayChannel *display = DCC_TO_DC(dcc);
> > -    int x;
> > -
> > -    for (x = 0; x < 3; ++x) {
> > -        int surface_id;
> > -
> > -        surface_id = drawable->surface_deps[x];
> > -        if (surface_id != -1) {
> > -            if (dcc->surface_client_created[surface_id] == TRUE) {
> > -                continue;
> > -            }
> > -            dcc_create_surface(dcc, surface_id);
> > -            display_channel_current_flush(display, surface_id);
> > -            dcc_push_surface_image(dcc, surface_id);
> > -        }
> > -    }
> > -
> > -    if (dcc->surface_client_created[drawable->surface_id] == TRUE) {
> > -        return;
> > -    }
> > -
> > -    dcc_create_surface(dcc, drawable->surface_id);
> > -    display_channel_current_flush(display, drawable->surface_id);
> > -    dcc_push_surface_image(dcc, drawable->surface_id);
> > -}
> > -
> >  static int display_is_connected(RedWorker *worker)
> >  {
> >      return (worker->display_channel && red_channel_is_connected(
> > @@ -293,76 +226,6 @@ static int cursor_is_connected(RedWorker *worker)
> >          red_channel_is_connected(RED_CHANNEL(worker->cursor_channel));
> >  }
> >
> > -void dcc_add_drawable(DisplayChannelClient *dcc, Drawable *drawable)
> > -{
> > -    DrawablePipeItem *dpi;
> > -
> > -    red_handle_drawable_surfaces_client_synced(dcc, drawable);
> > -    dpi = drawable_pipe_item_new(dcc, drawable);
> > -    red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item);
> > -}
> > -
> > -void red_pipes_add_drawable(DisplayChannel *display, Drawable *drawable)
> > -{
> > -    DisplayChannelClient *dcc;
> > -    RingItem *dcc_ring_item, *next;
> > -
> > -    spice_warn_if(!ring_is_empty(&drawable->pipes));
> > -    FOREACH_DCC(display, dcc_ring_item, next, dcc) {
> > -        dcc_add_drawable(dcc, drawable);
> > -    }
> > -}
> > -
> > -static void dcc_add_drawable_to_tail(DisplayChannelClient *dcc, Drawable
> > *drawable)
> > -{
> > -    DrawablePipeItem *dpi;
> > -
> > -    if (!dcc) {
> > -        return;
> > -    }
> > -    red_handle_drawable_surfaces_client_synced(dcc, drawable);
> > -    dpi = drawable_pipe_item_new(dcc, drawable);
> > -    red_channel_client_pipe_add_tail(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item);
> > -}
> > -
> > -void red_pipes_add_drawable_after(DisplayChannel *display,
> > -                                  Drawable *drawable, Drawable *pos_after)
> > -{
> > -    DrawablePipeItem *dpi, *dpi_pos_after;
> > -    RingItem *dpi_link, *dpi_next;
> > -    DisplayChannelClient *dcc;
> > -    int num_other_linked = 0;
> > -
> > -    DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next,
> > dpi_pos_after) {
> > -        num_other_linked++;
> > -        dcc = dpi_pos_after->dcc;
> > -        red_handle_drawable_surfaces_client_synced(dcc, drawable);
> > -        dpi = drawable_pipe_item_new(dcc, drawable);
> > -        red_channel_client_pipe_add_after(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item,
> > -                                          &dpi_pos_after->dpi_pipe_item);
> > -    }
> > -    if (num_other_linked == 0) {
> > -        red_pipes_add_drawable(display, drawable);
> > -        return;
> > -    }
> > -    if (num_other_linked != display->common.base.clients_num) {
> > -        RingItem *item, *next;
> > -        spice_debug("TODO: not O(n^2)");
> > -        FOREACH_DCC(display, item, next, dcc) {
> > -            int sent = 0;
> > -            DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next,
> > dpi_pos_after) {
> > -                if (dpi_pos_after->dcc == dcc) {
> > -                    sent = 1;
> > -                    break;
> > -                }
> > -            }
> > -            if (!sent) {
> > -                dcc_add_drawable(dcc, drawable);
> > -            }
> > -        }
> > -    }
> > -}
> > -
> >  static inline PipeItem *red_pipe_get_tail(DisplayChannelClient *dcc)
> >  {
> >      if (!dcc) {
> > @@ -1226,9 +1089,8 @@ static inline void red_process_draw(RedWorker
> > *worker, RedDrawable *red_drawable
> >          goto cleanup;
> >      }
> >
> > -    if (display_channel_add_drawable(worker->display_channel, drawable)) {
> > -        red_pipes_add_drawable(worker->display_channel, drawable);
> > -    }
> > +    display_channel_add_drawable(worker->display_channel, drawable);
> > +
> >  cleanup:
> >      display_channel_drawable_unref(display, drawable);
> >  }
> > @@ -2509,7 +2371,7 @@ static void
> > red_add_lossless_drawable_dependencies(RedChannelClient *rcc,
> >
> >      if (!sync_rendered) {
> >          // pushing the pipe item back to the pipe
> > -        dcc_add_drawable_to_tail(dcc, item);
> > +        dcc_add_drawable(dcc, item, TRUE);
> >          // the surfaces areas will be sent as DRAW_COPY commands, that
> >          // will be executed before the current drawable
> >          for (i = 0; i < num_deps; i++) {
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> Patch seems good, ACK!
> --
> Fabiano FidĂȘncio
> 

Ehmm... considering that your suggested code was like before...
Should we rollback a bit?

Frediano


More information about the Spice-devel mailing list