RE: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers
Hi Daniel, > -Original Message- > From: Daniel P. Berrangé > Sent: Tuesday, April 23, 2024 7:07 AM > To: Kim, Dongwon > 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 > > > > 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é ) > > > > Suggested-by: Marc-André Lureau > > Cc: Philippe Mathieu-Daudé > > Cc: Daniel P. Berrangé > > Cc: Vivek Kasireddy > > Signed-off-by: Dongwon Kim > > --- > > 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 00..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_du
Re: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers
On Tue, Apr 23, 2024 at 07:05:20PM +, Kim, Dongwon wrote: > Hi Daniel, > > > -Original Message- > > From: Daniel P. Berrangé > > Sent: Tuesday, April 23, 2024 7:07 AM > > To: Kim, Dongwon > > 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 > > > > > > 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é ) > > > > > > Suggested-by: Marc-André Lureau > > > Cc: Philippe Mathieu-Daudé > > > Cc: Daniel P. Berrangé > > > Cc: Vivek Kasireddy > > > Signed-off-by: Dongwon Kim > > > --- > > > 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 00..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_
RE: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers
Hi Daniel, > -Original Message- > From: Daniel P. Berrangé > Sent: Tuesday, April 23, 2024 7:07 AM > To: Kim, Dongwon > 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 > > > > 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é ) > > > > Suggested-by: Marc-André Lureau > > Cc: Philippe Mathieu-Daudé > > Cc: Daniel P. Berrangé > > Cc: Vivek Kasireddy > > Signed-off-by: Dongwon Kim > > --- > > 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 00..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
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 > > 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é ) > > Suggested-by: Marc-André Lureau > Cc: Philippe Mathieu-Daudé > Cc: Daniel P. Berrangé > Cc: Vivek Kasireddy > Signed-off-by: Dongwon Kim > --- > 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 00..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 00..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 :|
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 > > 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é ) FYI, in future, notes about changes made in each version would best go after the '---', since they're not something we need to record in the commit message once merged. Don't re-send the series now just for that reason though - whomever merges can trim this. > > Suggested-by: Marc-André Lureau > Cc: Philippe Mathieu-Daudé > Cc: Daniel P. Berrangé > Cc: Vivek Kasireddy > Signed-off-by: Dongwon Kim > --- > 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 Reviewed-by: Daniel P. Berrangé 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 :|