[Spice-devel] [PATCH server v2 10/13] Handle GL_SCANOUT messages
Marc-André Lureau
mlureau at redhat.com
Fri Jan 22 06:08:31 PST 2016
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 at gmail.com>
> > ---
> > 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.
> > + 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..)
>
> > 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.
> 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.
> > 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list