[Spice-devel] [RFC v4 39/62] server/red_worker: introduce RedSurfaceReleaseInfo
Marc-André Lureau
marcandre.lureau at gmail.com
Mon May 2 16:51:58 PDT 2011
On Tue, Apr 26, 2011 at 12:55 PM, Alon Levy <alevy at redhat.com> wrote:
> with multiple clients any surface creation or deletion cleanup needs to
> wait until all the clients have been sent the command, so add reference
> counting to the create and destroy release calls.
>
> Also introduces the SURFACES_FOREACH macro, a bit ugly due to the need
> to update multiple variables and trying to do it without introducing any
> function, so the macro can be used as a for loop head.
Ok, but readibility matters.
> ---
> server/red_worker.c | 201 ++++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 149 insertions(+), 52 deletions(-)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 388509a..1e4d70e 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -805,6 +805,11 @@ typedef struct DrawContext {
> void *line_0;
> } DrawContext;
>
> +typedef struct RedSurfaceReleaseInfo {
> + int refs;
> + QXLReleaseInfoExt create, destroy;
> +} RedSurfaceReleaseInfo;
> +
Why do we need a seperate structure?
(I wish we would have a base ref/type class again)
> typedef struct RedSurface {
> uint32_t refs;
So we have uint32_t refs here,
> Ring current;
> @@ -817,8 +822,8 @@ typedef struct RedSurface {
> Ring depend_on_me;
> QRegion draw_dirty_region;
>
> - //fix me - better handling here
> - QXLReleaseInfoExt create, destroy;
> + RedSurfaceReleaseInfo *release_info;
and int release_info->refs here. /me wonders
> + void *backend;
What is backend for? It doesn't seem used in this patch.
> } RedSurface;
>
> typedef struct ItemTrace {
> @@ -990,11 +995,29 @@ static void red_wait_pipe_item_sent(RedChannelClient *rcc, PipeItem *item);
> static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_id);
> #endif
>
> +static RingItem *red_worker_get_clients_tail(RedWorker *worker)
> +{
> + if (!worker->display_channel) {
> + return NULL;
> + }
> + return ring_get_tail(&(worker)->display_channel->common.base.clients);
> +}
> +
> /*
> * Macros to make iterating over stuff easier
> * The two collections we iterate over:
> * given a channel, iterate over it's clients
> */
> +#define SURFACES_FOREACH(var, surfs, worker) \
> + for ((var) = red_worker_get_clients_tail(worker), \
> + (surfs) = (!(var) ? &(worker)->surfaces : \
> + SPICE_CONTAINEROF((var), CommonChannelClient, \
> + base.channel_link)->surfaces); \
> + (var) || (surfs); \
> + (var) = (var) ? ring_prev( \
> + &(worker)->display_channel->common.base.clients, (var)) : NULL, \
> + (surfs) = (var) ? SPICE_CONTAINEROF((var), CommonChannelClient, \
> + base.channel_link)->surfaces : NULL)
This looks obfuscated to me.
So we iterate in reverse order the clients and take the
client->surfaces, ... eh I am lost. Care to explain better in your
comment above?
Can't we break this, and name it more sensibly? (ex, SURFACES_FOREACH
should take a ring of surfaces and iterate, not a worker and do
complex iteration, so a more descriptive name would help)
> #define RCC_FOREACH(link, rcc, channel) \
> for (link = ring_get_head(&(channel)->clients),\
> @@ -1474,6 +1497,26 @@ static inline void red_destroy_surface_item(RedWorker *worker,
> red_channel_client_pipe_add(&dcc->common.base, &destroy->pipe_item);
> }
>
> +static inline void red_release_surface(RedWorker *worker, RedSurface *surface)
> +{
> + RedSurfaceReleaseInfo *release_info = surface->release_info;
> +
> + if (surface->backend) {
> + free(surface->backend);
> + }
Any reason not to assign NULL (since surface is not freed later on)
> + if (!release_info || --release_info->refs) {
> + return;
> + }
> + if (release_info->create.info) {
> + worker->qxl->st->qif->release_resource(worker->qxl, release_info->create);
> + }
> + if (release_info->destroy.info) {
> + worker->qxl->st->qif->release_resource(worker->qxl, release_info->destroy);
> + }
> + free(release_info);
> + surface->release_info = NULL;
> +}
> +
> static inline void red_destroy_surface(RedWorker *worker, Surfaces *surfaces,
> uint32_t surface_id)
> {
> @@ -1487,12 +1530,7 @@ static inline void red_destroy_surface(RedWorker *worker, Surfaces *surfaces,
> ASSERT(surface->context.canvas);
>
> surface->context.canvas->ops->destroy(surface->context.canvas);
> - if (surface->create.info) {
> - worker->qxl->st->qif->release_resource(worker->qxl, surface->create);
> - }
> - if (surface->destroy.info) {
> - worker->qxl->st->qif->release_resource(worker->qxl, surface->destroy);
> - }
> + red_release_surface(worker, surface);
>
> region_destroy(&surface->draw_dirty_region);
> surface->context.canvas = NULL;
> @@ -1502,19 +1540,16 @@ static inline void red_destroy_surface(RedWorker *worker, Surfaces *surfaces,
> }
> }
>
> -static inline void set_surface_release_info(Surfaces *surfaces, uint32_t surface_id, int is_create,
> - QXLReleaseInfo *release_info, uint32_t group_id)
> +static inline void set_surface_release_info(RedSurfaceReleaseInfo *surface_release_info,
> + int is_create, QXLReleaseInfo *release_info, uint32_t group_id)
> {
> - RedSurface *surface;
> -
> - surface = &surfaces->surfaces[surface_id];
> -
> + ASSERT(release_info);
> if (is_create) {
> - surface->create.info = release_info;
> - surface->create.group_id = group_id;
> + surface_release_info->create.info = release_info;
> + surface_release_info->create.group_id = group_id;
> } else {
> - surface->destroy.info = release_info;
> - surface->destroy.group_id = group_id;
> + surface_release_info->destroy.info = release_info;
> + surface_release_info->destroy.group_id = group_id;
> }
> }
>
> @@ -3565,45 +3600,102 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *drawable
> static inline void red_create_surface(RedWorker *worker, Surfaces *surfaces,
> uint32_t surface_id, uint32_t width,
> uint32_t height, int32_t stride, uint32_t format,
> - void *line_0, int data_is_valid);
> + void *line_0, int data_is_valid,
> + RedSurfaceReleaseInfo *release_info);
>
> -static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface, uint32_t group_id, int data_is_valid)
> +static RedSurfaceReleaseInfo *red_create_surface_release_info(
> + int is_create, QXLReleaseInfo *qxl_release_info, uint32_t group_id)
> {
> - int surface_id;
> - RedSurface *red_surface;
> - uint8_t *data;
> - Surfaces *surfaces = &worker->surfaces;
> + RedSurfaceReleaseInfo *release_info =
> + spice_malloc0(sizeof(RedSurfaceReleaseInfo));
>
> - surface_id = surface->surface_id;
> - __validate_surface(surfaces, surface_id);
> + if (qxl_release_info) {
> + set_surface_release_info(release_info, is_create,
> + qxl_release_info, group_id);
> + }
> + return release_info;
> +}
>
> - red_surface = &surfaces->surfaces[surface_id];
> +static inline void red_create_surface_for_all_clients(RedWorker *worker,
> + uint32_t surface_id, uint32_t width, uint32_t height, int32_t stride,
> + uint32_t format, void *line_0, int data_is_valid,
> + QXLReleaseInfo *qxl_release_info, uint32_t group_id)
> +{
> + RingItem *link;
> + Surfaces *surfaces;
> + RedSurfaceReleaseInfo *release_info =
> + qxl_release_info == NULL ? NULL :
> + red_create_surface_release_info(1, qxl_release_info, group_id);
>
> - switch (surface->type) {
> - case QXL_SURFACE_CMD_CREATE: {
> - uint32_t height = surface->u.surface_create.height;
> - int32_t stride = surface->u.surface_create.stride;
> + SURFACES_FOREACH(link, surfaces, worker) {
> + red_create_surface(worker, surfaces, surface_id,
> + width, height, stride, format, line_0,
> + data_is_valid, release_info);
> + }
> +}
>
> - data = surface->u.surface_create.data;
> - if (stride < 0) {
> - data -= (int32_t)(stride * (height - 1));
> - }
> - red_create_surface(worker, surfaces, surface_id, surface->u.surface_create.width,
> - height, stride, surface->u.surface_create.format, data,
> - data_is_valid);
> - set_surface_release_info(&worker->surfaces, surface_id, 1, surface->release_info, group_id);
> - break;
> +static inline void red_create_surface_from_cmd_for_all_clients(RedWorker *worker,
> + RedSurfaceCmd *surface, uint32_t group_id, int data_is_valid)
> +{
> + uint8_t *line_0;
> + uint32_t height = surface->u.surface_create.height;
> + int32_t stride = surface->u.surface_create.stride;
> +
> + line_0 = surface->u.surface_create.data;
> + if (stride < 0) {
> + line_0 -= (int32_t)(stride * (height - 1));
> }
> - case QXL_SURFACE_CMD_DESTROY:
> - PANIC_ON(!red_surface->context.canvas);
> - set_surface_release_info(surfaces, surface_id, 0, surface->release_info, group_id);
> + return red_create_surface_for_all_clients(worker, surface->surface_id,
> + surface->u.surface_create.width, height, stride, surface->u.surface_create.format,
> + line_0, data_is_valid, surface->release_info, group_id);
> +}
> +
> +static inline void red_destroy_surface_for_all_clients(RedWorker *worker,
> + RedSurfaceCmd *surface, uint32_t group_id, int data_is_valid)
> +{
> + int surface_id;
> + RingItem *link;
> + Surfaces *surfaces;
> + ASSERT(surface->release_info); // we assume primary doesn't reach here.
> +
> + surface_id = surface->surface_id;
> + SURFACES_FOREACH(link, surfaces, worker) {
> + PANIC_ON(!surfaces->surfaces[surface_id].context.canvas);
> + // this is actually redundant - but since it is possible for the
> + // surfaces to be freed in any order (TODO: is it really? could all of
> + // this be much simpler then I'm making it up to be?) just set the
> + // release info for all
> + set_surface_release_info(surfaces->surfaces[surface_id].release_info,
> + 0, surface->release_info, group_id);
> red_handle_depends_on_target_surface(worker, surfaces, surface_id);
> - /* note that red_handle_depends_on_target_surface must be called before red_current_clear.
> - otherwise "current" will hold items that other drawables may depend on, and then
> - red_current_clear will remove them from the pipe. */
> + /* note that red_handle_depends_on_target_surface must be called before
> + * red_current_clear. otherwise "current" will hold items that other
> + * drawables may depend on, and then red_current_clear will remove them
> + * from the pipe. */
> red_current_clear(worker, surfaces, surface_id);
> - red_clear_surface_drawables_from_pipe(surfaces->dcc, surface_id, FALSE);
> + if (surfaces->dcc) {
> + red_clear_surface_drawables_from_pipe(surfaces->dcc,
> + surface_id, FALSE);
> + }
> red_destroy_surface(worker, surfaces, surface_id);
> + }
> +}
> +
> +static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface, uint32_t group_id, int data_is_valid)
> +{
> + int surface_id;
> +
> + surface_id = surface->surface_id;
> + __validate_surface(&worker->surfaces, surface_id);
> +
> + switch (surface->type) {
> + case QXL_SURFACE_CMD_CREATE:
> + red_create_surface_from_cmd_for_all_clients(worker, surface, group_id,
> + data_is_valid);
> + break;
> + case QXL_SURFACE_CMD_DESTROY:
> + red_destroy_surface_for_all_clients(worker, surface, group_id,
> + data_is_valid);
> break;
> default:
> red_error("unknown surface command");
> @@ -8675,7 +8767,8 @@ static inline void red_create_surface_item(RedWorker *worker,
> static inline void red_create_surface(RedWorker *worker, Surfaces *surfaces,
Note: we have a lot of rather big inline. I would remove them, and let
the compiler do the right choice (and also, check with -Winline if it
really happens)
> uint32_t surface_id, uint32_t width,
> uint32_t height, int32_t stride, uint32_t format,
> - void *line_0, int data_is_valid)
> + void *line_0, int data_is_valid,
> + RedSurfaceReleaseInfo *release_info)
> {
> uint32_t i;
> RedSurface *surface = &surfaces->surfaces[surface_id];
> @@ -8684,8 +8777,13 @@ static inline void red_create_surface(RedWorker *worker, Surfaces *surfaces,
> if (stride >= 0) {
> PANIC("Untested path stride >= 0");
> }
> + surface->backend = NULL;
> PANIC_ON(surface->context.canvas);
>
> + if (release_info) {
> + release_info->refs++;
> + }
> + surface->release_info = release_info;
> surface->context.canvas_draws_on_surface = FALSE;
> surface->context.width = width;
> surface->context.height = height;
> @@ -8695,8 +8793,6 @@ static inline void red_create_surface(RedWorker *worker, Surfaces *surfaces,
> if (!data_is_valid) {
> memset(line_0 + (int32_t)(stride * (height - 1)), 0, height*abs(stride));
> }
> - surface->create.info = NULL;
> - surface->destroy.info = NULL;
> ring_init(&surface->current);
> ring_init(&surface->current_list);
> ring_init(&surface->depend_on_me);
> @@ -10050,9 +10146,10 @@ static inline void handle_dev_create_primary_surface(RedWorker *worker)
> line_0 -= (int32_t)(surface.stride * (surface.height -1));
> }
>
> - red_create_surface(worker, &worker->surfaces, 0, surface.width,
> + red_create_surface_for_all_clients(worker, 0, surface.width,
> surface.height, surface.stride, surface.format,
> - line_0, surface.flags & QXL_SURF_FLAG_KEEP_DATA);
> + line_0, surface.flags & QXL_SURF_FLAG_KEEP_DATA,
> + NULL /* release_info */, surface.group_id);
why a NULL release_info here?
> if (display_connected(worker)) {
> red_pipes_add_verb(&worker->display_channel->common.base,
> --
> 1.7.4.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
--
Marc-André Lureau
More information about the Spice-devel
mailing list