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.
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."
>
> > 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