On Tue, Oct 11, 2011 at 02:33:06PM +0200, Marc-André Lureau wrote:
> Hi
>
Thanks for the review.
> On Sun, Oct 9, 2011 at 4:29 PM, Alon Levy <al...@redhat.com> wrote:
> > ---
> > server/red_dispatcher.c | 70
> > +++++++++++++++++++++++++++++++++-------------
> > 1 files changed, 50 insertions(+), 20 deletions(-)
> >
> > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> > index a998238..8289c6b 100644
> > --- a/server/red_dispatcher.c
> > +++ b/server/red_dispatcher.c
> > @@ -40,6 +40,13 @@ static int num_active_workers = 0;
> >
> > //volatile
> >
> > +typedef struct AsyncCommand AsyncCommand;
> > +struct AsyncCommand {
> > + AsyncCommand *next;
> > + RedWorkerMessage message;
> > + uint64_t cookie;
> > +};
> > +
> > struct RedDispatcher {
> > QXLWorker base;
> > QXLInstance *qxl;
> > @@ -51,7 +58,7 @@ struct RedDispatcher {
> > int y_res;
> > int use_hardware_cursor;
> > RedDispatcher *next;
> > - RedWorkerMessage async_message;
> > + AsyncCommand* async_commands;
> > pthread_mutex_t async_lock;
> > QXLDevSurfaceCreate surface_create;
> > };
> > @@ -224,17 +231,27 @@ static void red_dispatcher_update_area(RedDispatcher
> > *dispatcher, uint32_t surfa
> > ASSERT(message == RED_WORKER_MESSAGE_READY);
> > }
> >
> > +static AsyncCommand *push_async_command(RedDispatcher *dispatcher,
> > + RedWorkerMessage message,
> > + uint64_t cookie)
> > +{
> > + AsyncCommand *async_command = spice_new(AsyncCommand, 1);
> > +
> > + async_command->cookie = cookie;
> > + async_command->message = message;
> > + if (dispatcher->async_commands) {
> > + async_command->next = dispatcher->async_commands;
> > + }
>
> The assignment could be unconditionnal.
Right.
> Shouldn't the function take the async_lock instead of the caller?
Right.
>
> > + dispatcher->async_commands = async_command;
Here (answering the last question).
> > + return async_command;
> > +}
> > +
> > static RedWorkerMessage red_dispatcher_async_start(RedDispatcher
> > *dispatcher,
> > - RedWorkerMessage
> > message)
> > + RedWorkerMessage
> > message,
> > + uint64_t cookie)
> > {
> > pthread_mutex_lock(&dispatcher->async_lock);
> > - if (dispatcher->async_message != RED_WORKER_MESSAGE_NOP) {
> > - red_warn("warning: async clash. have %d, ignoring %d",
> > - dispatcher->async_message, message);
> > - pthread_mutex_unlock(&dispatcher->async_lock);
> > - return RED_WORKER_MESSAGE_NOP;
> > - }
> > - dispatcher->async_message = message;
> > + push_async_command(dispatcher, message, cookie);
> > pthread_mutex_unlock(&dispatcher->async_lock);
> > return message;
> > }
> > @@ -246,7 +263,8 @@ static void
> > red_dispatcher_update_area_async(RedDispatcher *dispatcher,
> > uint64_t cookie)
> > {
> > RedWorkerMessage message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_UPDATE_ASYNC);
> > +
> > RED_WORKER_MESSAGE_UPDATE_ASYNC,
> > + cookie);
> >
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > @@ -265,7 +283,7 @@ static void
> > red_dispatcher_update_area_dirty_async(RedDispatcher *dispatcher,
> > uint32_t clear_dirty_region,
> > uint64_t cookie)
> > {
> > RedWorkerMessage message = red_dispatcher_async_start(
> > - dispatcher, RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC);
> > + dispatcher, RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC, cookie);
> >
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > @@ -306,7 +324,8 @@ static void qxl_worker_add_memslot(QXLWorker
> > *qxl_worker, QXLDevMemSlot *mem_slo
> > static void red_dispatcher_add_memslot_async(RedDispatcher *dispatcher,
> > QXLDevMemSlot *mem_slot, uint64_t cookie)
> > {
> > RedWorkerMessage message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC);
> > +
> > RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC,
> > + cookie);
> >
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > @@ -347,7 +366,8 @@ static void qxl_worker_destroy_surfaces(QXLWorker
> > *qxl_worker)
> > static void red_dispatcher_destroy_surfaces_async(RedDispatcher
> > *dispatcher, uint64_t cookie)
> > {
> > RedWorkerMessage message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_DESTROY_SURFACES_ASYNC);
> > +
> > RED_WORKER_MESSAGE_DESTROY_SURFACES_ASYNC,
> > + cookie);
> >
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > @@ -374,7 +394,8 @@ red_dispatcher_destroy_primary_surface(RedDispatcher
> > *dispatcher,
> >
> > if (async) {
> > message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE_ASYNC);
> > +
> > RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE_ASYNC,
> > + cookie);
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > }
> > @@ -420,7 +441,8 @@ red_dispatcher_create_primary_surface(RedDispatcher
> > *dispatcher, uint32_t surfac
> >
> > if (async) {
> > message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC);
> > +
> > RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC,
> > + cookie);
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > }
> > @@ -483,7 +505,8 @@ static void
> > red_dispatcher_destroy_surface_wait(RedDispatcher *dispatcher, uint3
> >
> > if (async ) {
> > message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC);
> > +
> > RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC,
> > + cookie);
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > }
> > @@ -563,7 +586,8 @@ static void qxl_worker_start(QXLWorker *qxl_worker)
> > static void red_dispatcher_flush_surfaces_async(RedDispatcher *dispatcher,
> > uint64_t cookie)
> > {
> > RedWorkerMessage message = red_dispatcher_async_start(dispatcher,
> > -
> > RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC);
> > +
> > RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC,
> > + cookie);
> >
> > if (message == RED_WORKER_MESSAGE_NOP) {
> > return;
> > @@ -836,8 +860,15 @@ void spice_qxl_flush_surfaces_async(QXLInstance
> > *instance, uint64_t cookie)
> >
> > void red_dispatcher_async_complete(struct RedDispatcher *dispatcher,
> > uint64_t cookie)
> > {
> > + AsyncCommand *async_command;
> > pthread_mutex_lock(&dispatcher->async_lock);
> > - switch (dispatcher->async_message) {
> > + for (async_command = dispatcher->async_commands;
> > + async_command && async_command->cookie != cookie &&
> > async_command->next;
>
> I think the "&& async_command->next" condition is unnecessary.
Right.
>
> > + async_command = async_command->next) {};
> > + if (!async_command || async_command->cookie != cookie) {
>
> So you can only check if (!async_command)
Right.
>
> > + red_warn("unknown cookie");
>
> And should return or goto fail to avoid crash later.
Right.
>
> > + }
> > + switch (async_command->message) {
> > case RED_WORKER_MESSAGE_UPDATE_ASYNC:
> > case RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC:
> > break;
> > @@ -858,7 +889,7 @@ void red_dispatcher_async_complete(struct RedDispatcher
> > *dispatcher, uint64_t co
> > default:
> > red_warn("unexpected message");
> > }
> > - dispatcher->async_message = RED_WORKER_MESSAGE_NOP;
> > + free(async_command);
>
> I might be missing something but where is dispatcher->async_commands updated?
>
Answered above.
> > pthread_mutex_unlock(&dispatcher->async_lock);
> > dispatcher->qxl->st->qif->async_complete(dispatcher->qxl, cookie);
> > }
> > @@ -895,7 +926,6 @@ RedDispatcher *red_dispatcher_init(QXLInstance *qxl)
> > init_data.num_renderers = num_renderers;
> > memcpy(init_data.renderers, renderers, sizeof(init_data.renderers));
> >
> > - dispatcher->async_message = RED_WORKER_MESSAGE_NOP;
> > pthread_mutex_init(&dispatcher->async_lock, NULL);
> > init_data.image_compression = image_compression;
> > init_data.jpeg_state = jpeg_state;
> > --
> > 1.7.6.4
> >
> >
>
>
>
> --
> Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
