Re: [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode

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

 



Hi Frediano,

> 
> Il giorno ven 13 gen 2023 alle ore 04:08 Kasireddy, Vivek
> <vivek.kasireddy@xxxxxxxxx> ha scritto:
> >
> > Hi Frediano,
> >
> > >
> > > Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
> > > <vivek.kasireddy@xxxxxxxxx> ha scritto:
> > > >
> > > > Hi Frediano,
> > > >
> > > > >
> > > > > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > > > > <vivek.kasireddy@xxxxxxxxx> ha scritto:
> > > > > >
> > > > > > This async is triggered by the encoder indicating that the
> > > > > > encoding process is completed.
> > > > > >
> > > > >
> > > > > This description does not make much sense to me.
> > > > > You are adding a public function which, I suppose, should be called by
> > > > > Qemu but you are stating the encoder is calling it.
> > > > > Unless Qemu is the encoder it does not make sense.
> > > > [Kasireddy, Vivek] Sorry for the poor, misleading description. I originally
> > > > had this patch split into two, one for each async function and forgot to
> > > > update the commit message when I merged them. I'll update the
> > > > commit message in v2 which would probably have the following desc:
> > > > "This patch mainly adds two async functions: a public one and an
> > > > internal one. The public function spice_qxl_dmabuf_encode_async is
> > > > used by applications that would share their fd with the Spice server.
> > >
> > > I don't see a reason for the new API, it's just doing a combination of
> > > spice_qxl_gl_scanout + spice_qxl_gl_draw_async.
> > [Kasireddy, Vivek] I had considered reusing these functions for my use-case
> > but decided against cluttering them. Anyway, do you think it is ok to add a
> > new parameter or a flag to these public APIs to prevent them from doing
> > qxl_state->send_message() because my use-case does not involve sending
> > any messages (neither RedWorkerMessageGlDraw nor
> > RedWorkerMessageGlScanout) to the client.
> >
> 
> On the other end you are cluttering public API, Qemu and user usage.
> To use local connection you have to setup the VM in a different way
> than remote connection so you would have either to ask the
> administrator to reconfigure and restart the VM or having a possible
> suboptimal (or not working) connection. Doing inside spice-server
> won't require VM setup change (because spice-server can choose the way
> of handling it).
> About qxl_state->send_message() you need to do it, it has nothing to
> do with client handling but has to do with the way spice-server
> handles threading. If you don't do it you will create thread problems.
[Kasireddy, Vivek] It appears sending these messages is resulting in 
gl_scanout and gl_draw items getting added to the pipe leading to the
error message and client disconnection in the following function:

