Hi
----- Original Message -----
> >
> > 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.
Yes, that's a valid concern. In practice, nothing terribly wrong happens since opengl accepts any texture coordinates, and the rendering depends on various parameters (wrap, clamp etc)
I think I can address this with a serial/nframes number to discard updates. I'll take a look.
>
> > > > + 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.
I haven't seen that pattern anywhere else. I don't think we handle shuting down the server anyway, and it probably leaks a lot already. But for newly written code, I'd rather not do this again and instead keep resource references in better controlled memory.
> > > 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
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel