Hi Daniel,
> -----Original Message-----
> From: Daniel P. Berrangé <berra...@redhat.com>
> Sent: Wednesday, April 17, 2024 4:05 AM
> To: Kim, Dongwon <dongwon....@intel.com>
> Cc: qemu-devel@nongnu.org; marcandre.lur...@redhat.com
> Subject: Re: [PATCH v6 1/3] ui/console: Introduce
> dpy_gl_qemu_dmabuf_get_..() helpers
>
> On Tue, Apr 16, 2024 at 09:09:52PM -0700, dongwon....@intel.com wrote:
> > From: Dongwon Kim <dongwon....@intel.com>
> >
> > This commit introduces dpy_gl_qemu_dmabuf_get_... helpers to extract
> > specific fields from the QemuDmaBuf struct. It also updates all
> > instances where fields within the QemuDmaBuf struct are directly
> > accessed, replacing them with calls to these new helper functions.
> >
> > v6: fix typos in helper names in ui/spice-display.c
> >
> > Suggested-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Cc: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Cc: Vivek Kasireddy <vivek.kasire...@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon....@intel.com>
> > ---
> > include/ui/console.h | 17 +++++
> > hw/display/vhost-user-gpu.c | 6 +-
> > hw/display/virtio-gpu-udmabuf.c | 7 +-
> > hw/vfio/display.c | 15 +++--
> > ui/console.c | 116 +++++++++++++++++++++++++++++++-
> > ui/dbus-console.c | 9 ++-
> > ui/dbus-listener.c | 43 +++++++-----
> > ui/egl-headless.c | 23 +++++--
> > ui/egl-helpers.c | 47 +++++++------
> > ui/gtk-egl.c | 48 ++++++++-----
> > ui/gtk-gl-area.c | 37 ++++++----
> > ui/gtk.c | 6 +-
> > ui/spice-display.c | 50 ++++++++------
> > 13 files changed, 316 insertions(+), 108 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > 0bc7a00ac0..6292943a82 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -358,6 +358,23 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con,
> QemuDmaBuf *dmabuf,
> > bool have_hot, uint32_t hot_x, uint32_t
> > hot_y); void dpy_gl_cursor_position(QemuConsole *con,
> > uint32_t pos_x, uint32_t pos_y);
> > +
> > +int32_t dpy_gl_qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_width(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_height(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf); uint64_t
> > +dpy_gl_qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_x(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_y(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf);
> uint32_t
> > +dpy_gl_qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf); bool
> > +dpy_gl_qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf); void
> > +*dpy_gl_qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf); int32_t
> > +dpy_gl_qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf); bool
> > +dpy_gl_qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf); bool
> > +dpy_gl_qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf);
>
> IMHO these method names don't need a "dpy_gl_" prefix on them. Since
> they're accessors for the "QemuDmaBuf" struct, I think its sufficient to just
> have "qemu_dmabuf_" as the name prefix, making names more compact.
>
> The console.{h,c} files are a bit of a dumping ground for UI code. While
> QemuDmaBuf was just a struct with direct field access that's OK.
>
> With turning this into a more of an object with accessors, I think it would
> also
> be desirable to move the struct definition and all its methods into separate
> ui/dmabuf.{c,h} files, so its fully self-contained.
[Kim, Dongwon] I am ok with changing function names and create
separate c and h for dmabuf helpers as you suggested. But I would
like to hear Marc-André's opinion about this suggestion before I make
such changes.
Marc-André, do you have any thought on Daniel's suggestion?
>
> 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 :|
