| Message ID | 20240423022253.1003295-3-dongwon.kim@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ui/console: Private QemuDmaBuf struct | expand |
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>) 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 <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 Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
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
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? > > > + 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 :|
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
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] Daniel, I initially agreed with you on the idea that is we should close dmabuf->fd here in freeing routine and I already submitted v11 containing that change but I just found it causes some regression at least for virtio-gpu case because this fd is for udmabuf bound to the actual resource. This is designed to be closed by virtio-gpu when the resource is freed, not when QemuDmaBuf struct referencing that resource (udmabuf) is freed. So I guess we need to split free and close here. I will work on v12 but I will wait for your feedback before submitting it. Thanks! > > > + 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 :|
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, + 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); +uint32_t qemu_dmabuf_get_width(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_height(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf); +uint64_t qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_x(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_y(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf); +uint32_t qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf); +bool qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf); +void *qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf); +int32_t qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf); +bool qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf); +bool qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf); +void qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf, uint32_t texture); +void qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd); +void qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync); +void qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool draw_submitted); +void qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd); + +#endif diff --git a/ui/dmabuf.c b/ui/dmabuf.c new file mode 100644 index 0000000000..f447cce4fe --- /dev/null +++ b/ui/dmabuf.c @@ -0,0 +1,189 @@ +/* + * 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. + */ + +#include "qemu/osdep.h" +#include "ui/dmabuf.h" + +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, + bool allow_fences, bool y0_top) { + QemuDmaBuf *dmabuf; + + dmabuf = g_new0(QemuDmaBuf, 1); + + dmabuf->width = width; + dmabuf->height = height; + dmabuf->stride = stride; + dmabuf->x = x; + dmabuf->y = y; + dmabuf->backing_width = backing_width; + dmabuf->backing_height = backing_height; + dmabuf->fourcc = fourcc; + dmabuf->modifier = modifier; + dmabuf->fd = dmabuf_fd; + dmabuf->allow_fences = allow_fences; + dmabuf->y0_top = y0_top; + dmabuf->fence_fd = -1; + + return dmabuf; +} + +void qemu_dmabuf_free(QemuDmaBuf *dmabuf) +{ + if (dmabuf == NULL) { + return; + } + + g_free(dmabuf); +} + +int32_t qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->fd; +} + +uint32_t qemu_dmabuf_get_width(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->width; +} + +uint32_t qemu_dmabuf_get_height(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->height; +} + +uint32_t qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->stride; +} + +uint32_t qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->fourcc; +} + +uint64_t qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->modifier; +} + +uint32_t qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->texture; +} + +uint32_t qemu_dmabuf_get_x(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->x; +} + +uint32_t qemu_dmabuf_get_y(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->y; +} + +uint32_t qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->backing_width; +} + +uint32_t qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->backing_height; +} + +bool qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->y0_top; +} + +void *qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->sync; +} + +int32_t qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->fence_fd; +} + +bool qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->allow_fences; +} + +bool qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf) +{ + assert(dmabuf != NULL); + + return dmabuf->draw_submitted; +} + +void qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf, uint32_t texture) +{ + assert(dmabuf != NULL); + dmabuf->texture = texture; +} + +void qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd) +{ + assert(dmabuf != NULL); + dmabuf->fence_fd = fence_fd; +} + +void qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync) +{ + assert(dmabuf != NULL); + dmabuf->sync = sync; +} + +void qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool draw_submitted) +{ + assert(dmabuf != NULL); + dmabuf->draw_submitted = draw_submitted; +} + +void qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd) +{ + assert(dmabuf != NULL); + dmabuf->fd = fd; +} diff --git a/ui/meson.build b/ui/meson.build index a5ce22a678..5d89986b0e 100644 --- a/ui/meson.build +++ b/ui/meson.build @@ -7,6 +7,7 @@ system_ss.add(files( 'clipboard.c', 'console.c', 'cursor.c', + 'dmabuf.c', 'input-keymap.c', 'input-legacy.c', 'input-barrier.c',