Re: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Apr 23, 2024 at 07:05:20PM +0000, Kim, Dongwon wrote:
> Hi Daniel,
> 
> > -----Original Message-----
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > Sent: Tuesday, April 23, 2024 7:07 AM
> > To: Kim, Dongwon <dongwon.kim@intel.com>
> > Cc: qemu-devel@nongnu.org; marcandre.lureau@redhat.com;
> > philmd@linaro.org
> > Subject: Re: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for
> > QemuDmaBuf struct and helpers
> > 
> > On Mon, Apr 22, 2024 at 07:22:49PM -0700, dongwon.kim@intel.com wrote:
> > > From: Dongwon Kim <dongwon.kim@intel.com>
> > >
> > > New header and source files are added for containing QemuDmaBuf struct
> > > definition and newly introduced helpers for creating/freeing the
> > > struct and accessing its data.
> > >
> > > v10: Change the license type for both dmabuf.h and dmabuf.c from MIT to
> > >      GPL to be in line with QEMU's default license
> > >      (Daniel P. Berrangé <berrange@redhat.com>)
> > >
> > > Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > > ---
> > >  include/ui/console.h |  20 +----
> > >  include/ui/dmabuf.h  |  64 +++++++++++++++
> > >  ui/dmabuf.c          | 189
> > +++++++++++++++++++++++++++++++++++++++++++
> > >  ui/meson.build       |   1 +
> > >  4 files changed, 255 insertions(+), 19 deletions(-)  create mode
> > > 100644 include/ui/dmabuf.h  create mode 100644 ui/dmabuf.c
> > >
> > > diff --git a/include/ui/console.h b/include/ui/console.h index
> > > 0bc7a00ac0..a208a68b88 100644
> > > --- a/include/ui/console.h
> > > +++ b/include/ui/console.h
> > > @@ -7,6 +7,7 @@
> > >  #include "qapi/qapi-types-ui.h"
> > >  #include "ui/input.h"
> > >  #include "ui/surface.h"
> > > +#include "ui/dmabuf.h"
> > >
> > >  #define TYPE_QEMU_CONSOLE "qemu-console"
> > >  OBJECT_DECLARE_TYPE(QemuConsole, QemuConsoleClass,
> > QEMU_CONSOLE) @@
> > > -185,25 +186,6 @@ struct QEMUGLParams {
> > >      int minor_ver;
> > >  };
> > >
> > > -typedef struct QemuDmaBuf {
> > > -    int       fd;
> > > -    uint32_t  width;
> > > -    uint32_t  height;
> > > -    uint32_t  stride;
> > > -    uint32_t  fourcc;
> > > -    uint64_t  modifier;
> > > -    uint32_t  texture;
> > > -    uint32_t  x;
> > > -    uint32_t  y;
> > > -    uint32_t  backing_width;
> > > -    uint32_t  backing_height;
> > > -    bool      y0_top;
> > > -    void      *sync;
> > > -    int       fence_fd;
> > > -    bool      allow_fences;
> > > -    bool      draw_submitted;
> > > -} QemuDmaBuf;
> > > -
> > >  enum display_scanout {
> > >      SCANOUT_NONE,
> > >      SCANOUT_SURFACE,
> > > diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h new file mode
> > > 100644 index 0000000000..7a60116ee6
> > > --- /dev/null
> > > +++ b/include/ui/dmabuf.h
> > > @@ -0,0 +1,64 @@
> > > +/*
> > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > + *
> > > + * QemuDmaBuf struct and helpers used for accessing its data
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +
> > > +#ifndef DMABUF_H
> > > +#define DMABUF_H
> > > +
> > > +typedef struct QemuDmaBuf {
> > > +    int       fd;
> > > +    uint32_t  width;
> > > +    uint32_t  height;
> > > +    uint32_t  stride;
> > > +    uint32_t  fourcc;
> > > +    uint64_t  modifier;
> > > +    uint32_t  texture;
> > > +    uint32_t  x;
> > > +    uint32_t  y;
> > > +    uint32_t  backing_width;
> > > +    uint32_t  backing_height;
> > > +    bool      y0_top;
> > > +    void      *sync;
> > > +    int       fence_fd;
> > > +    bool      allow_fences;
> > > +    bool      draw_submitted;
> > > +} QemuDmaBuf;
> > > +
> > > +QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
> > > +                                   uint32_t stride, uint32_t x,
> > > +                                   uint32_t y, uint32_t backing_width,
> > > +                                   uint32_t backing_height, uint32_t fourcc,
> > > +                                   uint64_t modifier, int32_t
> > > +dmabuf_fd,
> > 
> > Should be 'int' not 'int32_t' for FDs.
> > 
> > > +                                   bool allow_fences, bool y0_top);
> > > +void qemu_dmabuf_free(QemuDmaBuf *dmabuf);
> > > +
> > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuDmaBuf,
> > qemu_dmabuf_free);
> > > +
> > > +int32_t qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
> > 
> > Again should be 'int' not 'int42_t'
> > 
> > I think there ought to also be a
> > 
> >   int qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf);
> > 
> > to do the dup() call in one go too
> > 
> > > diff --git a/ui/dmabuf.c b/ui/dmabuf.c new file mode 100644 index
> > > 0000000000..f447cce4fe
> > > --- /dev/null
> > > +++ b/ui/dmabuf.c
> > 
> > > +
> > > +void qemu_dmabuf_free(QemuDmaBuf *dmabuf)
> > > +{
> > > +    if (dmabuf == NULL) {
> > > +        return;
> > > +    }
> > > +
> > 
> > I think this method should be made to call
> > 
> >   qemu_dmabuf_close()
> > 
> > to release the FD, if not already released, otherwise
> > this method could be a resource leak.
>  
> [Kim, Dongwon]  So you mean this close call should close all FDs including duped FDs, which implies we should include the list of duped FD and its management?

No, only the fd that is stored by the struct.

*  qemu_dmabuf_get_fd

   the returned "fd" remains owned by QemuDmabuf and should be closed
   by qemu_dmabuf_close() or qemu_dmabuf_free()

* qemu_dmabuf_dup_fd

   the returned "fd" is owned by the caller and it must close it when
   needed.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|