Re: [PATCH 3/3] display: move more logic in add_drawable()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> 
> Acked-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]