[Spice-devel] [PATCH server v2 10/13] Handle GL_SCANOUT messages
Frediano Ziglio
fziglio at redhat.com
Fri Jan 22 05:17:20 PST 2016
>
> 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.
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.
> + 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.
> 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)
and send to dispatcher directly.
This will avoid the mutex and keep code more consistent.
> 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
More information about the Spice-devel
mailing list