[Spice-devel] [PATCH 3/3] display: move more logic in add_drawable()
Frediano Ziglio
fziglio at redhat.com
Fri Nov 27 06:21:33 PST 2015
>
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
>
> Acked-by: Fabiano Fidêncio <fidencio at redhat.com>
> fziglio ???
> ---
> server/display-channel.c | 129
> +++++++++++++++++------------------------------
> server/display-channel.h | 10 ++--
> server/red_worker.c | 18 ++++---
> 3 files changed, 60 insertions(+), 97 deletions(-)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 0b4415b..d8ab849 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -916,7 +916,7 @@ void display_channel_print_stats(DisplayChannel *display)
> #endif
> }
>
> -static inline void red_inc_surfaces_drawable_dependencies(DisplayChannel
> *display, Drawable *drawable)
> +static void drawable_ref_surface_deps(DisplayChannel *display, Drawable
> *drawable)
> {
> int x;
> int surface_id;
> @@ -932,23 +932,19 @@ static inline void
> red_inc_surfaces_drawable_dependencies(DisplayChannel *displa
> }
> }
>
> -static void red_get_area(DisplayChannel *display, int surface_id, const
> SpiceRect *area,
> - uint8_t *dest, int dest_stride, int update)
> +static void surface_read_bits(DisplayChannel *display, int surface_id,
> + const SpiceRect *area, uint8_t *dest, int
> dest_stride)
> {
> SpiceCanvas *canvas;
> - RedSurface *surface;
> -
> - surface = &display->surfaces[surface_id];
> - if (update) {
> - display_channel_draw(display, area, surface_id);
> - }
> + RedSurface *surface = &display->surfaces[surface_id];
>
> canvas = surface->context.canvas;
> canvas->ops->read_bits(canvas, dest, dest_stride, area);
> }
>
> -static int display_channel_handle_self_bitmap(DisplayChannel *display,
> Drawable *drawable)
> +static void handle_self_bitmap(DisplayChannel *display, Drawable *drawable)
> {
> + RedDrawable *red_drawable = drawable->red_drawable;
> SpiceImage *image;
> int32_t width;
> int32_t height;
> @@ -957,26 +953,17 @@ static int
> display_channel_handle_self_bitmap(DisplayChannel *display, Drawable
> RedSurface *surface;
> int bpp;
> int all_set;
> - RedDrawable *red_drawable = drawable->red_drawable;
> -
> - if (!red_drawable->self_bitmap) {
> - return TRUE;
> - }
>
This check is moved below.
> surface = &display->surfaces[drawable->surface_id];
>
> bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
> -
> - width = red_drawable->self_bitmap_area.right
> - - red_drawable->self_bitmap_area.left;
> - height = red_drawable->self_bitmap_area.bottom
> - - red_drawable->self_bitmap_area.top;
> + width = red_drawable->self_bitmap_area.right -
> red_drawable->self_bitmap_area.left;
> + height = red_drawable->self_bitmap_area.bottom -
> red_drawable->self_bitmap_area.top;
> dest_stride = SPICE_ALIGN(width * bpp, 4);
>
> image = spice_new0(SpiceImage, 1);
> image->descriptor.type = SPICE_IMAGE_TYPE_BITMAP;
> image->descriptor.flags = 0;
> -
> QXL_SET_IMAGE_ID(image, QXL_IMAGE_GROUP_RED,
> display_channel_generate_uid(display));
> image->u.bitmap.flags = surface->context.top_down ?
> SPICE_BITMAP_FLAGS_TOP_DOWN : 0;
> image->u.bitmap.format =
> spice_bitmap_from_surface_type(surface->context.format);
> @@ -989,8 +976,9 @@ static int
> display_channel_handle_self_bitmap(DisplayChannel *display, Drawable
> image->u.bitmap.data = spice_chunks_new_linear(dest, height *
> dest_stride);
> image->u.bitmap.data->flags |= SPICE_CHUNKS_FLAGS_FREE;
>
> - red_get_area(display, drawable->surface_id,
> - &red_drawable->self_bitmap_area, dest, dest_stride, TRUE);
> + display_channel_draw(display, &red_drawable->self_bitmap_area,
> drawable->surface_id);
> + surface_read_bits(display, drawable->surface_id,
> + &red_drawable->self_bitmap_area, dest, dest_stride);
>
> /* For 32bit non-primary surfaces we need to keep any non-zero
> high bytes as the surface may be used as source to an alpha_blend */
> @@ -1005,26 +993,25 @@ static int
> display_channel_handle_self_bitmap(DisplayChannel *display, Drawable
> }
>
> red_drawable->self_bitmap_image = image;
> - return TRUE;
> }
>
> -static inline void add_to_surface_dependency(DisplayChannel *display, int
> depend_on_surface_id,
> - DependItem *depend_item,
> Drawable *drawable)
> +static void surface_add_reverse_dependency(DisplayChannel *display, int
> surface_id,
> + DependItem *depend_item, Drawable
> *drawable)
> {
> RedSurface *surface;
>
> - if (depend_on_surface_id == -1) {
> + if (surface_id == -1) {
> depend_item->drawable = NULL;
> return;
> }
>
> - surface = &display->surfaces[depend_on_surface_id];
> + surface = &display->surfaces[surface_id];
>
> depend_item->drawable = drawable;
> ring_add(&surface->depend_on_me, &depend_item->ring_item);
> }
>
> -static inline int red_handle_surfaces_dependencies(DisplayChannel *display,
> Drawable *drawable)
> +static int handle_surface_deps(DisplayChannel *display, Drawable *drawable)
> {
> int x;
>
> @@ -1032,8 +1019,8 @@ static inline int
> red_handle_surfaces_dependencies(DisplayChannel *display, Draw
> // surface self dependency is handled by shadows in "current", or by
> // handle_self_bitmap
> if (drawable->surface_deps[x] != drawable->surface_id) {
> - add_to_surface_dependency(display, drawable->surface_deps[x],
> - &drawable->depend_items[x], drawable);
> + surface_add_reverse_dependency(display,
> drawable->surface_deps[x],
> + &drawable->depend_items[x],
> drawable);
>
> if (drawable->surface_deps[x] == 0) {
> QRegion depend_region;
> @@ -1091,49 +1078,28 @@ static int validate_drawable_bbox(DisplayChannel
> *display, RedDrawable *drawable
> return TRUE;
> }
>
> -Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t
> effect,
> - RedDrawable *red_drawable, uint32_t
> group_id,
> - uint32_t process_commands_generation)
> -{
> - Drawable *drawable;
> - int x;
> -
> - if (!validate_drawable_bbox(display, red_drawable)) {
> - return NULL;
> - }
> - for (x = 0; x < 3; ++x) {
> - if (red_drawable->surface_deps[x] != -1
> - && !validate_surface(display, red_drawable->surface_deps[x])) {
> - return NULL;
> - }
> - }
>
> - drawable = display_channel_drawable_try_new(display, group_id,
> process_commands_generation);
> - if (!drawable) {
> - return NULL;
> - }
> -
> - drawable->tree_item.effect = effect;
> - drawable->red_drawable = red_drawable_ref(red_drawable);
> -
> - drawable->surface_id = red_drawable->surface_id;
> - memcpy(drawable->surface_deps, red_drawable->surface_deps,
> sizeof(drawable->surface_deps));
> -
These looks a bit wrong here. I means, you copy surfaces without incrementing
references so if you need to free drawable you will get less reference then expected.
Looks like a bad dependency. This patch is going to address (perhaps) this but
IMHO not in a good way. display_channel_add_drawable is going to do too much.
> - return drawable;
> -}
> -
> -void display_channel_add_drawable(DisplayChannel *display, Drawable
> *drawable)
> +int display_channel_add_drawable(DisplayChannel *display, Drawable
> *drawable, RedDrawable *red_drawable)
> {
> - int success = FALSE, surface_id = drawable->surface_id;
> - RedDrawable *red_drawable = drawable->red_drawable;
> + int surface_id, x;
>
> + drawable->red_drawable = red_drawable_ref(red_drawable);
> + drawable->tree_item.effect = red_drawable->effect;
> + surface_id = drawable->surface_id = red_drawable->surface_id;
> + if (!validate_drawable_bbox(display, red_drawable))
> + return FALSE;
> + // FIXME here can leak if after we return!
> + display->surfaces[surface_id].refs++;
this should be moved after all read-only checks...
> red_drawable->mm_time = reds_get_mm_time();
This line looks misplaced
> - surface_id = drawable->surface_id;
>
> - display->surfaces[surface_id].refs++;
> + for (x = 0; x < 3; ++x) {
> + drawable->surface_deps[x] = red_drawable->surface_deps[x];
> + if (drawable->surface_deps[x] != -1
> + && !validate_surface(display, drawable->surface_deps[x]))
> + return FALSE;
> + }
>
or when we are returning here reference counting problems will happen.
A test would be awesome here!
- test invalid surface_id
- test invalid surface_deps
- all mix possible
On failure references to surfaces should be the same.
> region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
> -
> if (red_drawable->clip.type == SPICE_CLIP_TYPE_RECTS) {
> QRegion rgn;
>
> @@ -1143,44 +1109,43 @@ void display_channel_add_drawable(DisplayChannel
> *display, Drawable *drawable)
> region_destroy(&rgn);
> }
>
> + if (region_is_empty(&drawable->tree_item.base.rgn))
> + return TRUE;
> +
Is this move not causing possible leaks ?
> /*
> surface->refs is affected by a drawable (that is
> dependent on the surface) as long as the drawable is alive.
> However, surface->depend_on_me is affected by a drawable only
> as long as it is in the current tree (hasn't been rendered yet).
> */
> - red_inc_surfaces_drawable_dependencies(display, drawable);
> + drawable_ref_surface_deps(display, drawable);
>
> - if (region_is_empty(&drawable->tree_item.base.rgn)) {
> - return;
> - }
> -
> - if (!display_channel_handle_self_bitmap(display, drawable)) {
> - return;
> - }
> + if (red_drawable->self_bitmap)
> + handle_self_bitmap(display, drawable);
>
Here it is the check above.
> draw_depend_on_me(display, surface_id);
>
> - if (!red_handle_surfaces_dependencies(display, drawable)) {
> - return;
> - }
> + if (!handle_surface_deps(display, drawable))
> + return FALSE;
>
style, prefer {} always.
> Ring *ring = &display->surfaces[surface_id].current;
> -
> + int add_to_pipe = FALSE;
> if (has_shadow(red_drawable)) {
> - success = current_add_with_shadow(display, ring, drawable);
> + add_to_pipe = current_add_with_shadow(display, ring, drawable);
> } else {
> drawable->streamable = drawable_can_stream(display, drawable);
> - success = current_add(display, ring, drawable);
> + add_to_pipe = current_add(display, ring, drawable);
> }
>
> - if (success)
> + if (add_to_pipe)
> pipes_add_drawable(display, drawable);
>
> #ifdef RED_WORKER_STAT
> if ((++display->add_count % 100) == 0)
> display_channel_print_stats(display);
> #endif
> +
> + return TRUE;
> }
>
> int display_channel_wait_for_migrate_data(DisplayChannel *display)
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 195004d..2ac8173 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -286,8 +286,9 @@ void display_channel_surface_unref
> (DisplayCha
> uint32_t
> surface_id);
> bool display_channel_surface_has_canvas
> (DisplayChannel *display,
> uint32_t
> surface_id);
> -void display_channel_add_drawable
> (DisplayChannel *display,
> -
> Drawable
> *drawable);
> +int display_channel_add_drawable
> (DisplayChannel *display,
> +
> Drawable
> *drawable,
> +
> RedDrawable
> *red_drawable);
> void display_channel_current_flush
> (DisplayChannel *display,
> int
> surface_id);
> int display_channel_wait_for_migrate_data
> (DisplayChannel *display);
> @@ -300,11 +301,6 @@ void
> display_channel_destroy_surfaces (DisplayCha
> void display_channel_destroy_surface
> (DisplayChannel *display,
> uint32_t
> surface_id);
> uint32_t display_channel_generate_uid
> (DisplayChannel *display);
> -Drawable * display_channel_get_drawable
> (DisplayChannel *display,
> -
> uint8_t
> effect,
> -
> RedDrawable
> *red_drawable,
> -
> uint32_t
> group_id,
> -
> uint32_t
> process_commands_generation);
>
> static inline int validate_surface(DisplayChannel *display, uint32_t
> surface_id)
> {
> diff --git a/server/red_worker.c b/server/red_worker.c
> index ee26b63..f943898 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -172,26 +172,28 @@ void red_drawable_unref(RedWorker *worker, RedDrawable
> *red_drawable,
> free(red_drawable);
> }
>
> -static inline void red_process_draw(RedWorker *worker, RedDrawable
> *red_drawable,
> - uint32_t group_id)
> +static void red_process_draw(RedWorker *worker, RedDrawable *red_drawable,
> + uint32_t group_id)
> {
> DisplayChannel *display = worker->display_channel;
> - Drawable *drawable =
> - display_channel_get_drawable(display, red_drawable->effect,
> red_drawable, group_id,
> - worker->process_commands_generation);
> + Drawable *drawable;
> + bool success = FALSE;
>
> + drawable = display_channel_drawable_try_new(display, group_id,
> +
> worker->process_commands_generation);
> if (!drawable) {
> return;
> }
>
> - display_channel_add_drawable(worker->display_channel, drawable);
> + success = display_channel_add_drawable(worker->display_channel,
> drawable, red_drawable);
> + spice_warn_if_fail(success);
>
> display_channel_drawable_unref(display, drawable);
> }
>
>
> -static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd
> *surface,
> - uint32_t group_id, int loadvm)
> +static void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface,
> + uint32_t group_id, int loadvm)
> {
> DisplayChannel *display = worker->display_channel;
> uint32_t surface_id;
Frediano
More information about the Spice-devel
mailing list