Re: [PATCH v6 1/3] ui/console: Introduce dpy_gl_qemu_dmabuf_get_..() helpers

[Marc-André Lureau]

Hi

On Wed, Apr 17, 2024 at 9:06 PM Kim, Dongwon <dongwon.kim@intel.com> wrote:
>
> Hi Daniel,
>
> > -----Original Message-----
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > Sent: Wednesday, April 17, 2024 4:05 AM
> > To: Kim, Dongwon <dongwon.kim@intel.com>
> > Cc: qemu-devel@nongnu.org; marcandre.lureau@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.kim@intel.com wrote:
> > > From: Dongwon Kim <dongwon.kim@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.lureau@redhat.com>
> > > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > Signed-off-by: Dongwon Kim <dongwon.kim@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?

Sure, I was about to ask the same. Anything we can do to slim
ui/console.c helps :)