RE: [PATCH v4 1/3] ui/console: Introduce dpy_gl_dmabuf_get_height/width() helpers

[Kim, Dongwon]

Hi Marc-André,

> -----Original Message-----
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> Sent: Friday, March 22, 2024 2:06 AM
> To: Kim, Dongwon <dongwon.kim@intel.com>
> Cc: qemu-devel@nongnu.org; philmd@linaro.org
> Subject: Re: [PATCH v4 1/3] ui/console: Introduce
> dpy_gl_dmabuf_get_height/width() helpers
> 
> Hi Kim
> 
> On Fri, Mar 22, 2024 at 3:45 AM <dongwon.kim@intel.com> wrote:
> >
> > From: Dongwon Kim <dongwon.kim@intel.com>
> >
> > dpy_gl_dmabuf_get_height() and dpy_gl_dmabuf_get_width() are helpers
> > for retrieving width and height fields from QemuDmaBuf struct.
> >
> 
> There are many places left where width/height fields are still accessed 
> directly.
> 
> If we want to make the whole structure private, we will probably need setters.
[Kim, Dongwon]  I am wondering if you are saying we need setters and getters 
for all individual fields and use those new functions in any places in QEMU 
where any of those fields are accessed including ui/* (e.g. gtk-egl.c)? 

> 
> I don't see why the function should silently return 0 when given NULL.
> Imho an assert(dmabuf != NULL) is appropriate (or g_return_val_if_fail).
[Kim, Dongwon] Yeah I can do that. I will update that part.

> 
> 
> 
> 
> 
> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > ---
> >  include/ui/console.h            |  2 ++
> >  hw/display/virtio-gpu-udmabuf.c |  7 ++++---
> >  hw/vfio/display.c               |  9 ++++++---
> >  ui/console.c                    | 18 ++++++++++++++++++
> >  4 files changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > 0bc7a00ac0..6064487fc4 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -358,6 +358,8 @@ 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);
> > +uint32_t dpy_gl_dmabuf_get_width(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_dmabuf_get_height(QemuDmaBuf *dmabuf);
> >  void dpy_gl_release_dmabuf(QemuConsole *con,
> >                             QemuDmaBuf *dmabuf);  void
> > dpy_gl_update(QemuConsole *con, diff --git
> > a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> > index d51184d658..a4ebf828ec 100644
> > --- a/hw/display/virtio-gpu-udmabuf.c
> > +++ b/hw/display/virtio-gpu-udmabuf.c
> > @@ -206,6 +206,7 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,  {
> >      struct virtio_gpu_scanout *scanout = 
> > &g->parent_obj.scanout[scanout_id];
> >      VGPUDMABuf *new_primary, *old_primary = NULL;
> > +    uint32_t width, height;
> >
> >      new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
> >      if (!new_primary) {
> > @@ -216,10 +217,10 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
> >          old_primary = g->dmabuf.primary[scanout_id];
> >      }
> >
> > +    width = dpy_gl_dmabuf_get_width(&new_primary->buf);
> > +    height = dpy_gl_dmabuf_get_height(&new_primary->buf);
> >      g->dmabuf.primary[scanout_id] = new_primary;
> > -    qemu_console_resize(scanout->con,
> > -                        new_primary->buf.width,
> > -                        new_primary->buf.height);
> > +    qemu_console_resize(scanout->con, width, height);
> >      dpy_gl_scanout_dmabuf(scanout->con, &new_primary->buf);
> >
> >      if (old_primary) {
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index 1aa440c663..c962e5f88f 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -286,6 +286,7 @@ static void vfio_display_dmabuf_update(void
> *opaque)
> >      VFIOPCIDevice *vdev = opaque;
> >      VFIODisplay *dpy = vdev->dpy;
> >      VFIODMABuf *primary, *cursor;
> > +    uint32_t width, height;
> >      bool free_bufs = false, new_cursor = false;
> >
> >      primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
> > @@ -296,10 +297,12 @@ static void vfio_display_dmabuf_update(void
> *opaque)
> >          return;
> >      }
> >
> > +    width = dpy_gl_dmabuf_get_width(&primary->buf);
> > +    height = dpy_gl_dmabuf_get_height(&primary->buf);
> > +
> >      if (dpy->dmabuf.primary != primary) {
> >          dpy->dmabuf.primary = primary;
> > -        qemu_console_resize(dpy->con,
> > -                            primary->buf.width, primary->buf.height);
> > +        qemu_console_resize(dpy->con, width, height);
> >          dpy_gl_scanout_dmabuf(dpy->con, &primary->buf);
> >          free_bufs = true;
> >      }
> > @@ -328,7 +331,7 @@ static void vfio_display_dmabuf_update(void
> *opaque)
> >          cursor->pos_updates = 0;
> >      }
> >
> > -    dpy_gl_update(dpy->con, 0, 0, primary->buf.width, primary->buf.height);
> > +    dpy_gl_update(dpy->con, 0, 0, width, height);
> >
> >      if (free_bufs) {
> >          vfio_display_free_dmabufs(vdev);
> > diff --git a/ui/console.c b/ui/console.c
> > index 43226c5c14..1d0513a733 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -1132,6 +1132,24 @@ void dpy_gl_cursor_position(QemuConsole *con,
> >      }
> >  }
> >
> > +uint32_t dpy_gl_dmabuf_get_width(QemuDmaBuf *dmabuf)
> > +{
> > +    if (dmabuf) {
> > +        return dmabuf->width;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +uint32_t dpy_gl_dmabuf_get_height(QemuDmaBuf *dmabuf)
> > +{
> > +    if (dmabuf) {
> > +        return dmabuf->height;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  void dpy_gl_release_dmabuf(QemuConsole *con,
> >                            QemuDmaBuf *dmabuf)
> >  {
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau