Hi Daniel,
> -----Original Message-----
> From: Daniel P. Berrangé <berra...@redhat.com>
> Sent: Tuesday, April 23, 2024 7:07 AM
> To: Kim, Dongwon <dongwon....@intel.com>
> Cc: qemu-devel@nongnu.org; marcandre.lur...@redhat.com;
> phi...@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....@intel.com wrote:
> > From: Dongwon Kim <dongwon....@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é <berra...@redhat.com>)
> >
> > Suggested-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Cc: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Cc: Daniel P. Berrangé <berra...@redhat.com>
> > Cc: Vivek Kasireddy <vivek.kasire...@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon....@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?
>
> > + g_free(dmabuf);
> > +}
> > +
>
> 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 :|
