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.
>
> > > +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
