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

[Daniel P . Berrangé]

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.

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