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

[Kasireddy, Vivek]

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?

Also, my goal is to somehow get the fd from the application (Qemu) to the encoder.
I did not find any other way to easily accomplish this without using the scanout and
drawable. Given this, do you think protecting access to encoder->dmabuf_data with
a mutex would be sufficient to avoid races? Or, I guess I could just pass dmabuf_data
as an extra parameter to encode_frame and avoid storing it in the encoder. Or, is there
a more elegant (and race-free) way to do this that you can suggest?

Thanks,
Vivek

> 
> > Thanks,
> > Vivek
> >
> 
> Frediano