Hi Frediano,
>
> Il giorno gio 12 gen 2023 alle ore 07:03 Kasireddy, Vivek
> <vivek.kasire...@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.kasire...@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 <kra...@redhat.com>
> > > > Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > > Cc: Dongwon Kim <dongwon....@intel.com>
> > > > Signed-off-by: Vivek Kasireddy <vivek.kasire...@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
