[Spice-devel] [PATCH server v2 09/13] Add new spice-gl stubs API

[Frediano Ziglio]

> 
> Hi
> 
> ----- Original Message -----
> > > 
> > > - spice_gl_scanout() to take the current scanout
> > > 
> > > - spice_gl_draw_async() to draw the scanout, is like other Spice async
> > >   functions, it takes a cookie and will return in the
> > >   QXLInterface.async_complete()
> > > 
> > > Two new fields are also added to QXLState, in order to save the current
> > > scanout, and the pending async.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > ---
> > >  server/red-dispatcher.c  | 34 ++++++++++++++++++++++++++++++++++
> > >  server/reds.c            |  1 +
> > >  server/reds.h            |  2 ++
> > >  server/spice-qxl.h       | 11 +++++++++++
> > >  server/spice-server.syms |  6 ++++++
> > >  5 files changed, 54 insertions(+)
> > > 
> > > diff --git a/server/red-dispatcher.c b/server/red-dispatcher.c
> > > index 0810798..7fe74b6 100644
> > > --- a/server/red-dispatcher.c
> > > +++ b/server/red-dispatcher.c
> > > @@ -959,6 +959,40 @@ void spice_qxl_driver_unload(QXLInstance *instance)
> > >      red_dispatcher_driver_unload(instance->st->dispatcher);
> > >  }
> > >  
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_gl_scanout(QXLInstance *qxl,
> > > +                      int fd,
> > > +                      uint32_t width, uint32_t height,
> > > +                      uint32_t stride, uint32_t format,
> > > +                      int y_0_top)
> > > +{
> > > +    spice_return_if_fail(qxl != NULL);
> > > +
> > > +    if (qxl->st->scanout.drm_dma_buf_fd != -1) {
> > > +        close(qxl->st->scanout.drm_dma_buf_fd);
> > > +    }
> > > +
> > > +    qxl->st->scanout = (SpiceMsgDisplayGlScanoutUnix) {
> > > +        .flags = y_0_top ? SPICE_GL_SCANOUT_FLAGS_Y0TOP : 0,
> > > +        .drm_dma_buf_fd = fd,
> > > +        .width = width,
> > > +        .height = height,
> > > +        .stride = stride,
> > > +        .drm_fourcc_format = format
> > > +    };
> > > +}
> > > +
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_gl_draw_async(QXLInstance *qxl,
> > > +                         uint32_t x, uint32_t y,
> > > +                         uint32_t w, uint32_t h,
> > > +                         uint64_t cookie)
> > > +{
> > > +    spice_return_if_fail(qxl != NULL);
> > > +    spice_return_if_fail(qxl->st->scanout.drm_dma_buf_fd != -1);
> > > +    spice_return_if_fail(qxl->st->gl_draw_async == NULL);
> > > +}
> > > +
> > >  void red_dispatcher_async_complete(struct RedDispatcher *dispatcher,
> > >                                     AsyncCommand *async_command)
> > >  {
> > > diff --git a/server/reds.c b/server/reds.c
> > > index bee2574..09540ad 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);
> > > +        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 f3a9ce4..d0f95da 100644
> > > --- a/server/reds.h
> > > +++ b/server/reds.h
> > > @@ -33,6 +33,8 @@
> > >  struct QXLState {
> > >      QXLInterface          *qif;
> > >      struct RedDispatcher  *dispatcher;
> > > +    SpiceMsgDisplayGlScanoutUnix scanout;
> > > +    struct AsyncCommand *gl_draw_async;
> > >  };
> > >  
> > 
> > This patch make sense. However I don't understand why there are so few
> > fields
> > here.
> 
> Well, I would say it's better that way :)
> 

Me too. I mean, the state is quite big but here there are few fields so I
just would like to see other opinions (from people with more year experience
on this code) on why is that so.

> > Looks like the real state is stored somewhere under dispatcher.
> 
> No, it's all in QXLState.
> 

Yes, pointed by RedDispatcher.

> > I'm not saying this hunk is wrong, just that it does not sound right.
> > I would like to have some other opinion on this.
> > 
> > >  struct TunnelWorker;
> > > diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> > > index e1f14e7..466b95c 100644
> > > --- a/server/spice-qxl.h
> > > +++ b/server/spice-qxl.h
> > > @@ -193,4 +193,15 @@ struct QXLInstance {
> > >      QXLState           *st;
> > >  };
> > >  
> > > +void spice_gl_scanout(QXLInstance *instance,
> > > +                      int fd,
> > > +                      uint32_t width, uint32_t height,
> > > +                      uint32_t stride, uint32_t format,
> > > +                      int y_0_top);
> > > +
> > > +void spice_gl_draw_async(QXLInstance *instance,
> > > +                         uint32_t x, uint32_t y,
> > > +                         uint32_t w, uint32_t h,
> > > +                         uint64_t cookie);
> > > +
> > >  #endif /* SPICE_QXL_H_ */
> > > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > > index 4137546..18cf1b4 100644
> > > --- a/server/spice-server.syms
> > > +++ b/server/spice-server.syms
> > > @@ -167,3 +167,9 @@ SPICE_SERVER_0.12.7 {
> > >  global:
> > >      spice_server_set_keepalive_timeout;
> > >  } SPICE_SERVER_0.12.6;
> > > +
> > > +SPICE_SERVER_0.13 {
> > > +global:
> > > +    spice_gl_scanout;
> > > +    spice_gl_draw_async;
> > > +} SPICE_SERVER_0.12.7;
> > 
> > I think this should be bumped.
> > 
> 
> should / should not?
> 

0.13.0 was released some time ago. Perhaps you should use 0.13.1.
But I'm not sure about the versioning schema on development releases.

Frediano