[Spice-devel] [RFC v1 1/4] red-parse-qxl: Extract the dmabuf fd from the scanout

[Frediano Ziglio]

Il giorno lun 16 gen 2023 alle ore 09:00 Kasireddy, Vivek
<vivek.kasireddy at intel.com> ha scritto:
>
> Hi Frediano,
>
> >
> > Il giorno ven 13 gen 2023 alle ore 04:08 Kasireddy, Vivek
> > <vivek.kasireddy at intel.com> ha scritto:
> > >
> > > Hi Frediano,
> > >
> > > >
> > > > Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
> > > > <vivek.kasireddy at intel.com> 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
> > > > > > <vivek.kasireddy at intel.com> 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 <kraxel at redhat.com>
> > > > > > > Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > > > > Cc: Dongwon Kim <dongwon.kim at intel.com>
> > > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> > > > > > > ---
> > > > > > >  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<QXLDrawable *>(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<RedDrawable> {
> > > > > > >          SpiceWhiteness whiteness;
> > > > > > >          SpiceComposite composite;
> > > > > > >      } u;
> > > > > > > +    int32_t dmabuf_fd;
> > > > > > >  };
> > > > > > >
> > > > > > >  struct RedUpdateCmd final: public RedQXLResource<RedUpdateCmd>
> > {
> > > > > >
> > > > > > 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 send the fd associated with the Guest desktop Framebuffer
> (that Qemu obtains) to the Gstreamer encoder (that is part of Spice) to have it
> streamed to a remote client.
>

Good, now it's more clear.

> >
> > Back a bit on API level and your specific question, there are 2
> > functions, spice_qxl_gl_scanout
> > and spice_qxl_gl_draw_async. An application (like Qemu) should set the
> > current active scanout (as dmabuf)
> > spice_qxl_gl_scanout. The dmabuf ownership is passed to SPICE (which
> > will take care of closing
> > the file descriptor). The application will request to use the current
> > scanout using spice_qxl_gl_draw_async.
> > Application should wait to update the current scanout till SPICE
> > finishes "drawing" (that is using the scanout
> > content).
> > Application can request multiple drawings using the same scanout (that
> > is a single spice_qxl_gl_scanout
> > can be followed by multiple spice_qxl_gl_draw_async calls).
> [Kasireddy, Vivek] Thank you for the explanation; however, IIUC, what you
> described seems relevant for a local client. I suspect most of what you said
> would probably be applicable for a remote client as well given my use-case
> (fd to encoder).
>

What you do with the information you get is implementation detail, not
interface detail.
I don't see why such an interface has to forcely deal with local
clients. It's and should be agnostic to client type.

> >
> > > Also, my goal is to somehow get the fd from the application (Qemu) to the
> > encoder.
> >
> > SPICE already has the fd so IMHO you just need to forward to the encoder.
> [Kasireddy, Vivek] This, I think is the crux of the issue. That is, how to forward
> the fd (from the scanout object which is a singleton) to the encoder in the most
> efficient and race-free way. I chose to use the drawable as a means to accomplish
> this but I think there might be a better way.
>

Race free and using drawable are IMHO separate things. I wish there
would be an easier and better way too. But yes, I agree at the moment
it's the shortest way. To be honest GStreamer uses the drawable
(technically you are using RedDrawable... the code is a bit weird in
this respect, there's also a Drawable structure which has a
RedDrawable structure) but only when that drawable is a simple bitmap.
So I would say the drawable solution, although a hack, it's a viable
solution.

> Thanks,
> Vivek
>