[Spice-devel] [PATCH 15/18] worker: move display_channel_free_some
Fabiano Fidêncio
fidencio at redhat.com
Mon Nov 23 01:21:19 PST 2015
On Fri, Nov 20, 2015 at 5:44 PM, Fabiano Fidêncio <fabiano at fidencio.org> wrote:
> 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>
>>
>> Author: Marc-André Lureau <marcandre.lureau at gmail.com>
>> ---
>> server/dcc-encoders.c | 25 +++++++++
>> server/dcc-encoders.h | 3 +-
>> server/display-channel.c | 108 ++++++++++++++++++++++++++++++++++++++
>> server/display-channel.h | 6 +++
>> server/red_worker.c | 134 +++--------------------------------------------
>> 5 files changed, 147 insertions(+), 129 deletions(-)
>>
>> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
>> index 4d8eab1..ea975e7 100644
>> --- a/server/dcc-encoders.c
>> +++ b/server/dcc-encoders.c
>> @@ -506,6 +506,31 @@ void dcc_free_glz_drawable(DisplayChannelClient *dcc, RedGlzDrawable *drawable)
>> }
>> }
>>
>> +/*
>> + * Remove from the global lz dictionary some glz_drawables that have no reference to
>> + * Drawable (their qxl drawables are released too).
>> + * NOTE - the caller should prevent encoding using the dictionary during the operation
>> + */
>> +int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc)
>> +{
>> + RingItem *ring_link;
>> + int n = 0;
>> +
>> + if (!dcc) {
>> + return 0;
>> + }
>> + ring_link = ring_get_head(&dcc->glz_drawables);
>> + while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
>> + RedGlzDrawable *glz_drawable = SPICE_CONTAINEROF(ring_link, RedGlzDrawable, link);
>> + ring_link = ring_next(&dcc->glz_drawables, ring_link);
>> + if (!glz_drawable->drawable) {
>> + dcc_free_glz_drawable(dcc, glz_drawable);
>> + n++;
>> + }
>> + }
>> + return n;
>> +}
>> +
>> void dcc_free_glz_drawables_to_free(DisplayChannelClient* dcc)
>> {
>> RingItem *ring_link;
>> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
>> index 231d37f..b0306e8 100644
>> --- a/server/dcc-encoders.h
>> +++ b/server/dcc-encoders.h
>> @@ -36,12 +36,12 @@ typedef struct RedCompressBuf RedCompressBuf;
>> typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
>> typedef struct RedGlzDrawable RedGlzDrawable;
>>
>> -
>> void dcc_encoders_init (DisplayChannelClient *dcc);
>> void dcc_free_glz_drawable_instance (DisplayChannelClient *dcc,
>> GlzDrawableInstanceItem *item);
>> void dcc_free_glz_drawable (DisplayChannelClient *dcc,
>> RedGlzDrawable *drawable);
>> +int dcc_free_some_independent_glz_drawables (DisplayChannelClient *dcc);
>> void dcc_free_glz_drawables (DisplayChannelClient *dcc);
>> void dcc_free_glz_drawables_to_free (DisplayChannelClient* dcc);
>> void dcc_freeze_glz (DisplayChannelClient *dcc);
>> @@ -163,5 +163,6 @@ struct RedGlzDrawable {
>> DisplayChannelClient *dcc;
>> };
>>
>> +#define RED_RELEASE_BUNCH_SIZE 64
>>
>> #endif /* DCC_ENCODERS_H_ */
>> diff --git a/server/display-channel.c b/server/display-channel.c
>> index ecf6f7d..affd2dd 100644
>> --- a/server/display-channel.c
>> +++ b/server/display-channel.c
>> @@ -918,3 +918,111 @@ void display_channel_free_glz_drawables(DisplayChannel *display)
>> dcc_free_glz_drawables(dcc);
>> }
>> }
>> +
>> +static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
>> +{
>> + RingItem *ring_item = ring_get_tail(&display->current_list);
>> + Drawable *drawable;
>> + Container *container;
>> +
>> + if (!ring_item) {
>> + return FALSE;
>> + }
>> +
>> + drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
>> + if (force_glz_free) {
>> + RingItem *glz_item, *next_item;
>> + RedGlzDrawable *glz;
>> + DRAWABLE_FOREACH_GLZ_SAFE(drawable, glz_item, next_item, glz) {
>> + dcc_free_glz_drawable(glz->dcc, glz);
>> + }
>> + }
>> + drawable_draw(display, drawable);
>> + container = drawable->tree_item.base.container;
>> +
>> + current_remove_drawable(display, drawable);
>> + container_cleanup(container);
>> + return TRUE;
>> +}
>> +
>> +void display_channel_current_flush(DisplayChannel *display, int surface_id)
>> +{
>> + while (!ring_is_empty(&display->surfaces[surface_id].current_list)) {
>> + free_one_drawable(display, FALSE);
>> + }
>> + current_remove_all(display, surface_id);
>> +}
>> +
>> +void display_channel_free_some(DisplayChannel *display)
>> +{
>> + int n = 0;
>> + DisplayChannelClient *dcc;
>> + RingItem *item, *next;
>> +
>> +#if FIXME
>> + spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d", display->drawable_count,
>> + worker->red_drawable_count, worker->glz_drawable_count);
>> +#endif
>> + FOREACH_DCC(display, item, next, dcc) {
>> + GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
>> +
>> + if (glz_dict) {
>> + // encoding using the dictionary is prevented since the following operations might
>> + // change the dictionary
>> + pthread_rwlock_wrlock(&glz_dict->encode_lock);
>> + n = dcc_free_some_independent_glz_drawables(dcc);
>> + }
>> + }
>> +
>> + while (!ring_is_empty(&display->current_list) && n++ < RED_RELEASE_BUNCH_SIZE) {
>> + free_one_drawable(display, TRUE);
>> + }
>> +
>> + FOREACH_DCC(display, item, next, dcc) {
>> + GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
>> +
>> + if (glz_dict) {
>> + pthread_rwlock_unlock(&glz_dict->encode_lock);
>> + }
>> + }
>> +}
>> +
>> +static Drawable* drawable_try_new(DisplayChannel *display)
>> +{
>> + Drawable *drawable;
>> +
>> + if (!display->free_drawables)
>> + return NULL;
>> +
>> + drawable = &display->free_drawables->u.drawable;
>> + display->free_drawables = display->free_drawables->u.next;
>> + display->drawable_count++;
>> +
>> + return drawable;
>> +}
>> +
>> +Drawable *display_channel_drawable_try_new(DisplayChannel *display,
>> + int group_id, int process_commands_generation)
>> +{
>> + Drawable *drawable;
>> +
>> + while (!(drawable = drawable_try_new(display))) {
>> + if (!free_one_drawable(display, FALSE))
>> + return NULL;
>> + }
>> +
>> + bzero(drawable, sizeof(Drawable));
>> + drawable->refs = 1;
>> + drawable->creation_time = red_get_monotonic_time();
>> + ring_item_init(&drawable->list_link);
>> + ring_item_init(&drawable->surface_list_link);
>> + ring_item_init(&drawable->tree_item.base.siblings_link);
>> + drawable->tree_item.base.type = TREE_ITEM_TYPE_DRAWABLE;
>> + region_init(&drawable->tree_item.base.rgn);
>> + ring_init(&drawable->pipes);
>> + ring_init(&drawable->glz_ring);
>> + drawable->process_commands_generation = process_commands_generation;
>> + drawable->group_id = group_id;
>> +
>> + return drawable;
>> +}
>> diff --git a/server/display-channel.h b/server/display-channel.h
>> index b49cec9..f6f7878 100644
>> --- a/server/display-channel.h
>> +++ b/server/display-channel.h
>> @@ -250,11 +250,15 @@ typedef struct UpgradeItem {
>> } UpgradeItem;
>>
>>
>> +void display_channel_free_some (DisplayChannel *display);
>> void display_channel_set_stream_video (DisplayChannel *display,
>> int stream_video);
>> 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);
>> +Drawable * display_channel_drawable_try_new (DisplayChannel *display,
>> + int group_id,
>> + int process_commands_generation);
>> void display_channel_drawable_unref (DisplayChannel *display, Drawable *drawable);
>> void display_channel_surface_unref (DisplayChannel *display,
>> uint32_t surface_id);
>> @@ -388,5 +392,7 @@ void current_remove_drawable(DisplayChannel *display, Drawable *item);
>> void red_pipes_remove_drawable(Drawable *drawable);
>> void current_remove(DisplayChannel *display, TreeItem *item);
>> void detach_streams_behind(DisplayChannel *display, QRegion *region, Drawable *drawable);
>> +void drawable_draw(DisplayChannel *display, Drawable *item);
>> +void current_remove_all(DisplayChannel *display, int surface_id);
>>
>> #endif /* DISPLAY_CHANNEL_H_ */
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index 3873da4..f9b54ad 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -138,12 +138,10 @@ typedef struct BitmapData {
>>
>> static inline int validate_surface(DisplayChannel *display, uint32_t surface_id);
>>
>> -static void drawable_draw(DisplayChannel *display, Drawable *item);
>> static void red_update_area(DisplayChannel *display, const SpiceRect *area, int surface_id);
>> static void red_update_area_till(DisplayChannel *display, const SpiceRect *area, int surface_id,
>> Drawable *last);
>> static inline void display_begin_send_message(RedChannelClient *rcc);
>> -static int red_display_free_some_independent_glz_drawables(DisplayChannelClient *dcc);
>> static void red_create_surface(DisplayChannel *display, uint32_t surface_id, uint32_t width,
>> uint32_t height, int32_t stride, uint32_t format,
>> void *line_0, int data_is_valid, int send_client);
>> @@ -170,7 +168,6 @@ static inline int validate_surface(DisplayChannel *display, uint32_t surface_id)
>> return 1;
>> }
>>
>> -
Could be removed from the patch ...
>> static int display_is_connected(RedWorker *worker)
>> {
>> return (worker->display_channel && red_channel_is_connected(
>> @@ -227,20 +224,6 @@ static void common_release_recv_buf(RedChannelClient *rcc, uint16_t type, uint32
>> }
>>
>>
>> -static Drawable* drawable_try_new(DisplayChannel *display)
>> -{
>> - Drawable *drawable;
>> -
>> - if (!display->free_drawables)
>> - return NULL;
>> -
>> - drawable = &display->free_drawables->u.drawable;
>> - display->free_drawables = display->free_drawables->u.next;
>> - display->drawable_count++;
>> -
>> - return drawable;
>> -}
>> -
>> static void drawable_free(DisplayChannel *display, Drawable *drawable)
>> {
>> ((_Drawable *)drawable)->u.next = display->free_drawables;
>> @@ -432,7 +415,7 @@ void current_remove(DisplayChannel *display, TreeItem *item)
>> }
>> }
>>
>> -static void current_remove_all(DisplayChannel *display, int surface_id)
>> +void current_remove_all(DisplayChannel *display, int surface_id)
>> {
>> Ring *ring = &display->surfaces[surface_id].current;
>> RingItem *ring_item;
>> @@ -790,31 +773,6 @@ static inline int red_handle_self_bitmap(RedWorker *worker, Drawable *drawable)
>> return TRUE;
>> }
>>
>> -static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
>> -{
>> - RingItem *ring_item = ring_get_tail(&display->current_list);
>> - Drawable *drawable;
>> - Container *container;
>> -
>> - if (!ring_item) {
>> - return FALSE;
>> - }
>> - drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
>> - if (force_glz_free) {
>> - RingItem *glz_item, *next_item;
>> - RedGlzDrawable *glz;
>> - DRAWABLE_FOREACH_GLZ_SAFE(drawable, glz_item, next_item, glz) {
>> - dcc_free_glz_drawable(glz->dcc, glz);
>> - }
>> - }
>> - drawable_draw(display, drawable);
>> - container = drawable->tree_item.base.container;
>> -
>> - current_remove_drawable(display, drawable);
>> - container_cleanup(container);
>> - return TRUE;
>> -}
>> -
>> static Drawable *get_drawable(RedWorker *worker, uint8_t effect, RedDrawable *red_drawable,
>> uint32_t group_id)
>> {
>> @@ -832,29 +790,17 @@ static Drawable *get_drawable(RedWorker *worker, uint8_t effect, RedDrawable *re
>> }
>> }
>>
>> - while (!(drawable = drawable_try_new(display))) {
>> - if (!free_one_drawable(display, FALSE))
>> - return NULL;
>> + drawable = display_channel_drawable_try_new(display, group_id, worker->process_commands_generation);
>> + if (!drawable) {
>> + return NULL;
>> }
>>
>> - bzero(drawable, sizeof(Drawable));
>> - drawable->refs = 1;
>> - drawable->creation_time = red_get_monotonic_time();
>> - ring_item_init(&drawable->list_link);
>> - ring_item_init(&drawable->surface_list_link);
>> - ring_item_init(&drawable->tree_item.base.siblings_link);
>> - drawable->tree_item.base.type = TREE_ITEM_TYPE_DRAWABLE;
>> - region_init(&drawable->tree_item.base.rgn);
>> drawable->tree_item.effect = effect;
>> drawable->red_drawable = red_drawable_ref(red_drawable);
>> - drawable->group_id = group_id;
>>
>> drawable->surface_id = red_drawable->surface_id;
>> memcpy(drawable->surface_deps, red_drawable->surface_deps, sizeof(drawable->surface_deps));
>> - ring_init(&drawable->pipes);
>> - ring_init(&drawable->glz_ring);
>>
>> - drawable->process_commands_generation = worker->process_commands_generation;
>> return drawable;
>> }
>>
>> @@ -1056,7 +1002,7 @@ static SpiceCanvas *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
>> return display->surfaces[surface_id].context.canvas;
>> }
>>
>> -static void drawable_draw(DisplayChannel *display, Drawable *drawable)
>> +void drawable_draw(DisplayChannel *display, Drawable *drawable)
>> {
>> RedSurface *surface;
>> SpiceCanvas *canvas;
>> @@ -1524,49 +1470,6 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
>> return n;
>> }
>>
>> -#define RED_RELEASE_BUNCH_SIZE 64
>> -
>> -static void red_free_some(RedWorker *worker)
>> -{
>> - DisplayChannel *display = worker->display_channel;
>> - int n = 0;
>> - DisplayChannelClient *dcc;
>> - RingItem *item, *next;
>> -
>> - spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d", display->drawable_count,
>> - worker->red_drawable_count, display->glz_drawable_count);
>> - FOREACH_DCC(worker->display_channel, item, next, dcc) {
>> - GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
>> -
>> - if (glz_dict) {
>> - // encoding using the dictionary is prevented since the following operations might
>> - // change the dictionary
>> - pthread_rwlock_wrlock(&glz_dict->encode_lock);
>> - n = red_display_free_some_independent_glz_drawables(dcc);
>> - }
>> - }
>> -
>> - while (!ring_is_empty(&display->current_list) && n++ < RED_RELEASE_BUNCH_SIZE) {
>> - free_one_drawable(display, TRUE);
>> - }
>> -
>> - FOREACH_DCC(worker->display_channel, item, next, dcc) {
>> - GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
>> -
>> - if (glz_dict) {
>> - pthread_rwlock_unlock(&glz_dict->encode_lock);
>> - }
>> - }
>> -}
>> -
>> -void display_channel_current_flush(DisplayChannel *display, int surface_id)
>> -{
>> - while (!ring_is_empty(&display->surfaces[surface_id].current_list)) {
>> - free_one_drawable(display, FALSE);
>> - }
>> - current_remove_all(display, surface_id);
>> -}
>> -
>> static void fill_base(SpiceMarshaller *base_marshaller, Drawable *drawable)
>> {
>> SpiceMsgDisplayBase base;
>> @@ -1578,31 +1481,6 @@ static void fill_base(SpiceMarshaller *base_marshaller, Drawable *drawable)
>> spice_marshall_DisplayBase(base_marshaller, &base);
>> }
>>
>> -/*
>> - * Remove from the global lz dictionary some glz_drawables that have no reference to
>> - * Drawable (their qxl drawables are released too).
>> - * NOTE - the caller should prevent encoding using the dictionary during the operation
>> - */
>> -static int red_display_free_some_independent_glz_drawables(DisplayChannelClient *dcc)
>> -{
>> - RingItem *ring_link;
>> - int n = 0;
>> -
>> - if (!dcc) {
>> - return 0;
>> - }
>> - ring_link = ring_get_head(&dcc->glz_drawables);
>> - while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
>> - RedGlzDrawable *glz_drawable = SPICE_CONTAINEROF(ring_link, RedGlzDrawable, link);
>> - ring_link = ring_next(&dcc->glz_drawables, ring_link);
>> - if (!glz_drawable->drawable) {
>> - dcc_free_glz_drawable(dcc, glz_drawable);
>> - n++;
>> - }
>> - }
>> - return n;
>> -}
>> -
>> static inline void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
>> SpiceImage *image, SpiceImage *io_image,
>> int is_lossy)
>> @@ -5057,7 +4935,7 @@ static void handle_dev_oom(void *opaque, void *payload)
>> red_channel_push(&worker->display_channel->common.base);
>> }
>> if (worker->qxl->st->qif->flush_resources(worker->qxl) == 0) {
>> - red_free_some(worker);
>> + display_channel_free_some(worker->display_channel);
>> worker->qxl->st->qif->flush_resources(worker->qxl);
>> }
>> spice_debug("OOM2 #draw=%u, #red_draw=%u, #glz_draw=%u current %u pipes %u",
>> --
>> 2.4.3
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> I have the feeling this patch could be split in a few other patches.
> If anyone takes the bullet till Monday, I will :-)
I am taking my word back. No need to split and I am ACKing it as it is.
>
> --
> Fabiano Fidêncio
> _______________________________________________
> 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