RedPipeItemPtr dcc_gl_scanout_item_new(RedChannelClient *rcc, void *data, int num)
{
    /* FIXME: on !unix peer, start streaming with a video codec */
    if (!red_stream_is_plain_unix(rcc->get_stream()) ||
        !rcc->test_remote_cap(SPICE_DISPLAY_CAP_GL_SCANOUT)) {
        red_channel_warning(rcc->get_channel(),
                            "FIXME: client does not support GL scanout");
        rcc->disconnect();
        return RedPipeItemPtr();
    }

Thanks,
Vivek

> There's a "spice_threading_model.txt" document inside the "docs"
> directory. Basically it's a message for the DisplayChannel thread to
> do something.
> 
> > >
> > > > The internal function red_qxl_dmabuf_encode_async_complete is only
> > > > used by the Spice server to indicate completion of the encoding process
> > > > and send the completion cookie to applications."
> > > >
> > >
> > > That's what red_qxl_gl_draw_async_complete is doing.
> > [Kasireddy, Vivek] Yeah, this one, I can totally reuse.
> >
> > >
> > > > >
> > > > > > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > > > > Cc: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > > > > > Cc: Dongwon Kim <dongwon.kim@xxxxxxxxx>
> > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > > > > > ---
> > > > > >  server/display-channel.cpp |  9 +++++++++
> > > > > >  server/display-channel.h   |  2 ++
> > > > > >  server/red-qxl.cpp         | 26 ++++++++++++++++++++++++++
> > > > > >  server/red-qxl.h           |  1 +
> > > > > >  server/spice-qxl.h         |  2 ++
> > > > > >  server/spice-server.syms   |  1 +
> > > > > >  6 files changed, 41 insertions(+)
> > > > > >
> > > > > > diff --git a/server/display-channel.cpp b/server/display-channel.cpp
> > > > > > index 4bd0cf41..81df5420 100644
> > > > > > --- a/server/display-channel.cpp
> > > > > > +++ b/server/display-channel.cpp
> > > > > > @@ -2334,6 +2334,15 @@ void
> > > > > display_channel_gl_draw_done(DisplayChannel *display)
> > > > > >      set_gl_draw_async_count(display, display->priv-
> >gl_draw_async_count -
> > > 1);
> > > > > >  }
> > > > > >
> > > > > > +void display_channel_encode_done(DisplayChannel *display,
> > > > > > +                                 RedDrawable *red_drawable)
> > > > > > +{
> > > > > > +    if (red_drawable->dmabuf_fd > 0) {
> > > > > > +        red_qxl_dmabuf_encode_async_complete(display->priv->qxl);
> > > > > > +        red_drawable->dmabuf_fd = 0;
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > >  int display_channel_get_video_stream_id(DisplayChannel *display,
> > > > > VideoStream *stream)
> > > > > >  {
> > > > > >      return static_cast<int>(stream - display->priv->streams_buf.data());
> > > > > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > > > > index c54df25c..0a1e520c 100644
> > > > > > --- a/server/display-channel.h
> > > > > > +++ b/server/display-channel.h
> > > > > > @@ -127,6 +127,8 @@ void                       display_channel_gl_scanout
> > > > > (DisplayCha
> > > > > >  void                       display_channel_gl_draw                   (DisplayChannel
> > > > > *display,
> > > > > >                                                                        SpiceMsgDisplayGlDraw
> *draw);
> > > > > >  void                       display_channel_gl_draw_done
> (DisplayChannel
> > > > > *display);
> > > > > > +void                       display_channel_encode_done
> (DisplayChannel
> > > > > *display,
> > > > > > +                                                                      RedDrawable *drawable);
> > > > > >
> > > > > >  void display_channel_process_draw(DisplayChannel *display,
> > > > > >                                    red::shared_ptr<RedDrawable> &&red_drawable,
> > > > > > diff --git a/server/red-qxl.cpp b/server/red-qxl.cpp
> > > > > > index 48c293ae..42a4029b 100644
> > > > > > --- a/server/red-qxl.cpp
> > > > > > +++ b/server/red-qxl.cpp
> > > > > > @@ -493,6 +493,32 @@ void
> > > red_qxl_gl_draw_async_complete(QXLInstance
> > > > > *qxl)
> > > > > >      red_qxl_async_complete(qxl, cookie);
> > > > > >  }
> > > > > >
> > > > > > +SPICE_GNUC_VISIBLE
> > > > > > +void spice_qxl_dmabuf_encode_async(QXLInstance *qxl,
> > > > > > +                                   int fd, uint64_t cookie)
> > > > > > +{
> > > > > > +    QXLState *qxl_state;
> > > > > > +
> > > > > > +    spice_return_if_fail(qxl != nullptr);
> > > > > > +    qxl_state = qxl->st;
> > > > > > +
> > > > > > +    qxl_state->scanout.drm_dma_buf_fd = fd;
> > > > > > +    qxl_state->gl_draw_cookie = cookie;
> > > > >
> > > > > This behaviour is prone to leak resources.
> > > > [Kasireddy, Vivek] I guess I could do what spice_qxl_gl_scanout does:
> > > > that is, prevent the (Gstreamer) encoder from closing the fd and instead
> > > > do the following here:
> > > >     pthread_mutex_lock(&qxl_state->scanout_mutex);
> > > >
> > > >     if (qxl_state->scanout.drm_dma_buf_fd >= 0) {
> > > >         close(qxl_state->scanout.drm_dma_buf_fd);
> > > >     }
> > > >
> > > > Do you think this would help?
> > > >
> > > > Thanks,
> > > > Vivek
> > > >
> > > > >
> > > > > > +}
> > > > > > +
> > > > > > +void red_qxl_dmabuf_encode_async_complete(QXLInstance *qxl)
> > > > > > +{
> > > > > > +    QXLState *qxl_state = qxl->st;
> > > > > > +    uint64_t cookie = qxl->st->gl_draw_cookie;
> > > > > > +
> > > > > > +    if (cookie == GL_DRAW_COOKIE_INVALID) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +    qxl_state->scanout.drm_dma_buf_fd = 0;
> > > > > > +    qxl->st->gl_draw_cookie = GL_DRAW_COOKIE_INVALID;
> > > > > > +    red_qxl_async_complete(qxl, cookie);
> > > > > > +}
> > > > > > +
> > > > > >  SPICE_GNUC_VISIBLE
> > > > > >  void spice_qxl_set_device_info(QXLInstance *instance,
> > > > > >                                 const char *device_address,
> > > > > > diff --git a/server/red-qxl.h b/server/red-qxl.h
> > > > > > index 2084acb1..e8e7c373 100644
> > > > > > --- a/server/red-qxl.h
> > > > > > +++ b/server/red-qxl.h
> > > > > > @@ -40,6 +40,7 @@ bool
> red_qxl_get_allow_client_mouse(QXLInstance
> > > *qxl,
> > > > > int *x_res, int *y_res, in
> > > > > >  SpiceMsgDisplayGlScanoutUnix *red_qxl_get_gl_scanout(QXLInstance
> > > *qxl);
> > > > > >  void red_qxl_put_gl_scanout(QXLInstance *qxl,
> > > > > SpiceMsgDisplayGlScanoutUnix *scanout);
> > > > > >  void red_qxl_gl_draw_async_complete(QXLInstance *qxl);
> > > > > > +void red_qxl_dmabuf_encode_async_complete(QXLInstance *qxl);
> > > > > >  int red_qxl_check_qxl_version(QXLInstance *qxl, int major, int minor);
> > > > > >  SpiceServer* red_qxl_get_server(QXLState *qxl);
> > > > > >  uint32_t red_qxl_marshall_device_display_info(const QXLInstance
> *qxl,
> > > > > SpiceMarshaller *m);
> > > > > > diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> > > > > > index bf17476b..ca9816ec 100644
> > > > > > --- a/server/spice-qxl.h
> > > > > > +++ b/server/spice-qxl.h
> > > > > > @@ -92,6 +92,8 @@ void spice_qxl_gl_draw_async(QXLInstance *qxl,
> > > > > >                               uint32_t x, uint32_t y,
> > > > > >                               uint32_t w, uint32_t h,
> > > > > >                               uint64_t cookie);
> > > > > > +void spice_qxl_dmabuf_encode_async(QXLInstance *qxl,
> > > > > > +                                   int fd, uint64_t cookie);
> > > > > >
> > > > > >  /* since spice 0.14.2 */
> > > > > >
> > > > > > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > > > > > index 8da46c20..9748cc24 100644
> > > > > > --- a/server/spice-server.syms
> > > > > > +++ b/server/spice-server.syms
> > > > > > @@ -182,4 +182,5 @@ SPICE_SERVER_0.14.3 {
> > > > > >  global:
> > > > > >      spice_server_get_video_codecs;
> > > > > >      spice_server_free_video_codecs;
> > > > > > +    spice_qxl_dmabuf_encode_async;
> > > > > >  } SPICE_SERVER_0.14.2;
> > > > >
> > > > > Frediano




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