Re: [PATCH 01/14] Store display in Drawable struct

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

 



Hi Jonathon,

On Thu, 2016-04-21 at 16:43 -0500, Jonathon Jongsma wrote:
> If the Drawable keeps a pointer to the Display channel that it is
> associated with, we can unref it directly and not need to pass the
> 'display' parameter separately to the unref function

So drawable needs 'display' just for unref'ing surface(s) - detach_stream()
doesn't need DisplayChannel, the same applies for
drawable_remove_dependencies(). Looking at the DisplayChannel struct it still
has an array of Drawables and free_drawables. (It is not clear to me - I need to
read previous reviews)

> ---
> NEW PATCH. This prepares for the UpgradeItem change as discussed on the
> previous review.
> 
>  server/dcc.c             | 14 +++++---------
>  server/display-channel.c | 17 +++++++++++------
>  server/display-channel.h |  4 +++-
>  3 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index 91c3f82..4f3f227 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -277,13 +277,11 @@ static void
> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
>  void drawable_pipe_item_free(PipeItem *item)
>  {
>      DrawablePipeItem *dpi = SPICE_CONTAINEROF(item, DrawablePipeItem,
> dpi_pipe_item);
> -    DisplayChannel *display = DCC_TO_DC(dpi->dcc);
> -
>      spice_assert(item->refcount == 0);
>  
>      spice_warn_if_fail(!ring_item_is_linked(&item->link));
>      spice_warn_if_fail(!ring_item_is_linked(&dpi->base));
> -    display_channel_drawable_unref(display, dpi->drawable);
> +    drawable_unref(dpi->drawable);
>      free(dpi);
>  }
>  
> @@ -1591,20 +1589,18 @@ int dcc_handle_migrate_data(DisplayChannelClient *dcc,
> uint32_t size, void *mess
>      return TRUE;
>  }
>  
> -static void upgrade_item_unref(DisplayChannel *display, UpgradeItem *item)
> +static void upgrade_item_unref(UpgradeItem *item)
>  {
>      if (--item->refs != 0)
>          return;
>  
> -    display_channel_drawable_unref(display, item->drawable);
> +    drawable_unref(item->drawable);
>      free(item->rects);
>      free(item);
>  }
>  
>  static void release_item_after_push(DisplayChannelClient *dcc, PipeItem
> *item)
dcc is unused in the function, it should be removed

The patch makes a sense to me, in my opinion more cleanups should be done.

Pavel

