[Spice-devel] [PATCH 14/15] worker: tried to move dpi functions to display channel, fail
Fabiano Fidêncio
fidencio at redhat.com
Tue Nov 3 08:54:28 PST 2015
On Tue, Nov 3, 2015 at 11:20 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
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.
>
> ---
> 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;
>
> 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);
> }
>
> 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--;
> }
>
> 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(DisplayChannelClient
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list