From: Qiang Yu <yuq825@gmail.com>
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 <yuq825@gmail.com>
---
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);
+void qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf, int *fd);
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.0Hi
On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
>
> From: Qiang Yu <yuq825@gmail.com>
>
> 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 <yuq825@gmail.com>
> ---
> 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.
> +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)
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
>
On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
> >
> > From: Qiang Yu <yuq825@gmail.com>
> >
> > 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 <yuq825@gmail.com>
> > ---
> > 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.
> > +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
> >
>
Hi
On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu <yuq825@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi
> >
> > On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
> > >
> > > From: Qiang Yu <yuq825@gmail.com>
> > >
> > > 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 <yuq825@gmail.com>
> > > ---
> > > 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
On Mon, Mar 24, 2025 at 10:06 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu <yuq825@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
> > > >
> > > > From: Qiang Yu <yuq825@gmail.com>
> > > >
> > > > 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 <yuq825@gmail.com>
> > > > ---
> > > > 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
Hi
On Tue, Mar 25, 2025 at 7:26 AM Qiang Yu <yuq825@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 10:06 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu <yuq825@gmail.com> wrote:
> > >
> > > On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> > > <marcandre.lureau@redhat.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
> > > > >
> > > > > From: Qiang Yu <yuq825@gmail.com>
> > > > >
> > > > > 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 <yuq825@gmail.com>
> > > > > ---
> > > > > 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);
Sorry, that is not good enough. Someone reading this declaration will
rightfully expect the function to only set *fd. At the bare minimum,
you would need to document the function to explain what to expect
instead.
Contrast with:
void qemu_dmabuf_dup_fds(QemuDmaBuf *dmabuf, int *fds, size_t n_fds);
Where you explicitly have to deal with an array of fds.
It is similar for the rest of the functions and for consistency.
>
> > >
> > > > > +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
--
Marc-André Lureau
On Tue, Mar 25, 2025 at 2:37 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Tue, Mar 25, 2025 at 7:26 AM Qiang Yu <yuq825@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 10:06 PM Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu <yuq825@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> > > > <marcandre.lureau@redhat.com> wrote:
> > > > >
> > > > > Hi
> > > > >
> > > > > On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
> > > > > >
> > > > > > From: Qiang Yu <yuq825@gmail.com>
> > > > > >
> > > > > > 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 <yuq825@gmail.com>
> > > > > > ---
> > > > > > 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);
>
> Sorry, that is not good enough. Someone reading this declaration will
> rightfully expect the function to only set *fd. At the bare minimum,
> you would need to document the function to explain what to expect
> instead.
>
> Contrast with:
>
> void qemu_dmabuf_dup_fds(QemuDmaBuf *dmabuf, int *fds, size_t n_fds);
>
> Where you explicitly have to deal with an array of fds.
>
> It is similar for the rest of the functions and for consistency.
>
Then is it OK for you that we always use like this?
qemu_dmabuf_dup_fds(dmabuf, fds, 4);
fds = qemu_dmabuf_get_fds(dmabuf, &nfds); /* nfds always equals 4 and unused */
> >
> > > >
> > > > > > +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
>
>
>
> --
> Marc-André Lureau
Hi
On Tue, Mar 25, 2025 at 10:52 AM Qiang Yu <yuq825@gmail.com> wrote:
>
> On Tue, Mar 25, 2025 at 2:37 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Tue, Mar 25, 2025 at 7:26 AM Qiang Yu <yuq825@gmail.com> wrote:
> > >
> > > On Mon, Mar 24, 2025 at 10:06 PM Marc-André Lureau
> > > <marcandre.lureau@gmail.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu <yuq825@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> > > > > <marcandre.lureau@redhat.com> wrote:
> > > > > >
> > > > > > Hi
> > > > > >
> > > > > > On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Qiang Yu <yuq825@gmail.com>
> > > > > > >
> > > > > > > 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 <yuq825@gmail.com>
> > > > > > > ---
> > > > > > > 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);
> >
> > Sorry, that is not good enough. Someone reading this declaration will
> > rightfully expect the function to only set *fd. At the bare minimum,
> > you would need to document the function to explain what to expect
> > instead.
> >
> > Contrast with:
> >
> > void qemu_dmabuf_dup_fds(QemuDmaBuf *dmabuf, int *fds, size_t n_fds);
> >
> > Where you explicitly have to deal with an array of fds.
> >
> > It is similar for the rest of the functions and for consistency.
> >
> Then is it OK for you that we always use like this?
> qemu_dmabuf_dup_fds(dmabuf, fds, 4);
> fds = qemu_dmabuf_get_fds(dmabuf, &nfds); /* nfds always equals 4 and unused */
It's better, with extra assert(nfds == 4) or runtime condition.
>
> > >
> > > > >
> > > > > > > +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
> >
> >
> >
> > --
> > Marc-André Lureau
--
Marc-André Lureau
© 2016 - 2025 Red Hat, Inc.