>  {
> -    DisplayChannel *display = DCC_TO_DC(dcc);
> -
>      switch (item->type) {
>      case PIPE_ITEM_TYPE_DRAW:
>      case PIPE_ITEM_TYPE_IMAGE:
> @@ -1613,7 +1609,7 @@ static void release_item_after_push(DisplayChannelClient
> *dcc, PipeItem *item)
>          pipe_item_unref(item);
>          break;
>      case PIPE_ITEM_TYPE_UPGRADE:
> -        upgrade_item_unref(display, (UpgradeItem *)item);
> +        upgrade_item_unref((UpgradeItem *)item);
>          break;
>      case PIPE_ITEM_TYPE_GL_SCANOUT:
>      case PIPE_ITEM_TYPE_GL_DRAW:
> @@ -1651,7 +1647,7 @@ static void
> release_item_before_push(DisplayChannelClient *dcc, PipeItem *item)
>      }
>      case PIPE_ITEM_TYPE_STREAM_CLIP:
>      case PIPE_ITEM_TYPE_UPGRADE:
> -        upgrade_item_unref(display, (UpgradeItem *)item);
> +        upgrade_item_unref((UpgradeItem *)item);
>          break;
>      case PIPE_ITEM_TYPE_IMAGE:
>      case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 88dbc74..f2bdc1d 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -397,7 +397,7 @@ static void current_remove_drawable(DisplayChannel
> *display, Drawable *item)
>      ring_remove(&item->tree_item.base.siblings_link);
>      ring_remove(&item->list_link);
>      ring_remove(&item->surface_list_link);
> -    display_channel_drawable_unref(display, item);
> +    drawable_unref(item);
>      display->current_size--;
>  }
>  
> @@ -495,7 +495,7 @@ static int current_add_equal(DisplayChannel *display,
> DrawItem *item, TreeItem *
>              pipes_add_drawable(display, drawable);
>          }
>          drawable_remove_from_pipes(other_drawable);
> -        display_channel_drawable_unref(display, other_drawable);
> +        drawable_unref(other_drawable);
>          return TRUE;
>      }
>  
> @@ -538,7 +538,7 @@ static int current_add_equal(DisplayChannel *display,
> DrawItem *item, TreeItem *
>              /* not sending other_drawable where possible */
>              drawable_remove_from_pipes(other_drawable);
>  
> -            display_channel_drawable_unref(display, other_drawable);
> +            drawable_unref(other_drawable);
>              return TRUE;
>          }
>          break;
> @@ -1192,7 +1192,7 @@ void display_channel_process_draw(DisplayChannel
> *display, RedDrawable *red_draw
>  
>      display_channel_add_drawable(display, drawable);
>  
> -    display_channel_drawable_unref(display, drawable);
> +    drawable_unref(drawable);
>  }
>  
>  int display_channel_wait_for_migrate_data(DisplayChannel *display)
> @@ -1380,6 +1380,10 @@ Drawable
> *display_channel_drawable_try_new(DisplayChannel *display,
>      }
>  
>      bzero(drawable, sizeof(Drawable));
> +    /* Pointer to the display from which the drawable is allocated.  This
> +     * pointer is safe to be retained as DisplayChannel lifespan is bigger
> than
> +     * all drawables.  */
> +    drawable->display = display;
>      drawable->refs = 1;
>      drawable->creation_time = drawable->first_frame_time =
> spice_get_monotonic_time_ns();
>      ring_item_init(&drawable->list_link);
> @@ -1430,8 +1434,9 @@ static void drawable_unref_surface_deps(DisplayChannel
> *display, Drawable *drawa
>      }
>  }
>  
> -void display_channel_drawable_unref(DisplayChannel *display, Drawable
> *drawable)
> +void drawable_unref(Drawable *drawable)
>  {
> +    DisplayChannel *display = drawable->display;
>      RingItem *item, *next;
>  
>      if (--drawable->refs != 0)
> @@ -1653,7 +1658,7 @@ static void draw_until(DisplayChannel *display,
> RedSurface *surface, Drawable *l
>             that display_channel_draw is called for, Otherwise, 'now' would
> have already been rendered.
>             See the call for red_handle_depends_on_target_surface in
> red_process_draw */
>          drawable_draw(display, now);
> -        display_channel_drawable_unref(display, now);
> +        drawable_unref(now);
>      } while (now != last);
>  }
>  
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 6b053de..c4a3b19 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -79,8 +79,11 @@ struct Drawable {
>      int surface_deps[3];
>  
>      uint32_t process_commands_generation;
> +    DisplayChannel *display;
>  };
>  
> +void drawable_unref (Drawable *drawable);
> +
>  #define LINK_TO_DPI(ptr) SPICE_CONTAINEROF((ptr), DrawablePipeItem, base)
>  #define DRAWABLE_FOREACH_DPI_SAFE(drawable, link, next, dpi)            \
>      SAFE_FOREACH(link, next, drawable,  &(drawable)->pipes, dpi,
> LINK_TO_DPI(link))
> @@ -280,7 +283,6 @@
> void                       display_channel_compress_stats_print      (const
> Disp
>  void                       display_channel_compress_stats_reset      (Display
> Channel *display);
>  Drawable
> *                 display_channel_drawable_try_new          (DisplayChannel
> *display,
>                                                                        int
> process_commands_generation);
> -void                       display_channel_drawable_unref            (Display
> Channel *display, Drawable *drawable);
>  void                       display_channel_surface_unref             (Display
> Channel *display,
>                                                                        uint32_
> t surface_id);
>  bool                       display_channel_surface_has_canvas        (Display
> Channel *display,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://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]