On Fri, Nov 20, 2015 at 5:04 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> On Fri, Nov 20, 2015 at 12:17 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
>> >
>> > ---
>> > 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@xxxxxxxxxxxxxxxxxxxxx
>> > 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?
Up to you. I do prefer to have something like _append() and
_prepend(), but that's my personal preference.
--
Fabiano Fidêncio
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel