[Spice-devel] [RFC v4 39/62] server/red_worker: introduce RedSurfaceReleaseInfo
Alon Levy
alevy at redhat.com
Thu May 5 06:36:35 PDT 2011
On Tue, May 03, 2011 at 01:51:58AM +0200, Marc-André Lureau wrote:
> 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.
Yeah - like I point below, this is meant to improve it :)
>
> > ---
> > 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?
>
One is referencing the device surface, which we put back on the release ring
once we are done with. The other is referencing the internal RedSurface, which
actually I'm not sure if we ever release when refs == 0 but is allocated by us,
not by the guest.
> (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.
>
We render all the non primary (and btw, primary just means "the one that uses the
pci vram for surfaces") clients on host memory, which this variable is used for. (yes,
the logic above is circular. It is also another way of saying there is no difference
between the clients. Actually there is another difference, but not related to this - the
first client gets *all* the channels, both those that support multiple clients and those
that support only one. This is not kept right now - so if that client disconnects while
there was already another client, and the second, now the only, client doesn't disconnect,
and a third comes, it will only get the smaller channel list).
> > } 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.
After reading the explanation below, if you come up with something better please send it.
It needs to not require function pointers (since those limit possible parameters).
>
> 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?
>
Yes - to you first, comment later: We have a single Surfaces (now RedRender after rename)
that is embedded in the RedWorker. The rest are embedded in the DisplayClientChannel that
are held in the client ring. This SURFACES_FOREACH goes over all of them. It uses the fact
that the first client on the ring also references the Surfaces on the worker. So:
0 clients:
return [&worker->surfaces]
1 client:
return [worker->display_channel->clients[0]->surfaces] == [&worker->surfaces]
n clients:
return surfaces=[worker->display_channel->clients[i]->surfaces for i in xrange(num_clients)]
where surfaces[0] == &worker->surfaces
> 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)
I think that is actually much less readable, due to the explanation above - you would then
have to distinguish the zero and non zero client case, at every usage. The SURFACES_FOREACH
hides that.
>
> > #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)
No, will fix.
>
> > + 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)
>
ok. I'll try that -Winline thing. But later patch (writing todo).
> > 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?
>
The primary surface is never freed (not added to the release ring) since the
guest doesn't actuall allocate it - it is the start of the ram bar on the qxl pci
device (we have two bars - the ram holds the primary surface plus all operations,
the vram holds all secondary surfaces).
> > 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