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

[Daniel P . Berrangé]

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 :|