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 display_ch
Re: [Spice-devel] [RFC v1 1/4] red-parse-qxl: Extract the dmabuf fd from the scanout
Il giorno lun 16 gen 2023 alle ore 09:00 Kasireddy, Vivek
ha scritto:
>
> 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,
> > > > >
> > > > > Thank you for taking a look at this patch series.
> > > > >
> > > > > >
> > > > > > Il giorno mer 11 gen 2023 alle ore 05:42 Vivek Kasireddy
> > > > > > ha scritto:
> > > > > > >
> > > > > > > If the scanout has a valid dmabuf fd, then it is extracted and a
> > > > > > > copy (of the fd) is stored in the drawable.
> > > > > > >
> > > > > > > Cc: Gerd Hoffmann
> > > > > > > Cc: Marc-André Lureau
> > > > > > > Cc: Dongwon Kim
> > > > > > > Signed-off-by: Vivek Kasireddy
> > > > > > > ---
> > > > > > > server/red-parse-qxl.cpp | 8
> > > > > > > server/red-parse-qxl.h | 1 +
> > > > > > > 2 files changed, 9 insertions(+)
> > > > > > >
> > > > > > > diff --git a/server/red-parse-qxl.cpp b/server/red-parse-qxl.cpp
> > > > > > > index 68b9759d..8d9b82fb 100644
> > > > > > > --- a/server/red-parse-qxl.cpp
> > > > > > > +++ b/server/red-parse-qxl.cpp
> > > > > > > @@ -1038,6 +1038,7 @@ static bool
> > red_get_native_drawable(QXLInstance
> > > > > > *qxl_instance, RedMemSlotInfo *s
> > > > > > > RedDrawable *red,
> > > > > > > QXLPHYSICAL addr,
> > uint32_t
> > > > flags)
> > > > > > > {
> > > > > > > QXLDrawable *qxl;
> > > > > > > +SpiceMsgDisplayGlScanoutUnix *scanout;
> > > > > > > int i;
> > > > > > >
> > > > > > > qxl = static_cast(memslot_get_virt(slots,
> > > > > > > addr,
> > > > > > sizeof(*qxl), group_id));
> > > > > > > @@ -1059,6 +1060,13 @@ static bool
> > > > red_get_native_drawable(QXLInstance
> > > > > > *qxl_instance, RedMemSlotInfo *s
> > > > > > > red_get_rect_ptr(&red->surfaces_rects[i],
> > > > > > > &qxl->surfaces_rects[i]);
> > > > > > > }
> > > > > > >
> > > > > > > +red->dmabuf_fd = 0;
> > > > > >
> > > > > > 0 is a perfectly valid file descriptor, usually invalid file
> > > > > > descriptors are initialized with -1.
> > > > > [Kasireddy, Vivek] Ok, I'll initialize it to -1.
> > > > >
> > > > > >
> > > > > > > +scanout = red_qxl_get_gl_scanout(qxl_instance);
> > > > > > > +if (scanout != nullptr) {
> > > > > > > +red->dmabuf_fd = scanout->drm_dma_buf_fd;
> > > > > > > +}
> > > > > > > +red_qxl_put_gl_scanout(qxl_instance, scanout);
> > > > > > > +
> > > > > > > red->type = qxl->type;
> > > > > > > switch (red->type) {
> > > > > > > case QXL_DRAW_ALPHA_BLEND:
> > > > > > > diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> > > > > > > index 8bf0e2e3..dee50743 100644
> > > > > > > --- a/server/red-parse-qxl.h
> > > > > > > +++ b/server/red-parse-qxl.h
> > > > > > > @@ -67,6 +67,7 @@ struct RedDrawable final: public
> > > > > > RedQXLResource {
> > > > > > > SpiceWhiteness whiteness;
> > > > > > > SpiceComposite composite;
> > > > > > > } u;
> > > > > > > +int32_t dmabuf_fd;
> > > > > > > };
> > > > > > >
> > > > > > > struct RedUpdateCmd final: public RedQXLResource
> > {
> > > > > >
> > > > > > This patch looks pretty error prone, it's easy to generate leaks or
> > > > > > use after free (of file descriptor).
> > > > > [Kasireddy, Vivek] At-least with Qemu, we usually hand over a dup of
> > > > > the
> > > > original
> > > > > fd to Spice server with the assumption that the server will close it
> > > > > after it is
> > > > > done using it.
> > > > >
> > > > > > Also it adds the assumption that the drawing is always associated
> > > > > > with
> > > > > > the DMA surface which is racy.
> > > > > [Kasireddy, Vivek] I see that access to the scanout and
> > > > > drm_dma_buf_fd is
> > > > protected
> > > > > with a scanout_mutex. Are you suggesting that using
> > red_qxl_get/put_gl_scanout
> > > > is
> > > > > not going to be sufficient to prevent races?
> > > > >
> > > >
> > > > No, now you created 3 copies and only one is protected by that mutex.
> > > > The race is in resource management.
> > > [Kasireddy, Vivek] My understanding is that applications are supposed to
> > > wait
> > until
> > > the encoding is done before submitting another frame/fd. I am not sure if
> > > it is
> > naive
> > > to assume that. Anyway, what do you suggest needs to happen if they submit
> > > another fd while the encoding of the previous fd has not been completed
> > > yet?
> > >
> >
> > Hi,
> >maybe a step back is helpful here.
> >
> > What is, from a high level perspective (that is, possibly no code
> > level) your objective?
> > What's the setup? virtio-vga? Remote? Local?
> [Kasireddy, Vivek] My objective (and setup) is described in the associated
> Qemu
> patch series here:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02094.html
>
> In a nutshell, I'd like to s
