Re: [PATCH server v2 10/13] Handle GL_SCANOUT messages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> > +    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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]