Re: [Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
Hi Frediano,
>
> Il giorno ven 13 gen 2023 alle ore 04:08 Kasireddy, Vivek
> ha scritto:
> >
> > Hi Frediano,
> >
> > >
> > > Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
> > > ha scritto:
> > > >
> > > > Hi Frediano,
> > > >
> > > > >
> > > > > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > > > > 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
> > > > > > Cc: Marc-André Lureau
> > > > > > Cc: Dongwon Kim
> > > > > > Signed-off-by: Vivek Kasireddy
> > > > > > ---
> > > > > > 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);
> > > > > > +
Re: [Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
Il giorno ven 13 gen 2023 alle ore 04:08 Kasireddy, Vivek
ha scritto:
>
> Hi Frediano,
>
> >
> > Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
> > ha scritto:
> > >
> > > Hi Frediano,
> > >
> > > >
> > > > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > > > 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.
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
> > > > > Cc: Marc-André Lureau
> > > > > Cc: Dongwon Kim
> > > > > Signed-off-by: Vivek Kasireddy
> > > > > ---
> > > > > 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(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
Re: [Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
Hi Frediano,
>
> Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
> ha scritto:
> >
> > Hi Frediano,
> >
> > >
> > > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > > 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.
>
> > 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
> > > > Cc: Marc-André Lureau
> > > > Cc: Dongwon Kim
> > > > Signed-off-by: Vivek Kasireddy
> > > > ---
> > > > 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(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
> > > > &_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 !=
Re: [Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
ha scritto:
>
> Hi Frediano,
>
> >
> > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > 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.
> 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.
> >
> > > Cc: Gerd Hoffmann
> > > Cc: Marc-André Lureau
> > > Cc: Dongwon Kim
> > > Signed-off-by: Vivek Kasireddy
> > > ---
> > > 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(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
> > > &_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(_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)
> > > +{
> > > +
Re: [Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
Hi Frediano,
>
> Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> 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
> > Cc: Marc-André Lureau
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > ---
> > 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(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
> > &_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(_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
Re: [Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
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.
> Cc: Gerd Hoffmann
> Cc: Marc-André Lureau
> Cc: Dongwon Kim
> Signed-off-by: Vivek Kasireddy
> ---
> 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(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
> &_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.
> +}
> +
> +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 */
[Spice-devel] [RFC v1 2/4] display-channel: Add the asyncs associated with dmabuf encode
This async is triggered by the encoder indicating that the
encoding process is completed.
Cc: Gerd Hoffmann
Cc: Marc-André Lureau
Cc: Dongwon Kim
Signed-off-by: Vivek Kasireddy
---
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(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 &_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;
+}
+
+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;
--
2.37.2
