On Tue, 2015-11-03 at 17:54 +0100, Fabiano Fidêncio wrote: > On Tue, Nov 3, 2015 at 11:20 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> > wrote: > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx> > > Why did it fail? What's the main issue? Was it achieved in some other > patch during the rebase? > I didn't go through the code because I didn't even understand the > main > point of this patch. > It appears that this is an intermediate step in a larger refactoring. I think marc-andre intended to move drawable_pipe_item_new/ref/unref to display-channel.c, but that drawable_pipe_item_unref() was not easily movable because it is intertwined with RedWorker (for example, it calls red_worker_drawable_unref()). In a later commit, these functions are all moved into dcc.[ch] (rather than display-channel.[ch]) as part of a larger refactoring. I'm ok with an intermediate commit like this. But I think this patch does too much. Comments below. I'll send a split patch as followup. > > > > --- > > server/display-channel.h | 74 ++++++++++++++++++++ > > server/pixmap-cache.h | 2 - > > server/red_worker.c | 178 +++++++++++++++++++---------------- > > ------------ > > server/tree.h | 34 --------- > > 4 files changed, 147 insertions(+), 141 deletions(-) > > > > diff --git a/server/display-channel.h b/server/display-channel.h > > index 5d2eee5..d8728a5 100644 > > --- a/server/display-channel.h > > +++ b/server/display-channel.h > > @@ -53,8 +53,10 @@ > > #include "spice_bitmap_utils.h" > > #include "spice_image_cache.h" > > #include "utils.h" > > +#include "tree.h" > > > > typedef struct DisplayChannel DisplayChannel; > > +typedef struct DisplayChannelClient DisplayChannelClient; > > > > typedef struct Drawable Drawable; > > > > @@ -143,6 +145,40 @@ struct Stream { > > uint32_t input_fps; > > }; > > > > +typedef struct DependItem { > > + Drawable *drawable; > > + RingItem ring_item; > > +} DependItem; > > + > > +struct Drawable { > > + uint8_t refs; > > + RingItem surface_list_link; > > + RingItem list_link; > > + DrawItem tree_item; > > + Ring pipes; > > + PipeItem *pipe_item_rest; > > + uint32_t size_pipe_item_rest; > > + RedDrawable *red_drawable; > > + > > + Ring glz_ring; > > + > > + red_time_t creation_time; > > + int frames_count; > > + int gradual_frames_count; > > + int last_gradual_frame; > > + Stream *stream; > > + Stream *sized_stream; > > + int streamable; > > + BitmapGradualType copy_bitmap_graduality; > > + uint32_t group_id; > > + DependItem depend_items[3]; > > + > > + int surface_id; > > + int surfaces_dest[3]; > > + > > + uint32_t process_commands_generation; > > +}; > > + > > #define STREAM_STATS > > #ifdef STREAM_STATS > > typedef struct StreamStats { > > @@ -228,6 +264,30 @@ struct DisplayChannelClient { > > uint64_t streams_max_bit_rate; > > }; > > > > +#define DCC_TO_WORKER(dcc) > > \ > > + (SPICE_CONTAINEROF((dcc)->common.base.channel, CommonChannel, > > base)->worker) > > +#define DCC_TO_DC(dcc) SPICE_CONTAINEROF((dcc) > > ->common.base.channel, \ > > + DisplayChannel, > > common.base) > > +#define RCC_TO_DCC(rcc) SPICE_CONTAINEROF((rcc), > > DisplayChannelClient, common.base) > > + > > + > > +enum { > > + PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST, > > + PIPE_ITEM_TYPE_IMAGE, > > + PIPE_ITEM_TYPE_STREAM_CREATE, > > + PIPE_ITEM_TYPE_STREAM_CLIP, > > + PIPE_ITEM_TYPE_STREAM_DESTROY, > > + PIPE_ITEM_TYPE_UPGRADE, > > + PIPE_ITEM_TYPE_MIGRATE_DATA, > > + PIPE_ITEM_TYPE_PIXMAP_SYNC, > > + PIPE_ITEM_TYPE_PIXMAP_RESET, > > + PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE, > > + PIPE_ITEM_TYPE_CREATE_SURFACE, > > + PIPE_ITEM_TYPE_DESTROY_SURFACE, > > + PIPE_ITEM_TYPE_MONITORS_CONFIG, > > + PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT, > > +}; > > + > > DisplayChannelClient* dcc_new > > (DisplayChannel *display, > > > > RedClient *client, > > > > RedsStream *stream, > > @@ -237,4 +297,18 @@ DisplayChannelClient* dcc_new > > (DisplayCha > > > > uint32_t *caps, > > > > int num_caps); > > > > +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); > > + > > + > > #endif /* DISPLAY_CHANNEL_H_ */ > > diff --git a/server/pixmap-cache.h b/server/pixmap-cache.h > > index 336c9ae..a4f6fea 100644 > > --- a/server/pixmap-cache.h > > +++ b/server/pixmap-cache.h > > @@ -28,8 +28,6 @@ > > #define BITS_CACHE_HASH_MASK (BITS_CACHE_HASH_SIZE - 1) > > #define BITS_CACHE_HASH_KEY(id) ((id) & BITS_CACHE_HASH_MASK) > > > > -typedef struct DisplayChannelClient DisplayChannelClient; > > - > > typedef struct PixmapCache PixmapCache; > > typedef struct NewCacheItem NewCacheItem; this change seems unrelated. I'd prefer to split it out. > > > > diff --git a/server/red_worker.c b/server/red_worker.c > > index e9ef821..e23c460 100644 > > --- a/server/red_worker.c > > +++ b/server/red_worker.c > > @@ -229,23 +229,6 @@ struct SpiceWatch { > > void *watch_func_opaque; > > }; > > > > -enum { > > - PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST, > > - PIPE_ITEM_TYPE_IMAGE, > > - PIPE_ITEM_TYPE_STREAM_CREATE, > > - PIPE_ITEM_TYPE_STREAM_CLIP, > > - PIPE_ITEM_TYPE_STREAM_DESTROY, > > - PIPE_ITEM_TYPE_UPGRADE, > > - PIPE_ITEM_TYPE_MIGRATE_DATA, > > - PIPE_ITEM_TYPE_PIXMAP_SYNC, > > - PIPE_ITEM_TYPE_PIXMAP_RESET, > > - PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE, > > - PIPE_ITEM_TYPE_CREATE_SURFACE, > > - PIPE_ITEM_TYPE_DESTROY_SURFACE, > > - PIPE_ITEM_TYPE_MONITORS_CONFIG, > > - PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT, > > -}; > > - > > #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS > > > > typedef struct SurfaceCreateItem { > > @@ -403,14 +386,6 @@ struct DisplayChannel { > > #endif > > }; > > > > -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; > > - > > typedef struct _Drawable _Drawable; > > struct _Drawable { > > union { > > @@ -589,7 +564,7 @@ static void red_draw_drawable(RedWorker > > *worker, Drawable *item); > > static void red_update_area(RedWorker *worker, const SpiceRect > > *area, int surface_id); > > static void red_update_area_till(RedWorker *worker, const > > SpiceRect *area, int surface_id, > > Drawable *last); > > -static inline void release_drawable(RedWorker *worker, Drawable > > *item); > > +static void red_worker_drawable_unref(RedWorker *worker, Drawable > > *drawable); > > static void red_display_release_stream(RedWorker *worker, > > StreamAgent *agent); > > static inline void red_detach_stream(RedWorker *worker, Stream > > *stream, int detach_sized); > > static void red_stop_stream(RedWorker *worker, Stream *stream); > > @@ -640,20 +615,48 @@ static void > > red_push_monitors_config(DisplayChannelClient *dcc); > > SAFE_FOREACH(link, next, drawable, &(drawable)->glz_ring, glz, > > LINK_TO_GLZ(link)) > > > > > > -#define DCC_TO_WORKER(dcc) \ > > - (SPICE_CONTAINEROF((dcc)->common.base.channel, CommonChannel, > > base)->worker) > > - > > // TODO: replace with DCC_FOREACH when it is introduced > > #define WORKER_TO_DCC(worker) \ > > (worker->display_channel ? SPICE_CONTAINEROF(worker > > ->display_channel->common.base.rcc,\ > > DisplayChannelClient, common.base) : NULL) > > > > -#define DCC_TO_DC(dcc) SPICE_CONTAINEROF((dcc) > > ->common.base.channel,\ > > - DisplayChannel, > > common.base) > > > > -#define RCC_TO_DCC(rcc) SPICE_CONTAINEROF((rcc), > > DisplayChannelClient, common.base) > > -#define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), > > CursorChannelClient, common.base) > > +/* 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(dcc->common.base.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) > > +{ > > + RedWorker *worker = DCC_TO_WORKER(dpi->dcc); > > + > > + if (--dpi->refs) { > > + return; > > + } > > + > > + spice_warn_if_fail(!ring_item_is_linked(&dpi > > ->dpi_pipe_item.link)); > > + spice_warn_if_fail(!ring_item_is_linked(&dpi->base)); > > + red_worker_drawable_unref(worker, dpi->drawable); > > + free(dpi); > > +} > > > > > > #ifdef COMPRESS_STAT > > @@ -865,49 +868,12 @@ static int cursor_is_connected(RedWorker > > *worker) > > red_channel_is_connected(RED_CHANNEL(worker > > ->cursor_channel)); > > } > > > > -static void put_drawable_pipe_item(DrawablePipeItem *dpi) > > -{ > > - RedWorker *worker = DCC_TO_WORKER(dpi->dcc); > > - > > - if (--dpi->refs) { > > - return; > > - } > > - > > - spice_assert(!ring_item_is_linked(&dpi->dpi_pipe_item.link)); > > - spice_assert(!ring_item_is_linked(&dpi->base)); > > - release_drawable(worker, dpi->drawable); > > - free(dpi); > > -} > > - > > -static inline DrawablePipeItem > > *get_drawable_pipe_item(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(dcc->common.base.channel, &dpi > > ->dpi_pipe_item, PIPE_ITEM_TYPE_DRAW); > > - dpi->refs++; > > - drawable->refs++; > > - return dpi; > > -} > > - > > -static inline DrawablePipeItem > > *ref_drawable_pipe_item(DrawablePipeItem *dpi) > > -{ > > - spice_assert(dpi->drawable); > > - dpi->refs++; > > - return dpi; > > -} > > - > > static inline void red_pipe_add_drawable(DisplayChannelClient > > *dcc, Drawable *drawable) > > { > > DrawablePipeItem *dpi; > > > > red_handle_drawable_surfaces_client_synced(dcc, drawable); > > - dpi = get_drawable_pipe_item(dcc, drawable); > > + dpi = drawable_pipe_item_new(dcc, drawable); > > red_channel_client_pipe_add(&dcc->common.base, &dpi > > ->dpi_pipe_item); > > } > > > > @@ -930,7 +896,7 @@ static inline void > > red_pipe_add_drawable_to_tail(DisplayChannelClient *dcc, Draw > > return; > > } > > red_handle_drawable_surfaces_client_synced(dcc, drawable); > > - dpi = get_drawable_pipe_item(dcc, drawable); > > + dpi = drawable_pipe_item_new(dcc, drawable); > > red_channel_client_pipe_add_tail(&dcc->common.base, &dpi > > ->dpi_pipe_item); > > } > > > > @@ -946,7 +912,7 @@ static inline void > > red_pipes_add_drawable_after(RedWorker *worker, > > num_other_linked++; > > dcc = dpi_pos_after->dcc; > > red_handle_drawable_surfaces_client_synced(dcc, drawable); > > - dpi = get_drawable_pipe_item(dcc, drawable); > > + dpi = drawable_pipe_item_new(dcc, drawable); > > red_channel_client_pipe_add_after(&dcc->common.base, &dpi > > ->dpi_pipe_item, > > &dpi_pos_after > > ->dpi_pipe_item); > > } > > @@ -1025,11 +991,12 @@ static void release_image_item(ImageItem > > *item) > > > > static void release_upgrade_item(RedWorker* worker, UpgradeItem > > *item) > > { > > - if (!--item->refs) { > > - release_drawable(worker, item->drawable); > > - free(item->rects); > > - free(item); > > - } > > + if (--item->refs) > > + return; > > + > > + red_worker_drawable_unref(worker, item->drawable); > > + free(item->rects); > > + free(item); I would also split out this style change. > > } > > > > static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc, > > uint16_t type, uint32_t size) > > @@ -1222,31 +1189,32 @@ static void > > remove_drawable_dependencies(RedWorker *worker, Drawable *drawable) > > } > > } > > > > -static inline void release_drawable(RedWorker *worker, Drawable > > *drawable) > > +static void red_worker_drawable_unref(RedWorker *worker, Drawable > > *drawable) > > { > > RingItem *item, *next; > > > > - if (!--drawable->refs) { > > - spice_assert(!drawable->tree_item.shadow); > > - spice_assert(ring_is_empty(&drawable->pipes)); > > + if (--drawable->refs) > > + return; > > > > - if (drawable->stream) { > > - red_detach_stream(worker, drawable->stream, TRUE); > > - } > > - region_destroy(&drawable->tree_item.base.rgn); > > + spice_return_if_fail(!drawable->tree_item.shadow); > > + spice_return_if_fail(ring_is_empty(&drawable->pipes)); > > + > > + if (drawable->stream) { > > + red_detach_stream(worker, drawable->stream, TRUE); > > + } > > + region_destroy(&drawable->tree_item.base.rgn); > > > > - remove_drawable_dependencies(worker, drawable); > > - red_dec_surfaces_drawable_dependencies(worker, drawable); > > - red_destroy_surface(worker, drawable->surface_id); > > + remove_drawable_dependencies(worker, drawable); > > + red_dec_surfaces_drawable_dependencies(worker, drawable); > > + red_destroy_surface(worker, drawable->surface_id); > > > > - RING_FOREACH_SAFE(item, next, &drawable->glz_ring) { > > - SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link) > > ->drawable = NULL; > > - ring_remove(item); > > - } > > - put_red_drawable(worker, drawable->red_drawable, drawable > > ->group_id); > > - free_drawable(worker, drawable); > > - worker->drawable_count--; > > + RING_FOREACH_SAFE(item, next, &drawable->glz_ring) { > > + SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link) > > ->drawable = NULL; > > + ring_remove(item); > > } > > + put_red_drawable(worker, drawable->red_drawable, drawable > > ->group_id); > > + free_drawable(worker, drawable); > > + worker->drawable_count--; These are also only style changes (return early vs. put everything inside an if statement). I'd split them out as well. I'm a little bit more ambivalent about the function rename. I think it can go in this patch even if it's not strictly necessary for the other changes. > > } > > > > static inline void remove_shadow(RedWorker *worker, DrawItem > > *item) > > @@ -1339,7 +1307,7 @@ static inline void > > current_remove_drawable(RedWorker *worker, Drawable *item) > > ring_remove(&item->tree_item.base.siblings_link); > > ring_remove(&item->list_link); > > ring_remove(&item->surface_list_link); > > - release_drawable(worker, item); > > + red_worker_drawable_unref(worker, item); > > worker->current_size--; > > } > > > > @@ -2747,7 +2715,7 @@ static inline int > > red_current_add_equal(RedWorker *worker, DrawItem *item, TreeI > > red_pipes_add_drawable(worker, drawable); > > } > > red_pipes_remove_drawable(other_drawable); > > - release_drawable(worker, other_drawable); > > + red_worker_drawable_unref(worker, other_drawable); > > return TRUE; > > } > > > > @@ -2790,7 +2758,7 @@ static inline int > > red_current_add_equal(RedWorker *worker, DrawItem *item, TreeI > > /* not sending other_drawable where possible */ > > red_pipes_remove_drawable(other_drawable); > > > > - release_drawable(worker, other_drawable); > > + red_worker_drawable_unref(worker, other_drawable); > > return TRUE; > > } > > break; > > @@ -3466,7 +3434,7 @@ static gboolean red_process_draw(RedWorker > > *worker, QXLCommandExt *ext_cmd) > > > > end: > > if (drawable != NULL) > > - release_drawable(worker, drawable); > > + red_worker_drawable_unref(worker, drawable); > > if (red_drawable != NULL) > > put_red_drawable(worker, red_drawable, ext_cmd->group_id); > > return success; > > @@ -3858,7 +3826,7 @@ static void red_update_area_till(RedWorker > > *worker, const SpiceRect *area, int s > > that red_update_area is called for, Otherwise, 'now' > > would have already been rendered. > > See the call for red_handle_depends_on_target_surface > > in red_process_draw */ > > red_draw_drawable(worker, now); > > - release_drawable(worker, now); > > + red_worker_drawable_unref(worker, now); > > } while (now != surface_last); > > validate_area(worker, area, surface_id); > > } > > @@ -3911,7 +3879,7 @@ static void red_update_area(RedWorker > > *worker, const SpiceRect *area, int surfac > > current_remove_drawable(worker, now); > > container_cleanup(worker, container); > > red_draw_drawable(worker, now); > > - release_drawable(worker, now); > > + red_worker_drawable_unref(worker, now); > > } while (now != last); > > validate_area(worker, area, surface_id); > > } > > @@ -9098,7 +9066,7 @@ static void > > display_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem > > *item > > spice_assert(item); > > switch (item->type) { > > case PIPE_ITEM_TYPE_DRAW: > > - ref_drawable_pipe_item(SPICE_CONTAINEROF(item, > > DrawablePipeItem, dpi_pipe_item)); > > + drawable_pipe_item_ref(SPICE_CONTAINEROF(item, > > DrawablePipeItem, dpi_pipe_item)); > > break; > > case PIPE_ITEM_TYPE_STREAM_CLIP: > > ((StreamClipItem *)item)->refs++; > > @@ -9121,7 +9089,7 @@ static void > > display_channel_client_release_item_after_push(DisplayChannelClient > > > > switch (item->type) { > > case PIPE_ITEM_TYPE_DRAW: > > - put_drawable_pipe_item(SPICE_CONTAINEROF(item, > > DrawablePipeItem, dpi_pipe_item)); > > + drawable_pipe_item_unref(SPICE_CONTAINEROF(item, > > DrawablePipeItem, dpi_pipe_item)); > > break; > > case PIPE_ITEM_TYPE_STREAM_CLIP: > > red_display_release_stream_clip(worker, (StreamClipItem > > *)item); > > @@ -9158,7 +9126,7 @@ static void > > display_channel_client_release_item_before_push(DisplayChannelClien > > t > > case PIPE_ITEM_TYPE_DRAW: { > > DrawablePipeItem *dpi = SPICE_CONTAINEROF(item, > > DrawablePipeItem, dpi_pipe_item); > > ring_remove(&dpi->base); > > - put_drawable_pipe_item(dpi); > > + drawable_pipe_item_unref(dpi); > > break; > > } > > case PIPE_ITEM_TYPE_STREAM_CREATE: { > > diff --git a/server/tree.h b/server/tree.h > > index 9ee6007..baba40a 100644 > > --- a/server/tree.h > > +++ b/server/tree.h > > @@ -73,40 +73,6 @@ struct DrawItem { > > #define IS_DRAW_ITEM(item) ((item)->type == > > TREE_ITEM_TYPE_DRAWABLE) > > #define DRAW_ITEM(item) ((DrawItem*)(item)) > > > > -typedef struct DependItem { > > - Drawable *drawable; > > - RingItem ring_item; > > -} DependItem; > > - > > -struct Drawable { > > - uint8_t refs; > > - RingItem surface_list_link; > > - RingItem list_link; > > - DrawItem tree_item; > > - Ring pipes; > > - PipeItem *pipe_item_rest; > > - uint32_t size_pipe_item_rest; > > - RedDrawable *red_drawable; > > - > > - Ring glz_ring; > > - > > - red_time_t creation_time; > > - int frames_count; > > - int gradual_frames_count; > > - int last_gradual_frame; > > - Stream *stream; > > - Stream *sized_stream; > > - int streamable; > > - BitmapGradualType copy_bitmap_graduality; > > - uint32_t group_id; > > - DependItem depend_items[3]; > > - > > - int surface_id; > > - int surfaces_dest[3]; > > - > > - uint32_t process_commands_generation; > > -}; > > - > > void tree_item_dump (TreeItem > > *item); > > Shadow* shadow_new (DrawItem > > *item, const SpicePoint *delta); > > Container* container_new (DrawItem > > *item); > > -- > > 2.4.3 > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel