>
> On Tue, 2015-11-10 at 11:42 -0600, Jonathon Jongsma wrote:
> > I'd like to propose splitting out the changes related to UpgradeItem since
> > they're not really related. I'll post a split patch.
>
>
> Hmm, nevermind. Now that I look at it again, the only 'unrelated' UpgradeItem
> change is that release_upgrade_item() is renamed to upgrade_item_unref(). But
> the signature of this function needs to change to accept a DisplayChannel*
> instead of a RedWorker*, so I think it's OK to change the name at the same
> time.
>
> So I'd ACK this patch.
>
>
Merged
Frediano
>
>
> >
> >
> >
> > On Tue, 2015-11-10 at 14:16 +0000, Frediano Ziglio wrote:
> > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > >
> > > ---
> > > server/display-channel.h | 24 ++++++++--
> > > server/red_worker.c | 118
> > > ++++++++++++++++++++++----------------------
> > > --
> > > -
> > > 2 files changed, 74 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index 898ff01..1566c21 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -273,6 +273,15 @@ void monitors_config_unref
> > >
> > > (MonitorsCo
> > > #define NUM_TRACE_ITEMS (1 << TRACE_ITEMS_SHIFT)
> > > #define ITEMS_TRACE_MASK (NUM_TRACE_ITEMS - 1)
> > >
> > > +#define NUM_DRAWABLES 1000
> > > +typedef struct _Drawable _Drawable;
> > > +struct _Drawable {
> > > + union {
> > > + Drawable drawable;
> > > + _Drawable *next;
> > > + } u;
> > > +};
> > > +
> > > struct DisplayChannel {
> > > CommonChannel common; // Must be the first thing
> > >
> > > @@ -281,16 +290,17 @@ struct DisplayChannel {
> > > uint32_t num_renderers;
> > > uint32_t renderers[RED_RENDERER_LAST];
> > > uint32_t renderer;
> > > -
> > > - Ring current_list; // of TreeItem
> > > - uint32_t current_size;
> > > -
> > > int enable_jpeg;
> > > int jpeg_quality;
> > > int enable_zlib_glz_wrap;
> > > int zlib_level;
> > >
> > > - RedCompressBuf *free_compress_bufs;
> > > + Ring current_list; // of TreeItem
> > > + uint32_t current_size;
> > > +
> > > + uint32_t drawable_count;
> > > + _Drawable drawables[NUM_DRAWABLES];
> > > + _Drawable *free_drawables;
> > >
> > > int stream_video;
> > > uint32_t stream_count;
> > > @@ -302,6 +312,7 @@ struct DisplayChannel {
> > > uint64_t streams_size_total;
> > >
> > > ImageCache image_cache;
> > > + RedCompressBuf *free_compress_bufs;
> > >
> > > #ifdef RED_STATISTICS
> > > uint64_t *cache_hits_counter;
> > > @@ -344,5 +355,8 @@ void
> > > display_channel_attach_stream
> > >
> > > (DisplayCha
> > > int display_channel_get_streams_timeout
> > > (DisplayChannel *display);
> > > void display_channel_compress_stats_print
> > > (const
> > > DisplayChannel *display);
> > > void display_channel_compress_stats_reset
> > > (DisplayChannel *display);
> > > +void display_channel_drawable_unref
> > > (DisplayChannel *display, Drawable *drawable);
> > > +
> > > +
> > >
> > > #endif /* DISPLAY_CHANNEL_H_ */
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 86ce7a1..23c54e8 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -300,14 +300,6 @@ struct RedGlzDrawable {
> > > pthread_mutex_t glz_dictionary_list_lock = PTHREAD_MUTEX_INITIALIZER;
> > > Ring glz_dictionary_list = {&glz_dictionary_list, &glz_dictionary_list};
> > >
> > > -typedef struct _Drawable _Drawable;
> > > -struct _Drawable {
> > > - union {
> > > - Drawable drawable;
> > > - _Drawable *next;
> > > - } u;
> > > -};
> > > -
> > > typedef struct DrawContext {
> > > SpiceCanvas *canvas;
> > > int canvas_draws_on_surface;
> > > @@ -332,8 +324,6 @@ typedef struct RedSurface {
> > > QXLReleaseInfoExt create, destroy;
> > > } RedSurface;
> > >
> > > -#define NUM_DRAWABLES 1000
> > > -
> > > typedef struct RedWorker {
> > > pthread_t thread;
> > > clockid_t clockid;
> > > @@ -354,14 +344,10 @@ typedef struct RedWorker {
> > > uint32_t n_surfaces;
> > > SpiceImageSurfaces image_surfaces;
> > >
> > > - uint32_t drawable_count;
> > > uint32_t red_drawable_count;
> > > uint32_t glz_drawable_count;
> > > uint32_t bits_unique;
> > >
> > > - _Drawable drawables[NUM_DRAWABLES];
> > > - _Drawable *free_drawables;
> > > -
> > > RedMemSlotInfo mem_slots;
> > >
> > > SpiceImageCompression image_compression;
> > > @@ -435,7 +421,6 @@ 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 void red_worker_drawable_unref(RedWorker *worker, Drawable
> > > *drawable);
> > > static void detach_stream(DisplayChannel *display, Stream *stream, int
> > > detach_sized);
> > > static inline void display_channel_stream_maintenance(DisplayChannel
> > > *display, Drawable *candidate, Drawable *sect);
> > > static inline void display_begin_send_message(RedChannelClient *rcc);
> > > @@ -635,7 +620,7 @@ DrawablePipeItem
> > > *drawable_pipe_item_ref(DrawablePipeItem
> > > *dpi)
> > >
> > > void drawable_pipe_item_unref(DrawablePipeItem *dpi)
> > > {
> > > - RedWorker *worker = DCC_TO_WORKER(dpi->dcc);
> > > + DisplayChannel *display = DCC_TO_DC(dpi->dcc);
> > >
> > > if (--dpi->refs != 0) {
> > > return;
> > > @@ -643,7 +628,7 @@ void drawable_pipe_item_unref(DrawablePipeItem *dpi)
> > >
> > > 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);
> > > + display_channel_drawable_unref(display, dpi->drawable);
> > > free(dpi);
> > > }
> > >
> > > @@ -869,12 +854,12 @@ static void release_image_item(ImageItem *item)
> > > }
> > > }
> > >
> > > -static void release_upgrade_item(RedWorker* worker, UpgradeItem *item)
> > > +static void upgrade_item_unref(DisplayChannel *display, UpgradeItem
> > > *item)
> > > {
> > > if (--item->refs != 0)
> > > return;
> > >
> > > - red_worker_drawable_unref(worker, item->drawable);
> > > + display_channel_drawable_unref(display, item->drawable);
> > > free(item->rects);
> > > free(item);
> > > }
> > > @@ -912,30 +897,33 @@ static void
> > > red_reset_palette_cache(DisplayChannelClient
> > > *dcc)
> > > red_palette_cache_reset(dcc, CLIENT_PALETTE_CACHE_SIZE);
> > > }
> > >
> > > -static inline Drawable *alloc_drawable(RedWorker *worker)
> > > +static Drawable* drawable_try_new(DisplayChannel *display)
> > > {
> > > Drawable *drawable;
> > > - if (!worker->free_drawables) {
> > > +
> > > + if (!display->free_drawables)
> > > return NULL;
> > > - }
> > > - drawable = &worker->free_drawables->u.drawable;
> > > - worker->free_drawables = worker->free_drawables->u.next;
> > > +
> > > + drawable = &display->free_drawables->u.drawable;
> > > + display->free_drawables = display->free_drawables->u.next;
> > > + display->drawable_count++;
> > > +
> > > return drawable;
> > > }
> > >
> > > -static inline void free_drawable(RedWorker *worker, Drawable *item)
> > > +static void drawable_free(DisplayChannel *display, Drawable *drawable)
> > > {
> > > - ((_Drawable *)item)->u.next = worker->free_drawables;
> > > - worker->free_drawables = (_Drawable *)item;
> > > + ((_Drawable *)drawable)->u.next = display->free_drawables;
> > > + display->free_drawables = (_Drawable *)drawable;
> > > }
> > >
> > > -static void drawables_init(RedWorker *worker)
> > > +static void drawables_init(DisplayChannel *display)
> > > {
> > > int i;
> > >
> > > - worker->free_drawables = NULL;
> > > + display->free_drawables = NULL;
> > > for (i = 0; i < NUM_DRAWABLES; i++) {
> > > - free_drawable(worker, &worker->drawables[i].u.drawable);
> > > + drawable_free(display, &display->drawables[i].u.drawable);
> > > }
> > > }
> > >
> > > @@ -1031,7 +1019,7 @@ static void remove_depended_item(DependItem *item)
> > > ring_remove(&item->ring_item);
> > > }
> > >
> > > -static void drawable_unref_surface_deps(RedWorker *worker, Drawable
> > > *drawable)
> > > +static void drawable_unref_surface_deps(DisplayChannel *display,
> > > Drawable
> > > *drawable)
> > > {
> > > int x;
> > > int surface_id;
> > > @@ -1041,11 +1029,11 @@ static void drawable_unref_surface_deps(RedWorker
> > > *worker, Drawable *drawable)
> > > if (surface_id == -1) {
> > > continue;
> > > }
> > > - red_surface_unref(worker, surface_id);
> > > + red_surface_unref(COMMON_CHANNEL(display)->worker, surface_id);
> > > }
> > > }
> > >
> > > -static void remove_drawable_dependencies(RedWorker *worker, Drawable
> > > *drawable)
> > > +static void drawable_remove_dependencies(DisplayChannel *display,
> > > Drawable
> > > *drawable)
> > > {
> > > int x;
> > > int surface_id;
> > > @@ -1058,7 +1046,7 @@ static void remove_drawable_dependencies(RedWorker
> > > *worker, Drawable *drawable)
> > > }
> > > }
> > >
> > > -static void red_worker_drawable_unref(RedWorker *worker, Drawable
> > > *drawable)
> > > +void display_channel_drawable_unref(DisplayChannel *display, Drawable
> > > *drawable)
> > > {
> > > RingItem *item, *next;
> > >
> > > @@ -1069,21 +1057,21 @@ static void red_worker_drawable_unref(RedWorker
> > > *worker, Drawable *drawable)
> > > spice_warn_if_fail(ring_is_empty(&drawable->pipes));
> > >
> > > if (drawable->stream) {
> > > - detach_stream(worker->display_channel, drawable->stream, TRUE);
> > > + detach_stream(display, drawable->stream, TRUE);
> > > }
> > > region_destroy(&drawable->tree_item.base.rgn);
> > >
> > > - remove_drawable_dependencies(worker, drawable);
> > > - drawable_unref_surface_deps(worker, drawable);
> > > - red_surface_unref(worker, drawable->surface_id);
> > > + drawable_remove_dependencies(display, drawable);
> > > + drawable_unref_surface_deps(display, drawable);
> > > + red_surface_unref(COMMON_CHANNEL(display)->worker, drawable
> > > ->surface_id);
> > >
> > > RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> > > SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable
> > > =
> > > NULL;
> > > ring_remove(item);
> > > }
> > > - red_drawable_unref(worker, drawable->red_drawable,
> > > drawable->group_id);
> > > - free_drawable(worker, drawable);
> > > - worker->drawable_count--;
> > > + red_drawable_unref(COMMON_CHANNEL(display)->worker, drawable
> > > ->red_drawable, drawable->group_id);
> > > + drawable_free(display, drawable);
> > > + display->drawable_count--;
> > > }
> > >
> > > static inline void remove_shadow(DrawItem *item)
> > > @@ -1172,7 +1160,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);
> > > - red_worker_drawable_unref(worker, item);
> > > + display_channel_drawable_unref(display, item);
> > > display->current_size--;
> > > }
> > >
> > > @@ -2334,6 +2322,7 @@ static inline int
> > > is_drawable_independent_from_surfaces(Drawable *drawable)
> > >
> > > static inline int red_current_add_equal(RedWorker *worker, DrawItem
> > > *item,
> > > TreeItem *other)
> > > {
> > > + DisplayChannel *display = worker->display_channel;
> > > DrawItem *other_draw_item;
> > > Drawable *drawable;
> > > Drawable *other_drawable;
> > > @@ -2363,7 +2352,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);
> > > - red_worker_drawable_unref(worker, other_drawable);
> > > + display_channel_drawable_unref(display, other_drawable);
> > > return TRUE;
> > > }
> > >
> > > @@ -2406,7 +2395,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);
> > >
> > > - red_worker_drawable_unref(worker, other_drawable);
> > > + display_channel_drawable_unref(display, other_drawable);
> > > return TRUE;
> > > }
> > > break;
> > > @@ -2879,6 +2868,7 @@ static bool free_one_drawable(RedWorker *worker,
> > > int
> > > force_glz_free)
> > > static Drawable *get_drawable(RedWorker *worker, uint8_t effect,
> > > RedDrawable
> > > *red_drawable,
> > > uint32_t group_id)
> > > {
> > > + DisplayChannel *display = worker->display_channel;
> > > Drawable *drawable;
> > > int x;
> > >
> > > @@ -2893,12 +2883,12 @@ static Drawable *get_drawable(RedWorker *worker,
> > > uint8_t effect, RedDrawable *re
> > > }
> > > }
> > >
> > > - while (!(drawable = alloc_drawable(worker))) {
> > > - if (!free_one_drawable(worker, FALSE))
> > > + while (!(drawable = drawable_try_new(display))) {
> > > + if (!free_one_drawable(COMMON_CHANNEL(display)->worker, FALSE))
> > > return NULL;
> > > }
> > > - worker->drawable_count++;
> > > - memset(drawable, 0, sizeof(Drawable));
> > > +
> > > + bzero(drawable, sizeof(Drawable));
> > > drawable->refs = 1;
> > > drawable->creation_time = red_get_monotonic_time();
> > > ring_item_init(&drawable->list_link);
> > > @@ -2995,6 +2985,7 @@ static inline void
> > > red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
> > > static inline void red_process_draw(RedWorker *worker, RedDrawable
> > > *red_drawable,
> > > uint32_t group_id)
> > > {
> > > + DisplayChannel *display = worker->display_channel;
> > > int surface_id;
> > > Drawable *drawable = get_drawable(worker, red_drawable->effect,
> > > red_drawable, group_id);
> > >
> > > @@ -3045,7 +3036,7 @@ static inline void red_process_draw(RedWorker
> > > *worker,
> > > RedDrawable *red_drawable
> > > red_pipes_add_drawable(worker, drawable);
> > > }
> > > cleanup:
> > > - red_worker_drawable_unref(worker, drawable);
> > > + display_channel_drawable_unref(display, drawable);
> > > }
> > >
> > > static inline void red_create_surface(RedWorker *worker, uint32_t
> > > surface_id,uint32_t width,
> > > @@ -3374,13 +3365,14 @@ 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);
> > > - red_worker_drawable_unref(worker, now);
> > > + display_channel_drawable_unref(display, now);
> > > } while (now != surface_last);
> > > validate_area(worker, area, surface_id);
> > > }
> > >
> > > static void red_update_area(RedWorker *worker, const SpiceRect *area,
> > > int
> > > surface_id)
> > > {
> > > + DisplayChannel *display = worker->display_channel;
> > > RedSurface *surface;
> > > Ring *ring;
> > > RingItem *ring_item;
> > > @@ -3427,7 +3419,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);
> > > - red_worker_drawable_unref(worker, now);
> > > + display_channel_drawable_unref(display, now);
> > > } while (now != last);
> > > validate_area(worker, area, surface_id);
> > > }
> > > @@ -3612,7 +3604,7 @@ static void red_free_some(RedWorker *worker)
> > > DisplayChannelClient *dcc;
> > > RingItem *item, *next;
> > >
> > > - spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d", worker
> > > ->drawable_count,
> > > + spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d", display
> > > ->drawable_count,
> > > worker->red_drawable_count, worker->glz_drawable_count);
> > > FOREACH_DCC(worker->display_channel, item, next, dcc) {
> > > GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
> > > @@ -7579,7 +7571,7 @@ void red_show_tree(RedWorker *worker)
> > >
> > > static void display_channel_client_on_disconnect(RedChannelClient *rcc)
> > > {
> > > - DisplayChannel *display_channel;
> > > + DisplayChannel *display;
> > > DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> > > CommonChannel *common;
> > > RedWorker *worker;
> > > @@ -7590,9 +7582,9 @@ static void
> > > display_channel_client_on_disconnect(RedChannelClient *rcc)
> > > spice_info(NULL);
> > > common = SPICE_CONTAINEROF(rcc->channel, CommonChannel, base);
> > > worker = common->worker;
> > > - display_channel = (DisplayChannel *)rcc->channel;
> > > - spice_assert(display_channel == worker->display_channel);
> > > - display_channel_compress_stats_print(display_channel);
> > > + display = (DisplayChannel *)rcc->channel;
> > > + spice_assert(display == worker->display_channel);
> > > + display_channel_compress_stats_print(display);
> > > pixmap_cache_unref(dcc->pixmap_cache);
> > > dcc->pixmap_cache = NULL;
> > > red_release_glz(dcc);
> > > @@ -7604,10 +7596,10 @@ static void
> > > display_channel_client_on_disconnect(RedChannelClient *rcc)
> > >
> > > // this was the last channel client
> > > if (!red_channel_is_connected(rcc->channel)) {
> > > - red_display_destroy_compress_bufs(display_channel);
> > > + red_display_destroy_compress_bufs(display);
> > > }
> > > spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d",
> > > - worker->drawable_count, worker->red_drawable_count,
> > > + display->drawable_count, worker->red_drawable_count,
> > > worker->glz_drawable_count);
> > > }
> > >
> > > @@ -8658,7 +8650,7 @@ static void
> > > display_channel_client_release_item_after_push(DisplayChannelClient
> > > display_stream_clip_unref(display, (StreamClipItem *)item);
> > > break;
> > > case PIPE_ITEM_TYPE_UPGRADE:
> > > - release_upgrade_item(DCC_TO_WORKER(dcc), (UpgradeItem *)item);
> > > + upgrade_item_unref(display, (UpgradeItem *)item);
> > > break;
> > > case PIPE_ITEM_TYPE_IMAGE:
> > > release_image_item((ImageItem *)item);
> > > @@ -8706,7 +8698,7 @@ static void
> > > display_channel_client_release_item_before_push(DisplayChannelClient
> > > break;
> > > }
> > > case PIPE_ITEM_TYPE_UPGRADE:
> > > - release_upgrade_item(DCC_TO_WORKER(dcc), (UpgradeItem *)item);
> > > + upgrade_item_unref(display, (UpgradeItem *)item);
> > > break;
> > > case PIPE_ITEM_TYPE_IMAGE:
> > > release_image_item((ImageItem *)item);
> > > @@ -8818,6 +8810,7 @@ static void display_channel_create(RedWorker
> > > *worker,
> > > int migrate)
> > > init_streams(display_channel);
> > > image_cache_init(&display_channel->image_cache);
> > > ring_init(&display_channel->current_list);
> > > + drawables_init(display_channel);
> > > }
> > >
> > > static void guest_set_client_capabilities(RedWorker *worker)
> > > @@ -9378,7 +9371,7 @@ static void handle_dev_oom(void *opaque, void
> > > *payload)
> > > spice_assert(worker->running);
> > > // streams? but without streams also leak
> > > spice_debug("OOM1 #draw=%u, #red_draw=%u, #glz_draw=%u current %u
> > > pipes
> > > %u",
> > > - worker->drawable_count,
> > > + display->drawable_count,
> > > worker->red_drawable_count,
> > > worker->glz_drawable_count,
> > > display->current_size,
> > > @@ -9392,7 +9385,7 @@ static void handle_dev_oom(void *opaque, void
> > > *payload)
> > > worker->qxl->st->qif->flush_resources(worker->qxl);
> > > }
> > > spice_debug("OOM2 #draw=%u, #red_draw=%u, #glz_draw=%u current %u
> > > pipes
> > > %u",
> > > - worker->drawable_count,
> > > + display->drawable_count,
> > > worker->red_drawable_count,
> > > worker->glz_drawable_count,
> > > display->current_size,
> > > @@ -9943,7 +9936,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > > RedDispatcher *red_dispatcher)
> > > worker->zlib_glz_state = zlib_glz_state;
> > > worker->driver_cap_monitors_config = 0;
> > > image_surface_init(worker);
> > > - drawables_init(worker);
> > > stat_init(&worker->add_stat, add_stat_name);
> > > stat_init(&worker->exclude_stat, exclude_stat_name);
> > > stat_init(&worker->__exclude_stat, __exclude_stat_name);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel