>
> 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
Yes, the display is useful to make possible to free Drawable just having
a pointer to it.
Yes, some function have additional parameters not used but they could be useful
in the future. Could be the Stream does not have a pointer to Drawable to
get the DisplayChannel. Looks like not really related to this patch.
There could be more cleanup possibly following this patch.
> 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)
You are confusing _Drawable with Drawable, _Drawable(s) are used for
memory pooling of Drawable(s).
>
> > ---
> > 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,
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel