On Mon, Mar 24, 2025 at 10:06 PM Marc-André Lureau
<[email protected]> wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu <[email protected]> wrote:
> >
> > On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> > <[email protected]> wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 12:19 PM <[email protected]> wrote:
> > > >
> > > > From: Qiang Yu <[email protected]>
> > > >
> > > > mesa/radeonsi is going to support explicit midifier which
> > > > may export a multi-plane texture. For example, texture with
> > > > DCC enabled (a compressed format) has two planes, one with
> > > > compressed data, the other with meta data for compression.
> > > >
> > > > Signed-off-by: Qiang Yu <[email protected]>
> > > > ---
> > > > hw/display/vhost-user-gpu.c | 6 ++-
> > > > hw/display/virtio-gpu-udmabuf.c | 6 ++-
> > > > hw/vfio/display.c | 7 ++--
> > > > include/ui/dmabuf.h | 20 ++++++----
> > > > ui/dbus-listener.c | 10 ++---
> > > > ui/dmabuf.c | 67 +++++++++++++++++++++------------
> > > > ui/egl-helpers.c | 4 +-
> > > > ui/spice-display.c | 4 +-
> > > > 8 files changed, 76 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> > > > index 2aed6243f6..a7949c7078 100644
> > > > --- a/hw/display/vhost-user-gpu.c
> > > > +++ b/hw/display/vhost-user-gpu.c
> > > > @@ -249,6 +249,8 @@ 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);
> > > > + uint32_t offset = 0;
> > > > + uint32_t stride = m->fd_stride;
> > > > uint64_t modifier = 0;
> > > > QemuDmaBuf *dmabuf;
> > > >
> > > > @@ -282,10 +284,10 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> > > > VhostUserGpuMsg *msg)
> > > > }
> > > >
> > > > dmabuf = qemu_dmabuf_new(m->width, m->height,
> > > > - m->fd_stride, 0, 0,
> > > > + &offset, &stride, 0, 0,
> > > > m->fd_width, m->fd_height,
> > > > m->fd_drm_fourcc, modifier,
> > > > - fd, false, m->fd_flags &
> > > > + &fd, 1, false, m->fd_flags &
> > > > VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP);
> > > >
> > > > dpy_gl_scanout_dmabuf(con, dmabuf);
> > > > diff --git a/hw/display/virtio-gpu-udmabuf.c
> > > > b/hw/display/virtio-gpu-udmabuf.c
> > > > index 85ca23cb32..34fbe05b7a 100644
> > > > --- a/hw/display/virtio-gpu-udmabuf.c
> > > > +++ b/hw/display/virtio-gpu-udmabuf.c
> > > > @@ -176,16 +176,18 @@ static VGPUDMABuf
> > > > struct virtio_gpu_rect *r)
> > > > {
> > > > VGPUDMABuf *dmabuf;
> > > > + uint32_t offset = 0;
> > > >
> > > > if (res->dmabuf_fd < 0) {
> > > > return NULL;
> > > > }
> > > >
> > > > dmabuf = g_new0(VGPUDMABuf, 1);
> > > > - dmabuf->buf = qemu_dmabuf_new(r->width, r->height, fb->stride,
> > > > + dmabuf->buf = qemu_dmabuf_new(r->width, r->height,
> > > > + &offset, &fb->stride,
> > > > r->x, r->y, fb->width, fb->height,
> > > >
> > > > qemu_pixman_to_drm_format(fb->format),
> > > > - 0, res->dmabuf_fd, true, false);
> > > > + 0, &res->dmabuf_fd, 1, true, false);
> > > > dmabuf->scanout_id = scanout_id;
> > > > QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
> > > >
> > > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > > > index ea87830fe0..9d882235fb 100644
> > > > --- a/hw/vfio/display.c
> > > > +++ b/hw/vfio/display.c
> > > > @@ -214,6 +214,7 @@ static VFIODMABuf
> > > > *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> > > > struct vfio_device_gfx_plane_info plane;
> > > > VFIODMABuf *dmabuf;
> > > > int fd, ret;
> > > > + uint32_t offset = 0;
> > > >
> > > > memset(&plane, 0, sizeof(plane));
> > > > plane.argsz = sizeof(plane);
> > > > @@ -246,10 +247,10 @@ static VFIODMABuf
> > > > *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> > > >
> > > > dmabuf = g_new0(VFIODMABuf, 1);
> > > > dmabuf->dmabuf_id = plane.dmabuf_id;
> > > > - dmabuf->buf = qemu_dmabuf_new(plane.width, plane.height,
> > > > - plane.stride, 0, 0, plane.width,
> > > > + dmabuf->buf = qemu_dmabuf_new(plane.width, plane.height, &offset,
> > > > + &plane.stride, 0, 0, plane.width,
> > > > plane.height, plane.drm_format,
> > > > - plane.drm_format_mod, fd, false,
> > > > false);
> > > > + plane.drm_format_mod, &fd, 1, false,
> > > > false);
> > > >
> > > > if (plane_type == DRM_PLANE_TYPE_CURSOR) {
> > > > vfio_display_update_cursor(dmabuf, &plane);
> > > > diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h
> > > > index dc74ba895a..407fb2274e 100644
> > > > --- a/include/ui/dmabuf.h
> > > > +++ b/include/ui/dmabuf.h
> > > > @@ -10,24 +10,29 @@
> > > > #ifndef DMABUF_H
> > > > #define DMABUF_H
> > > >
> > > > +#define DMABUF_MAX_PLANES 4
> > > > +
> > > > typedef struct QemuDmaBuf QemuDmaBuf;
> > > >
> > > > QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
> > > > - uint32_t stride, uint32_t x,
> > > > - uint32_t y, uint32_t backing_width,
> > > > - uint32_t backing_height, uint32_t fourcc,
> > > > - uint64_t modifier, int dmabuf_fd,
> > > > + const uint32_t *offset, const uint32_t
> > > > *stride,
> > > > + uint32_t x, uint32_t y,
> > > > + uint32_t backing_width, uint32_t
> > > > backing_height,
> > > > + uint32_t fourcc, uint64_t modifier,
> > > > + const int32_t *dmabuf_fd, uint32_t
> > > > num_planes,
> > > > bool allow_fences, bool y0_top);
> > > > void qemu_dmabuf_free(QemuDmaBuf *dmabuf);
> > > >
> > > > G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuDmaBuf, qemu_dmabuf_free);
> > > >
> > > > -int qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
> > > > -int qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf);
> > > > +const int *qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
> > >
> > > For clarity, I think the functions should be renamed to a plural form:
> > > get_fds, dup_fds, get_strides, get_offsets etc.. I would also have a
> > > size_t *n_fds to return the length of the returned array.
> > >
> > The returned array length is always 4. Valid fds are within num_planes
> > elements. There's no user for n_fds argument.
>
> I understand it is not strictly needed, but it's about being explicit
> for the API, and making it safer for future refactoring as well.
>
Then we have to maintain a record for valid fds just for "explicit for API"?
These APIs are all internal use, we can change them whenever needed.
We should not add an used arg which costs extra maintenance effort.
> >
> > > > +void qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf, int *fd);
> > >
> > > That function should take an n_fds arguments as well (fd being renamed
> > > to fds as well)
> > >
> > We just dup all fds in dmabuf, so no need for n_fds argument. And there's
> > no use for this arg either.
> >
> > > thanks
> > >
> > > > void qemu_dmabuf_close(QemuDmaBuf *dmabuf);
> > > > uint32_t qemu_dmabuf_get_width(QemuDmaBuf *dmabuf);
> > > > uint32_t qemu_dmabuf_get_height(QemuDmaBuf *dmabuf);
> > > > -uint32_t qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf);
> > > > +const uint32_t *qemu_dmabuf_get_offset(QemuDmaBuf *dmabuf);
> > > > +const uint32_t *qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf);
> > > > +uint32_t qemu_dmabuf_get_num_planes(QemuDmaBuf *dmabuf);
> > > > uint32_t qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf);
> > > > uint64_t qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf);
> > > > uint32_t qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf);
> > > > @@ -44,6 +49,5 @@ void qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf,
> > > > uint32_t texture);
> > > > void qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd);
> > > > void qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync);
> > > > void qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool
> > > > draw_submitted);
> > > > -void qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd);
> > > >
> > > > #endif
> > > > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > > > index 51244c9240..3b39a23846 100644
> > > > --- a/ui/dbus-listener.c
> > > > +++ b/ui/dbus-listener.c
> > > > @@ -299,7 +299,7 @@ static void
> > > > dbus_scanout_dmabuf(DisplayChangeListener *dcl,
> > > > uint64_t modifier;
> > > > bool y0_top;
> > > >
> > > > - fd = qemu_dmabuf_get_fd(dmabuf);
> > > > + fd = qemu_dmabuf_get_fd(dmabuf)[0];
> > > > fd_list = g_unix_fd_list_new();
> > > > if (g_unix_fd_list_append(fd_list, fd, &err) != 0) {
> > > > error_report("Failed to setup dmabuf fdlist: %s",
> > > > err->message);
> > > > @@ -310,7 +310,7 @@ static void
> > > > dbus_scanout_dmabuf(DisplayChangeListener *dcl,
> > > >
> > > > width = qemu_dmabuf_get_width(dmabuf);
> > > > height = qemu_dmabuf_get_height(dmabuf);
> > > > - stride = qemu_dmabuf_get_stride(dmabuf);
> > > > + stride = qemu_dmabuf_get_stride(dmabuf)[0];
> > > > fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> > > > modifier = qemu_dmabuf_get_modifier(dmabuf);
> > > > y0_top = qemu_dmabuf_get_y0_top(dmabuf);
> > > > @@ -505,7 +505,7 @@ static void
> > > > dbus_scanout_texture(DisplayChangeListener *dcl,
> > > > #ifdef CONFIG_GBM
> > > > g_autoptr(QemuDmaBuf) dmabuf = NULL;
> > > > int fd;
> > > > - uint32_t stride, fourcc;
> > > > + uint32_t offset = 0, stride, fourcc;
> > > > uint64_t modifier;
> > > >
> > > > assert(tex_id);
> > > > @@ -515,8 +515,8 @@ static void
> > > > dbus_scanout_texture(DisplayChangeListener *dcl,
> > > > error_report("%s: failed to get fd for texture", __func__);
> > > > return;
> > > > }
> > > > - dmabuf = qemu_dmabuf_new(w, h, stride, x, y, backing_width,
> > > > - backing_height, fourcc, modifier, fd,
> > > > + dmabuf = qemu_dmabuf_new(w, h, &offset, &stride, x, y,
> > > > backing_width,
> > > > + backing_height, fourcc, modifier, &fd, 1,
> > > > false, backing_y_0_top);
> > > >
> > > > dbus_scanout_dmabuf(dcl, dmabuf);
> > > > diff --git a/ui/dmabuf.c b/ui/dmabuf.c
> > > > index df7a09703f..c4eb307042 100644
> > > > --- a/ui/dmabuf.c
> > > > +++ b/ui/dmabuf.c
> > > > @@ -11,10 +11,12 @@
> > > > #include "ui/dmabuf.h"
> > > >
> > > > struct QemuDmaBuf {
> > > > - int fd;
> > > > + int fd[DMABUF_MAX_PLANES];
> > > > uint32_t width;
> > > > uint32_t height;
> > > > - uint32_t stride;
> > > > + uint32_t offset[DMABUF_MAX_PLANES];
> > > > + uint32_t stride[DMABUF_MAX_PLANES];
> > > > + uint32_t num_planes;
> > > > uint32_t fourcc;
> > > > uint64_t modifier;
> > > > uint32_t texture;
> > > > @@ -30,28 +32,33 @@ struct QemuDmaBuf {
> > > > };
> > > >
> > > > QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
> > > > - uint32_t stride, uint32_t x,
> > > > - uint32_t y, uint32_t backing_width,
> > > > - uint32_t backing_height, uint32_t fourcc,
> > > > - uint64_t modifier, int32_t dmabuf_fd,
> > > > + const uint32_t *offset, const uint32_t
> > > > *stride,
> > > > + uint32_t x, uint32_t y,
> > > > + uint32_t backing_width, uint32_t
> > > > backing_height,
> > > > + uint32_t fourcc, uint64_t modifier,
> > > > + const int32_t *dmabuf_fd, uint32_t
> > > > num_planes,
> > > > bool allow_fences, bool y0_top) {
> > > > QemuDmaBuf *dmabuf;
> > > >
> > > > + assert(num_planes > 0 && num_planes <= DMABUF_MAX_PLANES);
> > > > +
> > > > dmabuf = g_new0(QemuDmaBuf, 1);
> > > >
> > > > dmabuf->width = width;
> > > > dmabuf->height = height;
> > > > - dmabuf->stride = stride;
> > > > + memcpy(dmabuf->offset, offset, num_planes * sizeof(*offset));
> > > > + memcpy(dmabuf->stride, stride, num_planes * sizeof(*stride));
> > > > dmabuf->x = x;
> > > > dmabuf->y = y;
> > > > dmabuf->backing_width = backing_width;
> > > > dmabuf->backing_height = backing_height;
> > > > dmabuf->fourcc = fourcc;
> > > > dmabuf->modifier = modifier;
> > > > - dmabuf->fd = dmabuf_fd;
> > > > + memcpy(dmabuf->fd, dmabuf_fd, num_planes * sizeof(*dmabuf_fd));
> > > > dmabuf->allow_fences = allow_fences;
> > > > dmabuf->y0_top = y0_top;
> > > > dmabuf->fence_fd = -1;
> > > > + dmabuf->num_planes = num_planes;
> > > >
> > > > return dmabuf;
> > > > }
> > > > @@ -65,31 +72,35 @@ void qemu_dmabuf_free(QemuDmaBuf *dmabuf)
> > > > g_free(dmabuf);
> > > > }
> > > >
> > > > -int qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf)
> > > > +const int *qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf)
> > > > {
> > > > assert(dmabuf != NULL);
> > > >
> > > > return dmabuf->fd;
> > > > }
> > > >
> > > > -int qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf)
> > > > +void qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf, int *fd)
> > > > {
> > > > + int i;
> > > > +
> > > > assert(dmabuf != NULL);
> > > >
> > > > - if (dmabuf->fd >= 0) {
> > > > - return dup(dmabuf->fd);
> > > > - } else {
> > > > - return -1;
> > > > + for (i = 0; i < dmabuf->num_planes; i++) {
> > > > + fd[i] = dmabuf->fd[i] >= 0 ? dup(dmabuf->fd[i]) : -1;
> > > > }
> > > > }
> > > >
> > > > void qemu_dmabuf_close(QemuDmaBuf *dmabuf)
> > > > {
> > > > + int i;
> > > > +
> > > > assert(dmabuf != NULL);
> > > >
> > > > - if (dmabuf->fd >= 0) {
> > > > - close(dmabuf->fd);
> > > > - dmabuf->fd = -1;
> > > > + for (i = 0; i < dmabuf->num_planes; i++) {
> > > > + if (dmabuf->fd[i] >= 0) {
> > > > + close(dmabuf->fd[i]);
> > > > + dmabuf->fd[i] = -1;
> > > > + }
> > > > }
> > > > }
> > > >
> > > > @@ -107,13 +118,27 @@ uint32_t qemu_dmabuf_get_height(QemuDmaBuf
> > > > *dmabuf)
> > > > return dmabuf->height;
> > > > }
> > > >
> > > > -uint32_t qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf)
> > > > +const uint32_t *qemu_dmabuf_get_offset(QemuDmaBuf *dmabuf)
> > > > +{
> > > > + assert(dmabuf != NULL);
> > > > +
> > > > + return dmabuf->offset;
> > > > +}
> > > > +
> > > > +const uint32_t *qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf)
> > > > {
> > > > assert(dmabuf != NULL);
> > > >
> > > > return dmabuf->stride;
> > > > }
> > > >
> > > > +uint32_t qemu_dmabuf_get_num_planes(QemuDmaBuf *dmabuf)
> > > > +{
> > > > + assert(dmabuf != NULL);
> > > > +
> > > > + return dmabuf->num_planes;
> > > > +}
> > > > +
> > > > uint32_t qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf)
> > > > {
> > > > assert(dmabuf != NULL);
> > > > @@ -221,9 +246,3 @@ void qemu_dmabuf_set_draw_submitted(QemuDmaBuf
> > > > *dmabuf, bool draw_submitted)
> > > > assert(dmabuf != NULL);
> > > > dmabuf->draw_submitted = draw_submitted;
> > > > }
> > > > -
> > > > -void qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd)
> > > > -{
> > > > - assert(dmabuf != NULL);
> > > > - dmabuf->fd = fd;
> > > > -}
> > > > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > > > index d591159480..72a1405782 100644
> > > > --- a/ui/egl-helpers.c
> > > > +++ b/ui/egl-helpers.c
> > > > @@ -323,9 +323,9 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > > attrs[i++] = qemu_dmabuf_get_fourcc(dmabuf);
> > > >
> > > > attrs[i++] = EGL_DMA_BUF_PLANE0_FD_EXT;
> > > > - attrs[i++] = qemu_dmabuf_get_fd(dmabuf);
> > > > + attrs[i++] = qemu_dmabuf_get_fd(dmabuf)[0];
> > > > attrs[i++] = EGL_DMA_BUF_PLANE0_PITCH_EXT;
> > > > - attrs[i++] = qemu_dmabuf_get_stride(dmabuf);
> > > > + attrs[i++] = qemu_dmabuf_get_stride(dmabuf)[0];
> > > > attrs[i++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
> > > > attrs[i++] = 0;
> > > > #ifdef EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT
> > > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > > index c794ae0649..82598fe9e8 100644
> > > > --- a/ui/spice-display.c
> > > > +++ b/ui/spice-display.c
> > > > @@ -1075,10 +1075,10 @@ static void
> > > > qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > stride, fourcc, false);
> > > > }
> > > > } else {
> > > > - stride = qemu_dmabuf_get_stride(dmabuf);
> > > > + stride = qemu_dmabuf_get_stride(dmabuf)[0];
> > > > fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> > > > y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> > > > - fd = qemu_dmabuf_dup_fd(dmabuf);
> > > > + qemu_dmabuf_dup_fd(dmabuf, &fd);
> > > >
> > > > trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width,
> > > > height);
> > > > /* note: spice server will close the fd, so hand over a
> > > > dup */
> > > > --
> > > > 2.43.0
> > > >
> > >
> >
>
>
> --
> Marc-André Lureau
