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

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