Re: [PATCH v10 3/6] ui/console: Use qemu_dmabuf_get_..() helpers instead

[Daniel P . Berrangé]

On Mon, Apr 22, 2024 at 07:22:50PM -0700, dongwon.kim@intel.com wrote:
> From: Dongwon Kim <dongwon.kim@intel.com>
> 
> This commit 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
> 
> v7: removed prefix, "dpy_gl_" from all helpers
> 
> v8: Introduction of helpers was removed as those were already added
>     by the previous commit
> 
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-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>
> ---
>  hw/display/vhost-user-gpu.c     |  6 ++--
>  hw/display/virtio-gpu-udmabuf.c |  7 +++--
>  hw/vfio/display.c               | 15 +++++++---
>  ui/console.c                    |  4 +--
>  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 +++++++++++++++++++--------------
>  12 files changed, 187 insertions(+), 108 deletions(-)
> 
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 709c8a02a1..ea9a6c5d10 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -249,6 +249,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, 
> VhostUserGpuMsg *msg)
>      case VHOST_USER_GPU_DMABUF_SCANOUT: {
>          VhostUserGpuDMABUFScanout *m = &msg->payload.dmabuf_scanout;
>          int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
> +        int old_fd;
>          QemuDmaBuf *dmabuf;
>  
>          if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
> @@ -262,8 +263,9 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, 
> VhostUserGpuMsg *msg)
>          g->parent_obj.enable = 1;
>          con = g->parent_obj.scanout[m->scanout_id].con;
>          dmabuf = &g->dmabuf[m->scanout_id];
> -        if (dmabuf->fd >= 0) {
> -            close(dmabuf->fd);
> +        old_fd = qemu_dmabuf_get_fd(dmabuf);
> +        if (old_fd >= 0) {
> +            close(old_fd);
>              dmabuf->fd = -1;

I think we shoul hdave a qemu_dmabuf_close() method for this logic


> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 1aa440c663..4861c8161d 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -259,9 +259,13 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice 
> *vdev,
>  
>  static void vfio_display_free_one_dmabuf(VFIODisplay *dpy, VFIODMABuf 
> *dmabuf)
>  {
> +    int fd;
> +
>      QTAILQ_REMOVE(&dpy->dmabuf.bufs, dmabuf, next);
> +
> +    fd = qemu_dmabuf_get_fd(&dmabuf->buf);
>      dpy_gl_release_dmabuf(dpy->con, &dmabuf->buf);
> -    close(dmabuf->buf.fd);
> +    close(fd);

Again, we should have a qemu_dmabuf_close() method

>      g_free(dmabuf);
>  }
>  


>          } else {
> -            trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id,
> -                                               dmabuf->width, 
> dmabuf->height);
> +            stride = qemu_dmabuf_get_stride(dmabuf);
> +            fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> +            y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> +            fd = qemu_dmabuf_get_fd(dmabuf);

This is where it would use a qemu_dmabuf_dup_fd(), avoiding
the subsequent dup() in the method call

> +
> +            trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
>              /* note: spice server will close the fd, so hand over a dup */
> -            spice_qxl_gl_scanout(&ssd->qxl, dup(dmabuf->fd),
> -                                 dmabuf->width, dmabuf->height,
> -                                 dmabuf->stride, dmabuf->fourcc,
> -                                 dmabuf->y0_top);
> +            spice_qxl_gl_scanout(&ssd->qxl, dup(fd), width, height,
> +                                 stride, fourcc, y_0_top);
>          }
> -        qemu_spice_gl_monitor_config(ssd, 0, 0, dmabuf->width, 
> dmabuf->height);
> +        qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
>          ssd->guest_dmabuf_refresh = false;

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