>
> Hi
>
> ----- Original Message -----
> > >
> > > Go through dispatcher and marshall scanout message. Since the marshaller
> > > and the QXL state are manipulated from different threads, add a mutex to
> > > protect the current scanout.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > > ---
> > > server/dcc-send.c | 25 +++++++++++++++++++++++++
> > > server/dcc.c | 21 +++++++++++++++++++++
> > > server/dcc.h | 6 ++++++
> > > server/display-channel.c | 5 +++++
> > > server/display-channel.h | 2 ++
> > > server/red-channel.h | 2 ++
> > > server/red-dispatcher.c | 8 ++++++++
> > > server/red-dispatcher.h | 1 +
> > > server/red-worker.c | 13 +++++++++++++
> > > server/reds.c | 1 +
> > > server/reds.h | 1 +
> > > 11 files changed, 85 insertions(+)
> > >
> > > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > > index c3f79ef..fd4afa5 100644
> > > --- a/server/dcc-send.c
> > > +++ b/server/dcc-send.c
> > > @@ -2339,6 +2339,28 @@ static void
> > > marshall_stream_activate_report(RedChannelClient *rcc,
> > > spice_marshall_msg_display_stream_activate_report(base_marshaller,
> > > &msg);
> > > }
> > >
> > > +static void marshall_gl_scanout(RedChannelClient *rcc,
> > > + SpiceMarshaller *m,
> > > + PipeItem *item)
> > > +{
> > > + DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> > > + DisplayChannel *display_channel = DCC_TO_DC(dcc);
> > > + RedWorker *worker = display_channel->common.worker;
> > > + QXLInstance* qxl = red_worker_get_qxl(worker);
> > > + SpiceMsgDisplayGlScanoutUnix *so = &qxl->st->scanout;
> > > +
> > > + pthread_mutex_lock(&qxl->st->scanout_mutex);
> > > +
> > > + if (so->drm_dma_buf_fd == -1)
> > > + goto end;
> > > +
> >
> > I would prefer not having the goto if possible.
>
> Ok
>
> > Also I was wondering why we should reach this code if handle is invalid.
> > I think we should handle the error before of this point.
>
> The dmabuf fd could be reset by calling gl_scanout(fd=-1) before the message
> is sent.
>
Ok, this is a requirement that was not clear.
So you say to dispatcher, please send this update but when the update is sent
you check again that everything is still valid.
This means that client can be possibly not that fast to handle changes to scanout.
What happen if server send SCANOUT + DRAW + SCANOUT?
Client can than have all these messages queues up and you risk to send the first scanout
with file descriptor of second one and you do a draw referring to the old one.
Shouldn't you remove then all draw messages after the SCANOUT?
Or perhaps I didn't get the real reason.
> > > + red_channel_client_init_send_data(rcc,
> > > SPICE_MSG_DISPLAY_GL_SCANOUT_UNIX, NULL);
> > > + spice_marshall_msg_display_gl_scanout_unix(m, so);
> > > +
> > > +end:
> > > + pthread_mutex_unlock(&qxl->st->scanout_mutex);
> > > +}
> > > +
> > > static void begin_send_message(RedChannelClient *rcc)
> > > {
> > > DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> > > @@ -2450,6 +2472,9 @@ void dcc_send_item(DisplayChannelClient *dcc,
> > > PipeItem
> > > *pipe_item)
> > > marshall_stream_activate_report(rcc, m, report_item->stream_id);
> > > break;
> > > }
> > > + case PIPE_ITEM_TYPE_GL_SCANOUT:
> > > + marshall_gl_scanout(rcc, m, pipe_item);
> > > + break;
> > > default:
> > > spice_warn_if_reached();
> > > }
> > > diff --git a/server/dcc.c b/server/dcc.c
> > > index 2568e24..3161375 100644
> > > --- a/server/dcc.c
> > > +++ b/server/dcc.c
> > > @@ -556,6 +556,25 @@ static SurfaceDestroyItem
> > > *surface_destroy_item_new(RedChannel *channel,
> > > return destroy;
> > > }
> > >
> > > +PipeItem *dcc_gl_scanout_item_new(RedChannelClient *rcc, void *data, int
> > > num)
> > > +{
> > > + GlScanoutUnixItem *item = spice_new(GlScanoutUnixItem, 1);
> > > + spice_return_val_if_fail(item != NULL, NULL);
> > > +
> > > + /* FIXME: on !unix peer, start streaming with a video codec */
> > > + if (!reds_stream_is_plain_unix(rcc->stream) ||
> > > + !red_channel_client_test_remote_cap(rcc,
> > > SPICE_DISPLAY_CAP_GL_SCANOUT)) {
> > > + spice_printerr("FIXME: client does not support GL scanout");
> > > + red_channel_client_disconnect(rcc);
> > > + return NULL;
> > > + }
> > > +
> > > + red_channel_pipe_item_init(rcc->channel, &item->base,
> > > + PIPE_ITEM_TYPE_GL_SCANOUT);
> > > +
> > > + return PIPE_ITEM(item);
> > > +}
> > > +
> > > void dcc_destroy_surface(DisplayChannelClient *dcc, uint32_t surface_id)
> > > {
> > > DisplayChannel *display;
> > > @@ -1524,6 +1543,7 @@ static void
> > > release_item_after_push(DisplayChannelClient *dcc, PipeItem *item)
> > > case PIPE_ITEM_TYPE_IMAGE:
> > > image_item_unref((ImageItem *)item);
> > > break;
> > > + case PIPE_ITEM_TYPE_GL_SCANOUT:
> > > case PIPE_ITEM_TYPE_VERB:
> > > free(item);
> > > break;
> > > @@ -1598,6 +1618,7 @@ static void
> > > release_item_before_push(DisplayChannelClient *dcc, PipeItem *item)
> > > case PIPE_ITEM_TYPE_PIXMAP_RESET:
> > > case PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE:
> > > case PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT:
> > > + case PIPE_ITEM_TYPE_GL_SCANOUT:
> > > free(item);
> > > break;
> > > default:
> > > diff --git a/server/dcc.h b/server/dcc.h
> > > index 2a12226..22d236e 100644
> > > --- a/server/dcc.h
> > > +++ b/server/dcc.h
> > > @@ -124,6 +124,10 @@ typedef struct SurfaceCreateItem {
> > > PipeItem pipe_item;
> > > } SurfaceCreateItem;
> > >
> > > +typedef struct GlScanoutUnixItem {
> > > + PipeItem base;
> > > +} GlScanoutUnixItem;
> > > +
> > > typedef struct ImageItem {
> > > PipeItem link;
> > > int refs;
> > > @@ -207,6 +211,8 @@ int
> > > dcc_clear_surface_drawables_from_pipe (DisplayCha
> > > int
> > > wait_if_used);
> > > int dcc_drawable_is_in_pipe
> > > (DisplayChannelClient *dcc,
> > > Drawable
> > > *drawable);
> > > +PipeItem * dcc_gl_scanout_item_new
> > > (RedChannelClient *rcc,
> > > +
> > > void
> > > *data, int num);
> > >
> > > typedef struct compress_send_data_t {
> > > void* comp_buf;
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 3bf065c..43fddff 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -2149,3 +2149,8 @@ void
> > > display_channel_update_compression(DisplayChannel
> > > *display, DisplayChannelC
> > > spice_info("jpeg %s", display->enable_jpeg ? "enabled" :
> > > "disabled");
> > > spice_info("zlib-over-glz %s", display->enable_zlib_glz_wrap ?
> > > "enabled"
> > > : "disabled");
> > > }
> > > +
> > > +void display_channel_gl_scanout(DisplayChannel *display)
> > > +{
> > > + red_channel_pipes_new_add_push(RED_CHANNEL(display),
> > > dcc_gl_scanout_item_new, NULL);
> > > +}
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index bf29cd3..346e61a 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -106,6 +106,7 @@ enum {
> > > PIPE_ITEM_TYPE_DESTROY_SURFACE,
> > > PIPE_ITEM_TYPE_MONITORS_CONFIG,
> > > PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> > > + PIPE_ITEM_TYPE_GL_SCANOUT,
> > > };
> > >
> > > typedef struct MonitorsConfig {
> > > @@ -306,6 +307,7 @@ void
> > > display_channel_process_surface_cmd (DisplayCha
> > > int
> > > loadvm);
> > > void display_channel_update_compression
> > > (DisplayChannel *display,
> > > DisplayChannelClient
> > > *dcc);
> > > +void display_channel_gl_scanout
> > > (DisplayChannel *display);
> > >
> > > static inline int validate_surface(DisplayChannel *display, uint32_t
> > > surface_id)
> > > {
> > > diff --git a/server/red-channel.h b/server/red-channel.h
> > > index fbb93b7..3e51b3a 100644
> > > --- a/server/red-channel.h
> > > +++ b/server/red-channel.h
> > > @@ -157,6 +157,8 @@ static inline int pipe_item_is_linked(PipeItem *item)
> > > return ring_item_is_linked(&item->link);
> > > }
> > >
> > > +#define PIPE_ITEM(item) ((PipeItem*)(item))
> > > +
> >
> > Personally I hate this kind of warning killer.
> > I think can be removed easily.
>
> Fine (I prefer it obviously, especially once we have gobject..)
>
That's one reason I don't like gobject, everything is everything else and
compiler does not complain much so is all up to the developer and tester.
> >
> > > typedef uint8_t *(*channel_alloc_msg_recv_buf_proc)(RedChannelClient
> > > *channel,
> > > uint16_t type,
> > > uint32_t
> > > size);
> > > typedef int (*channel_handle_parsed_proc)(RedChannelClient *rcc,
> > > uint32_t
> > > size, uint16_t type,
> > > diff --git a/server/red-dispatcher.c b/server/red-dispatcher.c
> > > index 7fe74b6..3c173c8 100644
> > > --- a/server/red-dispatcher.c
> > > +++ b/server/red-dispatcher.c
> > > @@ -968,6 +968,8 @@ void spice_gl_scanout(QXLInstance *qxl,
> > > {
> > > spice_return_if_fail(qxl != NULL);
> > >
> > > + pthread_mutex_lock(&qxl->st->scanout_mutex);
> > > +
> > > if (qxl->st->scanout.drm_dma_buf_fd != -1) {
> > > close(qxl->st->scanout.drm_dma_buf_fd);
> > > }
> > > @@ -980,6 +982,12 @@ void spice_gl_scanout(QXLInstance *qxl,
> > > .stride = stride,
> > > .drm_fourcc_format = format
> > > };
> > > +
> > > + pthread_mutex_unlock(&qxl->st->scanout_mutex);
> > > +
> > > + /* FIXME: find a way to coallesce all pending SCANOUTs */
> > > + dispatcher_send_message(&qxl->st->dispatcher->dispatcher,
> > > + RED_WORKER_MESSAGE_GL_SCANOUT, NULL);
> > > }
> > >
> > > SPICE_GNUC_VISIBLE
> > > diff --git a/server/red-dispatcher.h b/server/red-dispatcher.h
> > > index d99695d..cc60c10 100644
> > > --- a/server/red-dispatcher.h
> > > +++ b/server/red-dispatcher.h
> > > @@ -88,6 +88,7 @@ enum {
> > >
> > > RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC,
> > > RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> > > + RED_WORKER_MESSAGE_GL_SCANOUT,
> > >
> > > RED_WORKER_MESSAGE_COUNT // LAST
> > > };
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index 7d3cce3..dded97a 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -1283,6 +1283,14 @@ static void handle_dev_driver_unload(void *opaque,
> > > void *payload)
> > > worker->driver_cap_monitors_config = 0;
> > > }
> > >
> > > +static
> > > +void handle_dev_gl_scanout(void *opaque, void *payload)
> > > +{
> > > + RedWorker *worker = opaque;
> > > +
> > > + display_channel_gl_scanout(worker->display_channel);
> > > +}
> > > +
> > > static int loadvm_command(RedWorker *worker, QXLCommandExt *ext)
> > > {
> > > RedCursorCmd *cursor_cmd;
> > > @@ -1520,6 +1528,11 @@ static void register_callbacks(Dispatcher
> > > *dispatcher)
> > > handle_dev_driver_unload,
> > > sizeof(RedWorkerMessageDriverUnload),
> > > DISPATCHER_NONE);
> > > + dispatcher_register_handler(dispatcher,
> > > + RED_WORKER_MESSAGE_GL_SCANOUT,
> > > + handle_dev_gl_scanout,
> > > + 0,
> > > + DISPATCHER_NONE);
> > > }
> > >
> > >
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 09540ad..664fc5a 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -3226,6 +3226,7 @@ SPICE_GNUC_VISIBLE int
> > > spice_server_add_interface(SpiceServer *s,
> > >
> > > qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> > > qxl->st = spice_new0(QXLState, 1);
> > > + pthread_mutex_init(&qxl->st->scanout_mutex, NULL);
> >
> > Mutex... that means this is accessed by multiple thread.
> > This change a bit the way worker works by default.
> > The reason is that spice_gl_scanout is called directly by Qemu and
> > the change is done in that thread.
> > However in this patch you also ask dispatcher to send the message.
> > I think would be much better to marshall the entire data (very few
> > actually)
>
> Marshalling a fd? it's a receipe for leaks. I think we marshall pointers
> today, I find it very wrong.
>
You can dup for this. It's weird marshalling pointers but if you think is
the same process make sense.
> > and send to dispatcher directly.
> > This will avoid the mutex and keep code more consistent.
>
> The mutex is useful if the caller want to change / take away the fd before it
> is sent. This is useful to have up to date image sent to the client.
> Obviously, this changed a little bit meanwhile, because qemu now waits for
> the client before continuing rendering. But it could still change this
> behaviour if it has a more recent complete renderning (after resize for
> example). This is the right approach: keep up to date data in main thread,
> while accessing it from worker thread. Much more obvious and less error
> prone.
>
See above about queuing and possible problems.
> > > qxl->st->scanout.drm_dma_buf_fd = -1;
> > > qxl->st->qif = SPICE_CONTAINEROF(interface, QXLInterface, base);
> > > red_dispatcher_init(qxl);
> > > diff --git a/server/reds.h b/server/reds.h
> > > index d0f95da..b90a38b 100644
> > > --- a/server/reds.h
> > > +++ b/server/reds.h
> > > @@ -33,6 +33,7 @@
> > > struct QXLState {
> > > QXLInterface *qif;
> > > struct RedDispatcher *dispatcher;
> > > + pthread_mutex_t scanout_mutex;
> > > SpiceMsgDisplayGlScanoutUnix scanout;
> > > struct AsyncCommand *gl_draw_async;
> > > };
> >
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel