* [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-24 8:19 [PATCH 0/6] ui: support multi plane texture yuq825
@ 2025-03-24 8:19 ` yuq825
2025-03-24 10:04 ` Marc-André Lureau
2025-03-24 8:19 ` [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers yuq825
` (4 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: yuq825 @ 2025-03-24 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Qiang Yu
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.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-24 8:19 ` [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane yuq825
@ 2025-03-24 10:04 ` Marc-André Lureau
2025-03-24 13:16 ` Qiang Yu
0 siblings, 1 reply; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 10:04 UTC (permalink / raw)
To: yuq825; +Cc: qemu-devel
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.
> +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
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-24 10:04 ` Marc-André Lureau
@ 2025-03-24 13:16 ` Qiang Yu
2025-03-24 14:06 ` Marc-André Lureau
0 siblings, 1 reply; 25+ messages in thread
From: Qiang Yu @ 2025-03-24 13:16 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
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
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-24 13:16 ` Qiang Yu
@ 2025-03-24 14:06 ` Marc-André Lureau
2025-03-25 3:26 ` Qiang Yu
0 siblings, 1 reply; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 14:06 UTC (permalink / raw)
To: Qiang Yu; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-24 14:06 ` Marc-André Lureau
@ 2025-03-25 3:26 ` Qiang Yu
2025-03-25 6:37 ` Marc-André Lureau
0 siblings, 1 reply; 25+ messages in thread
From: Qiang Yu @ 2025-03-25 3:26 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-25 3:26 ` Qiang Yu
@ 2025-03-25 6:37 ` Marc-André Lureau
2025-03-25 6:52 ` Qiang Yu
0 siblings, 1 reply; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-25 6:37 UTC (permalink / raw)
To: Qiang Yu; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-25 6:37 ` Marc-André Lureau
@ 2025-03-25 6:52 ` Qiang Yu
2025-03-25 7:22 ` Marc-André Lureau
0 siblings, 1 reply; 25+ messages in thread
From: Qiang Yu @ 2025-03-25 6:52 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
2025-03-25 6:52 ` Qiang Yu
@ 2025-03-25 7:22 ` Marc-André Lureau
0 siblings, 0 replies; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-25 7:22 UTC (permalink / raw)
To: Qiang Yu; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
2025-03-24 8:19 [PATCH 0/6] ui: support multi plane texture yuq825
2025-03-24 8:19 ` [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane yuq825
@ 2025-03-24 8:19 ` yuq825
2025-03-24 10:09 ` Marc-André Lureau
2025-03-24 8:19 ` [PATCH 3/6] ui/egl: use DRM_FORMAT_MOD_INVALID as default modifier yuq825
` (3 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: yuq825 @ 2025-03-24 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Qiang Yu
From: Qiang Yu <yuq825@gmail.com>
It's used already, just check it explicitly.
Signed-off-by: Qiang Yu <yuq825@gmail.com>
---
ui/egl-helpers.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 72a1405782..45b1b0b700 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode, DisplayGLMode mode)
error_report("egl: EGL_MESA_image_dma_buf_export not supported");
goto err;
}
+ if (!epoxy_has_egl_extension(qemu_egl_display,
+ "EGL_EXT_image_dma_buf_import_modifiers")) {
+ error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not supported");
+ goto err;
+ }
qemu_egl_rn_ctx = qemu_egl_init_ctx();
if (!qemu_egl_rn_ctx) {
@@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
EGLImageKHR image = EGL_NO_IMAGE_KHR;
EGLint attrs[64];
int i = 0;
- uint64_t modifier;
+ uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
if (texture != 0) {
@@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *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
- modifier = qemu_dmabuf_get_modifier(dmabuf);
if (modifier) {
attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT;
attrs[i++] = (modifier >> 0) & 0xffffffff;
attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
attrs[i++] = (modifier >> 32) & 0xffffffff;
}
-#endif
attrs[i++] = EGL_NONE;
image = eglCreateImageKHR(qemu_egl_display,
--
2.43.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
2025-03-24 8:19 ` [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers yuq825
@ 2025-03-24 10:09 ` Marc-André Lureau
2025-03-24 13:22 ` Qiang Yu
0 siblings, 1 reply; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 10:09 UTC (permalink / raw)
To: yuq825; +Cc: qemu-devel
Hi
On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
>
> From: Qiang Yu <yuq825@gmail.com>
>
> It's used already, just check it explicitly.
>
> Signed-off-by: Qiang Yu <yuq825@gmail.com>
> ---
> ui/egl-helpers.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> index 72a1405782..45b1b0b700 100644
> --- a/ui/egl-helpers.c
> +++ b/ui/egl-helpers.c
> @@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode, DisplayGLMode mode)
> error_report("egl: EGL_MESA_image_dma_buf_export not supported");
> goto err;
> }
> + if (!epoxy_has_egl_extension(qemu_egl_display,
> + "EGL_EXT_image_dma_buf_import_modifiers")) {
> + error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not supported");
> + goto err;
> + }
>
> qemu_egl_rn_ctx = qemu_egl_init_ctx();
> if (!qemu_egl_rn_ctx) {
> @@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> EGLImageKHR image = EGL_NO_IMAGE_KHR;
> EGLint attrs[64];
> int i = 0;
> - uint64_t modifier;
> + uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
> uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
>
> if (texture != 0) {
> @@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *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
We should check for that define during meson.build when gbm.found(),
to avoid potential later build errors.
> - modifier = qemu_dmabuf_get_modifier(dmabuf);
> if (modifier) {
> attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT;
> attrs[i++] = (modifier >> 0) & 0xffffffff;
> attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
> attrs[i++] = (modifier >> 32) & 0xffffffff;
> }
> -#endif
> attrs[i++] = EGL_NONE;
>
> image = eglCreateImageKHR(qemu_egl_display,
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
2025-03-24 10:09 ` Marc-André Lureau
@ 2025-03-24 13:22 ` Qiang Yu
2025-03-24 13:45 ` Marc-André Lureau
0 siblings, 1 reply; 25+ messages in thread
From: Qiang Yu @ 2025-03-24 13:22 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
On Mon, Mar 24, 2025 at 6:09 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>
> >
> > It's used already, just check it explicitly.
> >
> > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > ---
> > ui/egl-helpers.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > index 72a1405782..45b1b0b700 100644
> > --- a/ui/egl-helpers.c
> > +++ b/ui/egl-helpers.c
> > @@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode, DisplayGLMode mode)
> > error_report("egl: EGL_MESA_image_dma_buf_export not supported");
> > goto err;
> > }
> > + if (!epoxy_has_egl_extension(qemu_egl_display,
> > + "EGL_EXT_image_dma_buf_import_modifiers")) {
> > + error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not supported");
> > + goto err;
> > + }
> >
> > qemu_egl_rn_ctx = qemu_egl_init_ctx();
> > if (!qemu_egl_rn_ctx) {
> > @@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > EGLImageKHR image = EGL_NO_IMAGE_KHR;
> > EGLint attrs[64];
> > int i = 0;
> > - uint64_t modifier;
> > + uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
> > uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
> >
> > if (texture != 0) {
> > @@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *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
>
> We should check for that define during meson.build when gbm.found(),
> to avoid potential later build errors.
>
This macro should be in EGL header for many years as this is a 2015 EGL
extension. Check the macro in meson.build is not necessary now like you
won't check all other EGL macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT.
> > - modifier = qemu_dmabuf_get_modifier(dmabuf);
> > if (modifier) {
> > attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT;
> > attrs[i++] = (modifier >> 0) & 0xffffffff;
> > attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
> > attrs[i++] = (modifier >> 32) & 0xffffffff;
> > }
> > -#endif
> > attrs[i++] = EGL_NONE;
> >
> > image = eglCreateImageKHR(qemu_egl_display,
> > --
> > 2.43.0
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
2025-03-24 13:22 ` Qiang Yu
@ 2025-03-24 13:45 ` Marc-André Lureau
2025-03-25 3:16 ` Qiang Yu
0 siblings, 1 reply; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 13:45 UTC (permalink / raw)
To: Qiang Yu; +Cc: qemu-devel
Hi
On Mon, Mar 24, 2025 at 5:27 PM Qiang Yu <yuq825@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 6:09 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>
> > >
> > > It's used already, just check it explicitly.
> > >
> > > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > > ---
> > > ui/egl-helpers.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > > index 72a1405782..45b1b0b700 100644
> > > --- a/ui/egl-helpers.c
> > > +++ b/ui/egl-helpers.c
> > > @@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode, DisplayGLMode mode)
> > > error_report("egl: EGL_MESA_image_dma_buf_export not supported");
> > > goto err;
> > > }
> > > + if (!epoxy_has_egl_extension(qemu_egl_display,
> > > + "EGL_EXT_image_dma_buf_import_modifiers")) {
> > > + error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not supported");
> > > + goto err;
> > > + }
> > >
> > > qemu_egl_rn_ctx = qemu_egl_init_ctx();
> > > if (!qemu_egl_rn_ctx) {
> > > @@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > EGLImageKHR image = EGL_NO_IMAGE_KHR;
> > > EGLint attrs[64];
> > > int i = 0;
> > > - uint64_t modifier;
> > > + uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
> > > uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
> > >
> > > if (texture != 0) {
> > > @@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *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
> >
> > We should check for that define during meson.build when gbm.found(),
> > to avoid potential later build errors.
> >
> This macro should be in EGL header for many years as this is a 2015 EGL
> extension. Check the macro in meson.build is not necessary now like you
> won't check all other EGL macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT.
Imho we should still check for those macros, as they are extensions to egl.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
2025-03-24 13:45 ` Marc-André Lureau
@ 2025-03-25 3:16 ` Qiang Yu
0 siblings, 0 replies; 25+ messages in thread
From: Qiang Yu @ 2025-03-25 3:16 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
On Mon, Mar 24, 2025 at 9:45 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 5:27 PM Qiang Yu <yuq825@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 6:09 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>
> > > >
> > > > It's used already, just check it explicitly.
> > > >
> > > > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > > > ---
> > > > ui/egl-helpers.c | 10 ++++++----
> > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > > > index 72a1405782..45b1b0b700 100644
> > > > --- a/ui/egl-helpers.c
> > > > +++ b/ui/egl-helpers.c
> > > > @@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode, DisplayGLMode mode)
> > > > error_report("egl: EGL_MESA_image_dma_buf_export not supported");
> > > > goto err;
> > > > }
> > > > + if (!epoxy_has_egl_extension(qemu_egl_display,
> > > > + "EGL_EXT_image_dma_buf_import_modifiers")) {
> > > > + error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not supported");
> > > > + goto err;
> > > > + }
> > > >
> > > > qemu_egl_rn_ctx = qemu_egl_init_ctx();
> > > > if (!qemu_egl_rn_ctx) {
> > > > @@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > > EGLImageKHR image = EGL_NO_IMAGE_KHR;
> > > > EGLint attrs[64];
> > > > int i = 0;
> > > > - uint64_t modifier;
> > > > + uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
> > > > uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
> > > >
> > > > if (texture != 0) {
> > > > @@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *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
> > >
> > > We should check for that define during meson.build when gbm.found(),
> > > to avoid potential later build errors.
> > >
> > This macro should be in EGL header for many years as this is a 2015 EGL
> > extension. Check the macro in meson.build is not necessary now like you
> > won't check all other EGL macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT.
>
> Imho we should still check for those macros, as they are extensions to egl.
>
Macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT are also from EGL extension
but we don't check them. EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT makes
no difference. And we do need this macro to be always present to make
sure exporter
and importer of the texture to work correctly.
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/6] ui/egl: use DRM_FORMAT_MOD_INVALID as default modifier
2025-03-24 8:19 [PATCH 0/6] ui: support multi plane texture yuq825
2025-03-24 8:19 ` [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane yuq825
2025-03-24 8:19 ` [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers yuq825
@ 2025-03-24 8:19 ` yuq825
2025-03-24 10:15 ` Marc-André Lureau
2025-03-24 8:19 ` [PATCH 4/6] ui/egl: support multi-plane dmabuf when egl export/import yuq825
` (2 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: yuq825 @ 2025-03-24 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Qiang Yu
From: Qiang Yu <yuq825@gmail.com>
0 is used as DRM_FORMAT_MOD_LINEAR already.
Signed-off-by: Qiang Yu <yuq825@gmail.com>
---
hw/display/vhost-user-gpu.c | 3 ++-
hw/display/virtio-gpu-udmabuf.c | 4 +++-
ui/egl-helpers.c | 3 ++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index a7949c7078..a6a510db65 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -18,6 +18,7 @@
#include "chardev/char-fe.h"
#include "qapi/error.h"
#include "migration/blocker.h"
+#include "standard-headers/drm/drm_fourcc.h"
typedef enum VhostUserGpuRequest {
VHOST_USER_GPU_NONE = 0,
@@ -251,7 +252,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
uint32_t offset = 0;
uint32_t stride = m->fd_stride;
- uint64_t modifier = 0;
+ uint64_t modifier = DRM_FORMAT_MOD_INVALID;
QemuDmaBuf *dmabuf;
if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index 34fbe05b7a..de6ce53f16 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -25,6 +25,7 @@
#include <linux/memfd.h>
#include "qemu/memfd.h"
#include "standard-headers/linux/udmabuf.h"
+#include "standard-headers/drm/drm_fourcc.h"
static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
{
@@ -187,7 +188,8 @@ static VGPUDMABuf
&offset, &fb->stride,
r->x, r->y, fb->width, fb->height,
qemu_pixman_to_drm_format(fb->format),
- 0, &res->dmabuf_fd, 1, true, false);
+ DRM_FORMAT_MOD_INVALID, &res->dmabuf_fd,
+ 1, true, false);
dmabuf->scanout_id = scanout_id;
QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 45b1b0b700..970286325f 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -23,6 +23,7 @@
#include "system/system.h"
#include "qapi/error.h"
#include "trace.h"
+#include "standard-headers/drm/drm_fourcc.h"
EGLDisplay *qemu_egl_display;
EGLConfig qemu_egl_config;
@@ -333,7 +334,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
attrs[i++] = qemu_dmabuf_get_stride(dmabuf)[0];
attrs[i++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
attrs[i++] = 0;
- if (modifier) {
+ if (modifier != DRM_FORMAT_MOD_INVALID) {
attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT;
attrs[i++] = (modifier >> 0) & 0xffffffff;
attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
--
2.43.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] ui/egl: use DRM_FORMAT_MOD_INVALID as default modifier
2025-03-24 8:19 ` [PATCH 3/6] ui/egl: use DRM_FORMAT_MOD_INVALID as default modifier yuq825
@ 2025-03-24 10:15 ` Marc-André Lureau
0 siblings, 0 replies; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 10:15 UTC (permalink / raw)
To: yuq825; +Cc: qemu-devel
On Mon, Mar 24, 2025 at 12:19 PM <yuq825@gmail.com> wrote:
>
> From: Qiang Yu <yuq825@gmail.com>
>
> 0 is used as DRM_FORMAT_MOD_LINEAR already.
>
> Signed-off-by: Qiang Yu <yuq825@gmail.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/display/vhost-user-gpu.c | 3 ++-
> hw/display/virtio-gpu-udmabuf.c | 4 +++-
> ui/egl-helpers.c | 3 ++-
> 3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index a7949c7078..a6a510db65 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -18,6 +18,7 @@
> #include "chardev/char-fe.h"
> #include "qapi/error.h"
> #include "migration/blocker.h"
> +#include "standard-headers/drm/drm_fourcc.h"
>
> typedef enum VhostUserGpuRequest {
> VHOST_USER_GPU_NONE = 0,
> @@ -251,7 +252,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
> int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
> uint32_t offset = 0;
> uint32_t stride = m->fd_stride;
> - uint64_t modifier = 0;
> + uint64_t modifier = DRM_FORMAT_MOD_INVALID;
> QemuDmaBuf *dmabuf;
>
> if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> index 34fbe05b7a..de6ce53f16 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -25,6 +25,7 @@
> #include <linux/memfd.h>
> #include "qemu/memfd.h"
> #include "standard-headers/linux/udmabuf.h"
> +#include "standard-headers/drm/drm_fourcc.h"
>
> static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
> {
> @@ -187,7 +188,8 @@ static VGPUDMABuf
> &offset, &fb->stride,
> r->x, r->y, fb->width, fb->height,
> qemu_pixman_to_drm_format(fb->format),
> - 0, &res->dmabuf_fd, 1, true, false);
> + DRM_FORMAT_MOD_INVALID, &res->dmabuf_fd,
> + 1, true, false);
> dmabuf->scanout_id = scanout_id;
> QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
>
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> index 45b1b0b700..970286325f 100644
> --- a/ui/egl-helpers.c
> +++ b/ui/egl-helpers.c
> @@ -23,6 +23,7 @@
> #include "system/system.h"
> #include "qapi/error.h"
> #include "trace.h"
> +#include "standard-headers/drm/drm_fourcc.h"
>
> EGLDisplay *qemu_egl_display;
> EGLConfig qemu_egl_config;
> @@ -333,7 +334,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> attrs[i++] = qemu_dmabuf_get_stride(dmabuf)[0];
> attrs[i++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
> attrs[i++] = 0;
> - if (modifier) {
> + if (modifier != DRM_FORMAT_MOD_INVALID) {
> attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT;
> attrs[i++] = (modifier >> 0) & 0xffffffff;
> attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/6] ui/egl: support multi-plane dmabuf when egl export/import
2025-03-24 8:19 [PATCH 0/6] ui: support multi plane texture yuq825
` (2 preceding siblings ...)
2025-03-24 8:19 ` [PATCH 3/6] ui/egl: use DRM_FORMAT_MOD_INVALID as default modifier yuq825
@ 2025-03-24 8:19 ` yuq825
2025-03-24 8:19 ` [PATCH 5/6] ui/dbus: change dbus ScanoutDMABUF interface yuq825
2025-03-24 8:19 ` [PATCH 6/6] ui/spice: support multi plane dmabuf scanout yuq825
5 siblings, 0 replies; 25+ messages in thread
From: yuq825 @ 2025-03-24 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Qiang Yu
From: Qiang Yu <yuq825@gmail.com>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
---
include/ui/egl-helpers.h | 5 ++-
ui/dbus-listener.c | 19 +++++----
ui/egl-helpers.c | 86 +++++++++++++++++++++++++++++-----------
ui/spice-display.c | 58 ++++++++++++++++++---------
4 files changed, 116 insertions(+), 52 deletions(-)
diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
index 4b8c0d2281..fb80e15142 100644
--- a/include/ui/egl-helpers.h
+++ b/include/ui/egl-helpers.h
@@ -46,8 +46,9 @@ extern int qemu_egl_rn_fd;
extern struct gbm_device *qemu_egl_rn_gbm_dev;
int egl_rendernode_init(const char *rendernode, DisplayGLMode mode);
-int egl_get_fd_for_texture(uint32_t tex_id, EGLint *stride, EGLint *fourcc,
- EGLuint64KHR *modifier);
+bool egl_dmabuf_export_texture(uint32_t tex_id, int *fd, EGLint *offset,
+ EGLint *stride, EGLint *fourcc, int *num_planes,
+ EGLuint64KHR *modifier);
void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf);
void egl_dmabuf_release_texture(QemuDmaBuf *dmabuf);
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 3b39a23846..73b72ed57c 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -504,19 +504,22 @@ static void dbus_scanout_texture(DisplayChangeListener *dcl,
backing_width, backing_height, x, y, w, h);
#ifdef CONFIG_GBM
g_autoptr(QemuDmaBuf) dmabuf = NULL;
- int fd;
- uint32_t offset = 0, stride, fourcc;
+ int fd[DMABUF_MAX_PLANES], num_planes;
+ uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc;
uint64_t modifier;
assert(tex_id);
- fd = egl_get_fd_for_texture(tex_id, (EGLint *)&stride, (EGLint *)&fourcc,
- &modifier);
- if (fd < 0) {
- error_report("%s: failed to get fd for texture", __func__);
+ if (!egl_dmabuf_export_texture(tex_id, fd, (EGLint *)offset, (EGLint *)stride,
+ (EGLint *)&fourcc, &num_planes, &modifier)) {
+ error_report("%s: failed to export dmabuf for texture", __func__);
+ return;
+ }
+ if (num_planes > 1) {
+ error_report("%s: does not support multi-plane dmabuf", __func__);
return;
}
- dmabuf = qemu_dmabuf_new(w, h, &offset, &stride, x, y, backing_width,
- backing_height, fourcc, modifier, &fd, 1,
+ dmabuf = qemu_dmabuf_new(w, h, offset, stride, x, y, backing_width,
+ backing_height, fourcc, modifier, fd, num_planes,
false, backing_y_0_top);
dbus_scanout_dmabuf(dcl, dmabuf);
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 970286325f..864855f540 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -283,39 +283,75 @@ err:
return -1;
}
-int egl_get_fd_for_texture(uint32_t tex_id, EGLint *stride, EGLint *fourcc,
- EGLuint64KHR *modifier)
+bool egl_dmabuf_export_texture(uint32_t tex_id, int *fd, EGLint *offset,
+ EGLint *stride, EGLint *fourcc, int *num_planes,
+ EGLuint64KHR *modifier)
{
EGLImageKHR image;
- EGLint num_planes, fd;
+ EGLuint64KHR modifiers[DMABUF_MAX_PLANES];
image = eglCreateImageKHR(qemu_egl_display, eglGetCurrentContext(),
EGL_GL_TEXTURE_2D_KHR,
(EGLClientBuffer)(unsigned long)tex_id,
NULL);
if (!image) {
- return -1;
+ return false;
}
eglExportDMABUFImageQueryMESA(qemu_egl_display, image, fourcc,
- &num_planes, modifier);
- if (num_planes != 1) {
- eglDestroyImageKHR(qemu_egl_display, image);
- return -1;
- }
- eglExportDMABUFImageMESA(qemu_egl_display, image, &fd, stride, NULL);
+ num_planes, modifiers);
+ eglExportDMABUFImageMESA(qemu_egl_display, image, fd, stride, offset);
eglDestroyImageKHR(qemu_egl_display, image);
- return fd;
+ /* Only first modifier matters. */
+ if (modifier)
+ *modifier = modifiers[0];
+
+ return true;
}
void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
{
EGLImageKHR image = EGL_NO_IMAGE_KHR;
EGLint attrs[64];
- int i = 0;
+ int i = 0, j;
uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
+ const int *fd = qemu_dmabuf_get_fd(dmabuf);
+ const uint32_t *offset = qemu_dmabuf_get_offset(dmabuf);
+ const uint32_t *stride = qemu_dmabuf_get_stride(dmabuf);
+ uint32_t num_planes = qemu_dmabuf_get_num_planes(dmabuf);
+
+ EGLint fd_attrs[] = {
+ EGL_DMA_BUF_PLANE0_FD_EXT,
+ EGL_DMA_BUF_PLANE1_FD_EXT,
+ EGL_DMA_BUF_PLANE2_FD_EXT,
+ EGL_DMA_BUF_PLANE3_FD_EXT,
+ };
+ EGLint offset_attrs[] = {
+ EGL_DMA_BUF_PLANE0_OFFSET_EXT,
+ EGL_DMA_BUF_PLANE1_OFFSET_EXT,
+ EGL_DMA_BUF_PLANE2_OFFSET_EXT,
+ EGL_DMA_BUF_PLANE3_OFFSET_EXT,
+ };
+ EGLint stride_attrs[] = {
+ EGL_DMA_BUF_PLANE0_PITCH_EXT,
+ EGL_DMA_BUF_PLANE1_PITCH_EXT,
+ EGL_DMA_BUF_PLANE2_PITCH_EXT,
+ EGL_DMA_BUF_PLANE3_PITCH_EXT,
+ };
+ EGLint modifier_lo_attrs[] = {
+ EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT,
+ EGL_DMA_BUF_PLANE1_MODIFIER_LO_EXT,
+ EGL_DMA_BUF_PLANE2_MODIFIER_LO_EXT,
+ EGL_DMA_BUF_PLANE3_MODIFIER_LO_EXT,
+ };
+ EGLint modifier_hi_attrs[] = {
+ EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT,
+ EGL_DMA_BUF_PLANE1_MODIFIER_HI_EXT,
+ EGL_DMA_BUF_PLANE2_MODIFIER_HI_EXT,
+ EGL_DMA_BUF_PLANE3_MODIFIER_HI_EXT,
+ };
if (texture != 0) {
return;
@@ -328,18 +364,22 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
attrs[i++] = EGL_LINUX_DRM_FOURCC_EXT;
attrs[i++] = qemu_dmabuf_get_fourcc(dmabuf);
- attrs[i++] = EGL_DMA_BUF_PLANE0_FD_EXT;
- attrs[i++] = qemu_dmabuf_get_fd(dmabuf)[0];
- attrs[i++] = EGL_DMA_BUF_PLANE0_PITCH_EXT;
- attrs[i++] = qemu_dmabuf_get_stride(dmabuf)[0];
- attrs[i++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
- attrs[i++] = 0;
- if (modifier != DRM_FORMAT_MOD_INVALID) {
- attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT;
- attrs[i++] = (modifier >> 0) & 0xffffffff;
- attrs[i++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
- attrs[i++] = (modifier >> 32) & 0xffffffff;
+ for (j = 0; j < num_planes; j++) {
+ attrs[i++] = fd_attrs[j];
+ /* fd[1-3] may be -1 if using a joint buffer for all planes */
+ attrs[i++] = fd[j] >= 0 ? fd[j] : fd[0];
+ attrs[i++] = stride_attrs[j];
+ attrs[i++] = stride[j];
+ attrs[i++] = offset_attrs[j];
+ attrs[i++] = offset[j];
+ if (modifier != DRM_FORMAT_MOD_INVALID) {
+ attrs[i++] = modifier_lo_attrs[j];
+ attrs[i++] = (modifier >> 0) & 0xffffffff;
+ attrs[i++] = modifier_hi_attrs[j];
+ attrs[i++] = (modifier >> 32) & 0xffffffff;
+ }
}
+
attrs[i++] = EGL_NONE;
image = eglCreateImageKHR(qemu_egl_display,
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 82598fe9e8..b7016ece6a 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -876,19 +876,24 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
struct DisplaySurface *new_surface)
{
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
- EGLint stride, fourcc;
- int fd;
if (ssd->ds) {
surface_gl_destroy_texture(ssd->gls, ssd->ds);
}
ssd->ds = new_surface;
if (ssd->ds) {
+ uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
+ int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
+
surface_gl_create_texture(ssd->gls, ssd->ds);
- fd = egl_get_fd_for_texture(ssd->ds->texture,
- &stride, &fourcc,
- NULL);
- if (fd < 0) {
+ if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
+ (EGLint *)stride, &fourcc, &num_planes, NULL)) {
+ surface_gl_destroy_texture(ssd->gls, ssd->ds);
+ return;
+ }
+
+ if (num_planes > 1) {
+ fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
surface_gl_destroy_texture(ssd->gls, ssd->ds);
return;
}
@@ -899,10 +904,10 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
fourcc);
/* note: spice server will close the fd */
- spice_qxl_gl_scanout(&ssd->qxl, fd,
+ spice_qxl_gl_scanout(&ssd->qxl, fd[0],
surface_width(ssd->ds),
surface_height(ssd->ds),
- stride, fourcc, false);
+ stride[0], fourcc, false);
ssd->have_surface = true;
ssd->have_scanout = false;
@@ -941,20 +946,24 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
void *d3d_tex2d)
{
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
- EGLint stride = 0, fourcc = 0;
- int fd = -1;
+ EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
+ int fd[DMABUF_MAX_PLANES], num_planes;
assert(tex_id);
- fd = egl_get_fd_for_texture(tex_id, &stride, &fourcc, NULL);
- if (fd < 0) {
- fprintf(stderr, "%s: failed to get fd for texture\n", __func__);
+ if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
+ &num_planes, NULL)) {
+ fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
+ return;
+ }
+ if (num_planes > 1) {
+ fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
return;
}
trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
/* note: spice server will close the fd */
- spice_qxl_gl_scanout(&ssd->qxl, fd, backing_width, backing_height,
- stride, fourcc, y_0_top);
+ spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
+ stride[0], fourcc, y_0_top);
qemu_spice_gl_monitor_config(ssd, x, y, w, h);
ssd->have_surface = false;
ssd->have_scanout = true;
@@ -1064,15 +1073,26 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
/* dest framebuffer */
if (ssd->blit_fb.width != width ||
ssd->blit_fb.height != height) {
+ int fds[DMABUF_MAX_PLANES], num_planes;
+ uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
+
trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
height);
egl_fb_destroy(&ssd->blit_fb);
egl_fb_setup_new_tex(&ssd->blit_fb,
width, height);
- fd = egl_get_fd_for_texture(ssd->blit_fb.texture,
- &stride, &fourcc, NULL);
- spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
- stride, fourcc, false);
+ if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
+ (EGLint *)offsets, (EGLint *)strides,
+ &fourcc, &num_planes, NULL)) {
+ fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
+ return;
+ }
+ if (num_planes > 1) {
+ fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
+ return;
+ }
+ spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
+ strides[0], fourcc, false);
}
} else {
stride = qemu_dmabuf_get_stride(dmabuf)[0];
--
2.43.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 5/6] ui/dbus: change dbus ScanoutDMABUF interface
2025-03-24 8:19 [PATCH 0/6] ui: support multi plane texture yuq825
` (3 preceding siblings ...)
2025-03-24 8:19 ` [PATCH 4/6] ui/egl: support multi-plane dmabuf when egl export/import yuq825
@ 2025-03-24 8:19 ` yuq825
2025-03-24 8:19 ` [PATCH 6/6] ui/spice: support multi plane dmabuf scanout yuq825
5 siblings, 0 replies; 25+ messages in thread
From: yuq825 @ 2025-03-24 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Qiang Yu
From: Qiang Yu <yuq825@gmail.com>
To handle multi plane.
Signed-off-by: Qiang Yu <yuq825@gmail.com>
---
ui/dbus-display1.xml | 37 ++++++++++++++++
ui/dbus-listener.c | 103 ++++++++++++++++++++++++++++++++++++++++---
2 files changed, 133 insertions(+), 7 deletions(-)
diff --git a/ui/dbus-display1.xml b/ui/dbus-display1.xml
index 72deefa455..c1d1a402b7 100644
--- a/ui/dbus-display1.xml
+++ b/ui/dbus-display1.xml
@@ -614,6 +614,43 @@
</method>
</interface>
+ <!--
+ org.qemu.Display1.Listener.Unix.MultiPlane:
+
+ This optional client-side interface can complement
+ org.qemu.Display1.Listener on ``/org/qemu/Display1/Listener`` for
+ Unix-specific multi plane DMABUF scanout setup.
+ -->
+ <?if $(env.HOST_OS) != windows?>
+ <interface name="org.qemu.Display1.Listener.Unix.MultiPlane">
+ <!--
+ ScanoutDMABUF2:
+ @dmabuf: DMABUF file descriptor of each plane.
+ @width: display width, in pixels.
+ @height: display height, in pixels.
+ @offset: offset of each plane, in bytes.
+ @stride: stride of each plane, in bytes.
+ @num_planes: plane number.
+ @fourcc: DMABUF fourcc.
+ @modifier: DMABUF modifier.
+ @y0_top: whether Y position 0 is the top or not.
+
+ Resize and update the display content with DMABUF.
+ -->
+ <method name="ScanoutDMABUF2">
+ <arg type="ah" name="dmabuf" direction="in"/>
+ <arg type="u" name="width" direction="in"/>
+ <arg type="u" name="height" direction="in"/>
+ <arg type="au" name="offset" direction="in"/>
+ <arg type="au" name="stride" direction="in"/>
+ <arg type="u" name="num_planes" direction="in"/>
+ <arg type="u" name="fourcc" direction="in"/>
+ <arg type="t" name="modifier" direction="in"/>
+ <arg type="b" name="y0_top" direction="in"/>
+ </method>
+ </interface>
+ <?endif?>
+
<!--
org.qemu.Display1.Clipboard:
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 73b72ed57c..b7b455a595 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -85,6 +85,7 @@ struct _DBusDisplayListener {
#endif
#else /* !WIN32 */
QemuDBusDisplay1ListenerUnixMap *map_proxy;
+ QemuDBusDisplay1ListenerUnixMultiPlane *multi_plane_proxy;
#endif
guint dbus_filter;
@@ -288,10 +289,9 @@ static void dbus_call_update_gl(DisplayChangeListener *dcl,
}
#ifdef CONFIG_GBM
-static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
- QemuDmaBuf *dmabuf)
+static void dbus_scanout_dmabuf_single_plane(DBusDisplayListener *ddl,
+ QemuDmaBuf *dmabuf)
{
- DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
g_autoptr(GError) err = NULL;
g_autoptr(GUnixFDList) fd_list = NULL;
int fd;
@@ -322,6 +322,76 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
y0_top, G_DBUS_CALL_FLAGS_NONE,
-1, fd_list, NULL, NULL, NULL);
}
+
+static void dbus_scanout_dmabuf_multi_plane(DBusDisplayListener *ddl,
+ QemuDmaBuf *dmabuf)
+{
+ g_autoptr(GError) err = NULL;
+ g_autoptr(GUnixFDList) fd_list = NULL;
+ int i, fd_index[DMABUF_MAX_PLANES], num_fds;
+ uint32_t width, height, fourcc, num_planes;
+ GVariant *fd, *offset, *stride, *fd_handles[DMABUF_MAX_PLANES];
+ uint64_t modifier;
+ bool y0_top;
+
+ num_planes = qemu_dmabuf_get_num_planes(dmabuf);
+
+ fd_list = g_unix_fd_list_new();
+
+ for (num_fds = 0; num_fds < num_planes; num_fds++) {
+ int plane_fd = qemu_dmabuf_get_fd(dmabuf)[num_fds];
+
+ if (plane_fd < 0)
+ break;
+
+ fd_index[num_fds] = g_unix_fd_list_append(fd_list, plane_fd, &err);
+ if (fd_index[num_fds] < 0) {
+ error_report("Failed to setup dmabuf fdlist: %s", err->message);
+ return;
+ }
+ }
+
+ ddl_discard_display_messages(ddl);
+
+ width = qemu_dmabuf_get_width(dmabuf);
+ height = qemu_dmabuf_get_height(dmabuf);
+ fourcc = qemu_dmabuf_get_fourcc(dmabuf);
+ modifier = qemu_dmabuf_get_modifier(dmabuf);
+ y0_top = qemu_dmabuf_get_y0_top(dmabuf);
+
+ offset = g_variant_new_fixed_array(G_VARIANT_TYPE_UINT32,
+ qemu_dmabuf_get_offset(dmabuf),
+ num_planes, sizeof(uint32_t));
+ stride = g_variant_new_fixed_array(G_VARIANT_TYPE_UINT32,
+ qemu_dmabuf_get_stride(dmabuf),
+ num_planes, sizeof(uint32_t));
+
+ for (i = 0; i < num_fds; i++) {
+ fd_handles[i] = g_variant_new_handle(fd_index[i]);
+ }
+ fd = g_variant_new_array(G_VARIANT_TYPE_HANDLE, fd_handles, num_fds);
+
+ qemu_dbus_display1_listener_unix_multi_plane_call_scanout_dmabuf2(
+ ddl->multi_plane_proxy, fd, width, height, offset, stride, num_planes,
+ fourcc, modifier, y0_top, G_DBUS_CALL_FLAGS_NONE,
+ -1, fd_list, NULL, NULL, NULL);
+}
+
+static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
+ QemuDmaBuf *dmabuf)
+{
+ DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
+
+ if (ddl->multi_plane_proxy) {
+ dbus_scanout_dmabuf_multi_plane(ddl, dmabuf);
+ } else {
+ if (qemu_dmabuf_get_num_planes(dmabuf) > 1) {
+ g_debug("org.qemu.Display1.Listener.ScanoutDMABUF does not support mutli plane");
+ return;
+ }
+ dbus_scanout_dmabuf_single_plane(ddl, dmabuf);
+ }
+}
#endif /* GBM */
#endif /* OPENGL */
@@ -514,10 +584,6 @@ static void dbus_scanout_texture(DisplayChangeListener *dcl,
error_report("%s: failed to export dmabuf for texture", __func__);
return;
}
- if (num_planes > 1) {
- error_report("%s: does not support multi-plane dmabuf", __func__);
- return;
- }
dmabuf = qemu_dmabuf_new(w, h, offset, stride, x, y, backing_width,
backing_height, fourcc, modifier, fd, num_planes,
false, backing_y_0_top);
@@ -886,6 +952,8 @@ dbus_display_listener_dispose(GObject *object)
#ifdef CONFIG_OPENGL
egl_fb_destroy(&ddl->fb);
#endif
+#else /* !WIN32 */
+ g_clear_object(&ddl->multi_plane_proxy);
#endif
G_OBJECT_CLASS(dbus_display_listener_parent_class)->dispose(object);
@@ -1074,6 +1142,26 @@ dbus_display_listener_setup_shared_map(DBusDisplayListener *ddl)
#endif
}
+static void dbus_display_listener_setup_multi_plane(DBusDisplayListener *ddl)
+{
+#ifndef WIN32
+ g_autoptr(GError) err = NULL;
+
+ if (!dbus_display_listener_implements(
+ ddl, "org.qemu.Display1.Listener.Unix.MultiPlane")) {
+ return;
+ }
+ ddl->multi_plane_proxy =
+ qemu_dbus_display1_listener_unix_multi_plane_proxy_new_sync(
+ ddl->conn, G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, NULL,
+ "/org/qemu/Display1/Listener", NULL, &err);
+ if (!ddl->multi_plane_proxy) {
+ g_debug("Failed to setup Unix multi plane proxy: %s", err->message);
+ return;
+ }
+#endif
+}
+
static GDBusMessage *
dbus_filter(GDBusConnection *connection,
GDBusMessage *message,
@@ -1162,6 +1250,7 @@ dbus_display_listener_new(const char *bus_name,
dbus_display_listener_setup_shared_map(ddl);
trace_dbus_can_share_map(ddl->can_share_map);
dbus_display_listener_setup_d3d11(ddl);
+ dbus_display_listener_setup_multi_plane(ddl);
con = qemu_console_lookup_by_index(dbus_display_console_get_index(console));
assert(con);
--
2.43.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-24 8:19 [PATCH 0/6] ui: support multi plane texture yuq825
` (4 preceding siblings ...)
2025-03-24 8:19 ` [PATCH 5/6] ui/dbus: change dbus ScanoutDMABUF interface yuq825
@ 2025-03-24 8:19 ` yuq825
2025-03-24 9:30 ` Marc-André Lureau
5 siblings, 1 reply; 25+ messages in thread
From: yuq825 @ 2025-03-24 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Qiang Yu
From: Qiang Yu <yuq825@gmail.com>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
---
meson.build | 2 +-
ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
2 files changed, 34 insertions(+), 33 deletions(-)
diff --git a/meson.build b/meson.build
index 9d9c11731f..b87704a83b 100644
--- a/meson.build
+++ b/meson.build
@@ -1329,7 +1329,7 @@ if get_option('spice') \
.require(pixman.found(),
error_message: 'cannot enable SPICE if pixman is not available') \
.allowed()
- spice = dependency('spice-server', version: '>=0.14.0',
+ spice = dependency('spice-server', version: '>=0.14.3',
required: get_option('spice'),
method: 'pkg-config')
endif
diff --git a/ui/spice-display.c b/ui/spice-display.c
index b7016ece6a..46b6d5dfc9 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -28,6 +28,8 @@
#include "ui/spice-display.h"
+#include "standard-headers/drm/drm_fourcc.h"
+
bool spice_opengl;
int qemu_spice_rect_is_empty(const QXLRect* r)
@@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
if (ssd->ds) {
uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
+ uint64_t modifier;
surface_gl_create_texture(ssd->gls, ssd->ds);
if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
- (EGLint *)stride, &fourcc, &num_planes, NULL)) {
- surface_gl_destroy_texture(ssd->gls, ssd->ds);
- return;
- }
-
- if (num_planes > 1) {
- fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
+ (EGLint *)stride, &fourcc, &num_planes, &modifier)) {
surface_gl_destroy_texture(ssd->gls, ssd->ds);
return;
}
@@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
fourcc);
/* note: spice server will close the fd */
- spice_qxl_gl_scanout(&ssd->qxl, fd[0],
- surface_width(ssd->ds),
- surface_height(ssd->ds),
- stride[0], fourcc, false);
+ spice_qxl_gl_scanout2(&ssd->qxl, fd,
+ surface_width(ssd->ds),
+ surface_height(ssd->ds),
+ offset, stride, num_planes,
+ fourcc, modifier, false);
ssd->have_surface = true;
ssd->have_scanout = false;
@@ -930,7 +928,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
- spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
+ spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
+ DRM_FORMAT_MOD_INVALID, false);
qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
ssd->have_surface = false;
ssd->have_scanout = false;
@@ -948,22 +947,21 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
int fd[DMABUF_MAX_PLANES], num_planes;
+ uint64_t modifier;
assert(tex_id);
if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
- &num_planes, NULL)) {
+ &num_planes, &modifier)) {
fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
return;
}
- if (num_planes > 1) {
- fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
- return;
- }
+
trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
/* note: spice server will close the fd */
- spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
- stride[0], fourcc, y_0_top);
+ spice_qxl_gl_scanout2(&ssd->qxl, fd, backing_width, backing_height,
+ (uint32_t *)offset, (uint32_t *)stride, num_planes,
+ fourcc, modifier, y_0_top);
qemu_spice_gl_monitor_config(ssd, x, y, w, h);
ssd->have_surface = false;
ssd->have_scanout = true;
@@ -1034,11 +1032,10 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
uint32_t x, uint32_t y, uint32_t w, uint32_t h)
{
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
- EGLint stride = 0, fourcc = 0;
+ EGLint fourcc = 0;
bool render_cursor = false;
bool y_0_top = false; /* FIXME */
uint64_t cookie;
- int fd;
uint32_t width, height, texture;
if (!ssd->have_scanout) {
@@ -1075,6 +1072,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
ssd->blit_fb.height != height) {
int fds[DMABUF_MAX_PLANES], num_planes;
uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
+ uint64_t modifier;
trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
height);
@@ -1083,27 +1081,30 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
width, height);
if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
(EGLint *)offsets, (EGLint *)strides,
- &fourcc, &num_planes, NULL)) {
+ &fourcc, &num_planes, &modifier)) {
fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
return;
}
- if (num_planes > 1) {
- fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
- return;
- }
- spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
- strides[0], fourcc, false);
+
+ spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height, offsets, strides,
+ num_planes, fourcc, modifier, false);
}
} else {
- stride = qemu_dmabuf_get_stride(dmabuf)[0];
+ int fds[DMABUF_MAX_PLANES];
+
fourcc = qemu_dmabuf_get_fourcc(dmabuf);
y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
- qemu_dmabuf_dup_fd(dmabuf, &fd);
+ qemu_dmabuf_dup_fd(dmabuf, fds);
trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
/* note: spice server will close the fd, so hand over a dup */
- spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
- stride, fourcc, y_0_top);
+ spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height,
+ qemu_dmabuf_get_offset(dmabuf),
+ qemu_dmabuf_get_stride(dmabuf),
+ qemu_dmabuf_get_num_planes(dmabuf),
+ fourcc,
+ qemu_dmabuf_get_modifier(dmabuf),
+ y_0_top);
}
qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
ssd->guest_dmabuf_refresh = false;
--
2.43.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-24 8:19 ` [PATCH 6/6] ui/spice: support multi plane dmabuf scanout yuq825
@ 2025-03-24 9:30 ` Marc-André Lureau
2025-03-24 10:00 ` Daniel P. Berrangé
2025-03-24 13:35 ` Qiang Yu
0 siblings, 2 replies; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 9:30 UTC (permalink / raw)
To: yuq825; +Cc: qemu-devel
Hi
On Mon, Mar 24, 2025 at 12:20 PM <yuq825@gmail.com> wrote:
>
> From: Qiang Yu <yuq825@gmail.com>
>
> Signed-off-by: Qiang Yu <yuq825@gmail.com>
> ---
> meson.build | 2 +-
> ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
> 2 files changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 9d9c11731f..b87704a83b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1329,7 +1329,7 @@ if get_option('spice') \
> .require(pixman.found(),
> error_message: 'cannot enable SPICE if pixman is not available') \
> .allowed()
> - spice = dependency('spice-server', version: '>=0.14.0',
> + spice = dependency('spice-server', version: '>=0.14.3',
I am okay with bumping dependency requirements, but the nicer
alternative would be to allow the current version and check the
existence of the new API/function instead.
> required: get_option('spice'),
> method: 'pkg-config')
> endif
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index b7016ece6a..46b6d5dfc9 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -28,6 +28,8 @@
>
> #include "ui/spice-display.h"
>
> +#include "standard-headers/drm/drm_fourcc.h"
> +
> bool spice_opengl;
>
> int qemu_spice_rect_is_empty(const QXLRect* r)
> @@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> if (ssd->ds) {
> uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
> int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
> + uint64_t modifier;
>
> surface_gl_create_texture(ssd->gls, ssd->ds);
> if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
> - (EGLint *)stride, &fourcc, &num_planes, NULL)) {
> - surface_gl_destroy_texture(ssd->gls, ssd->ds);
> - return;
> - }
> -
> - if (num_planes > 1) {
> - fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
> + (EGLint *)stride, &fourcc, &num_planes, &modifier)) {
> surface_gl_destroy_texture(ssd->gls, ssd->ds);
> return;
> }
> @@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> fourcc);
>
> /* note: spice server will close the fd */
> - spice_qxl_gl_scanout(&ssd->qxl, fd[0],
> - surface_width(ssd->ds),
> - surface_height(ssd->ds),
> - stride[0], fourcc, false);
> + spice_qxl_gl_scanout2(&ssd->qxl, fd,
> + surface_width(ssd->ds),
> + surface_height(ssd->ds),
> + offset, stride, num_planes,
> + fourcc, modifier, false);
> ssd->have_surface = true;
> ssd->have_scanout = false;
>
> @@ -930,7 +928,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
>
> trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> - spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> + spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
> + DRM_FORMAT_MOD_INVALID, false);
> qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> ssd->have_surface = false;
> ssd->have_scanout = false;
> @@ -948,22 +947,21 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
> SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
> int fd[DMABUF_MAX_PLANES], num_planes;
> + uint64_t modifier;
>
> assert(tex_id);
> if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
> - &num_planes, NULL)) {
> + &num_planes, &modifier)) {
> fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> return;
> }
> - if (num_planes > 1) {
> - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> - return;
> - }
> +
> trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
>
> /* note: spice server will close the fd */
> - spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
> - stride[0], fourcc, y_0_top);
> + spice_qxl_gl_scanout2(&ssd->qxl, fd, backing_width, backing_height,
> + (uint32_t *)offset, (uint32_t *)stride, num_planes,
> + fourcc, modifier, y_0_top);
> qemu_spice_gl_monitor_config(ssd, x, y, w, h);
> ssd->have_surface = false;
> ssd->have_scanout = true;
> @@ -1034,11 +1032,10 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> {
> SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> - EGLint stride = 0, fourcc = 0;
> + EGLint fourcc = 0;
> bool render_cursor = false;
> bool y_0_top = false; /* FIXME */
> uint64_t cookie;
> - int fd;
> uint32_t width, height, texture;
>
> if (!ssd->have_scanout) {
> @@ -1075,6 +1072,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> ssd->blit_fb.height != height) {
> int fds[DMABUF_MAX_PLANES], num_planes;
> uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
> + uint64_t modifier;
>
> trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
> height);
> @@ -1083,27 +1081,30 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> width, height);
> if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
> (EGLint *)offsets, (EGLint *)strides,
> - &fourcc, &num_planes, NULL)) {
> + &fourcc, &num_planes, &modifier)) {
> fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> return;
> }
> - if (num_planes > 1) {
> - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> - return;
> - }
> - spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
> - strides[0], fourcc, false);
> +
> + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height, offsets, strides,
> + num_planes, fourcc, modifier, false);
> }
> } else {
> - stride = qemu_dmabuf_get_stride(dmabuf)[0];
> + int fds[DMABUF_MAX_PLANES];
> +
> fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> - qemu_dmabuf_dup_fd(dmabuf, &fd);
> + qemu_dmabuf_dup_fd(dmabuf, fds);
>
> trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
> /* note: spice server will close the fd, so hand over a dup */
> - spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
> - stride, fourcc, y_0_top);
> + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height,
> + qemu_dmabuf_get_offset(dmabuf),
> + qemu_dmabuf_get_stride(dmabuf),
> + qemu_dmabuf_get_num_planes(dmabuf),
> + fourcc,
> + qemu_dmabuf_get_modifier(dmabuf),
> + y_0_top);
> }
> qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
> ssd->guest_dmabuf_refresh = false;
> --
> 2.43.0
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-24 9:30 ` Marc-André Lureau
@ 2025-03-24 10:00 ` Daniel P. Berrangé
2025-03-24 13:35 ` Qiang Yu
1 sibling, 0 replies; 25+ messages in thread
From: Daniel P. Berrangé @ 2025-03-24 10:00 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: yuq825, qemu-devel
On Mon, Mar 24, 2025 at 01:30:16PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 24, 2025 at 12:20 PM <yuq825@gmail.com> wrote:
> >
> > From: Qiang Yu <yuq825@gmail.com>
Please provide non-empty commit messages.
> >
> > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > ---
> > meson.build | 2 +-
> > ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
> > 2 files changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 9d9c11731f..b87704a83b 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1329,7 +1329,7 @@ if get_option('spice') \
> > .require(pixman.found(),
> > error_message: 'cannot enable SPICE if pixman is not available') \
> > .allowed()
> > - spice = dependency('spice-server', version: '>=0.14.0',
> > + spice = dependency('spice-server', version: '>=0.14.3',
>
> I am okay with bumping dependency requirements, but the nicer
> alternative would be to allow the current version and check the
> existence of the new API/function instead.
Bumping the min version must be a separate commit that lists
the current versions in our supported build platforms[1], in order
to demonstrate the that the chosen min version doesn't exclude
any supported platform.
See commit 34d55725e664445ccd5621165b1ef805197a530e for how
this should be done.
0.14.3 looks like it would probably be fine as a choice though,
as Ubuntu 20.04 has aged out of our support matrix.
With regards,
Daniel
[1] https://www.qemu.org/docs/master/about/build-platforms.html
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-24 9:30 ` Marc-André Lureau
2025-03-24 10:00 ` Daniel P. Berrangé
@ 2025-03-24 13:35 ` Qiang Yu
2025-03-24 14:02 ` Marc-André Lureau
1 sibling, 1 reply; 25+ messages in thread
From: Qiang Yu @ 2025-03-24 13:35 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
On Mon, Mar 24, 2025 at 5:30 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 12:20 PM <yuq825@gmail.com> wrote:
> >
> > From: Qiang Yu <yuq825@gmail.com>
> >
> > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > ---
> > meson.build | 2 +-
> > ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
> > 2 files changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 9d9c11731f..b87704a83b 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1329,7 +1329,7 @@ if get_option('spice') \
> > .require(pixman.found(),
> > error_message: 'cannot enable SPICE if pixman is not available') \
> > .allowed()
> > - spice = dependency('spice-server', version: '>=0.14.0',
> > + spice = dependency('spice-server', version: '>=0.14.3',
>
> I am okay with bumping dependency requirements, but the nicer
> alternative would be to allow the current version and check the
> existence of the new API/function instead.
>
I'm not familiar with how qemu handle new API dependency, but isn't it much
convenient to just bump lib version instead of a macro all around? And I can't
see similar macro in meson.build
>
> > required: get_option('spice'),
> > method: 'pkg-config')
> > endif
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index b7016ece6a..46b6d5dfc9 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -28,6 +28,8 @@
> >
> > #include "ui/spice-display.h"
> >
> > +#include "standard-headers/drm/drm_fourcc.h"
> > +
> > bool spice_opengl;
> >
> > int qemu_spice_rect_is_empty(const QXLRect* r)
> > @@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > if (ssd->ds) {
> > uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
> > int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
> > + uint64_t modifier;
> >
> > surface_gl_create_texture(ssd->gls, ssd->ds);
> > if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
> > - (EGLint *)stride, &fourcc, &num_planes, NULL)) {
> > - surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > - return;
> > - }
> > -
> > - if (num_planes > 1) {
> > - fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
> > + (EGLint *)stride, &fourcc, &num_planes, &modifier)) {
> > surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > return;
> > }
> > @@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > fourcc);
> >
> > /* note: spice server will close the fd */
> > - spice_qxl_gl_scanout(&ssd->qxl, fd[0],
> > - surface_width(ssd->ds),
> > - surface_height(ssd->ds),
> > - stride[0], fourcc, false);
> > + spice_qxl_gl_scanout2(&ssd->qxl, fd,
> > + surface_width(ssd->ds),
> > + surface_height(ssd->ds),
> > + offset, stride, num_planes,
> > + fourcc, modifier, false);
> > ssd->have_surface = true;
> > ssd->have_scanout = false;
> >
> > @@ -930,7 +928,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> >
> > trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> > - spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> > + spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
> > + DRM_FORMAT_MOD_INVALID, false);
> > qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> > ssd->have_surface = false;
> > ssd->have_scanout = false;
> > @@ -948,22 +947,21 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
> > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
> > int fd[DMABUF_MAX_PLANES], num_planes;
> > + uint64_t modifier;
> >
> > assert(tex_id);
> > if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
> > - &num_planes, NULL)) {
> > + &num_planes, &modifier)) {
> > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > return;
> > }
> > - if (num_planes > 1) {
> > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > - return;
> > - }
> > +
> > trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
> >
> > /* note: spice server will close the fd */
> > - spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
> > - stride[0], fourcc, y_0_top);
> > + spice_qxl_gl_scanout2(&ssd->qxl, fd, backing_width, backing_height,
> > + (uint32_t *)offset, (uint32_t *)stride, num_planes,
> > + fourcc, modifier, y_0_top);
> > qemu_spice_gl_monitor_config(ssd, x, y, w, h);
> > ssd->have_surface = false;
> > ssd->have_scanout = true;
> > @@ -1034,11 +1032,10 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> > {
> > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > - EGLint stride = 0, fourcc = 0;
> > + EGLint fourcc = 0;
> > bool render_cursor = false;
> > bool y_0_top = false; /* FIXME */
> > uint64_t cookie;
> > - int fd;
> > uint32_t width, height, texture;
> >
> > if (!ssd->have_scanout) {
> > @@ -1075,6 +1072,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > ssd->blit_fb.height != height) {
> > int fds[DMABUF_MAX_PLANES], num_planes;
> > uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
> > + uint64_t modifier;
> >
> > trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
> > height);
> > @@ -1083,27 +1081,30 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > width, height);
> > if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
> > (EGLint *)offsets, (EGLint *)strides,
> > - &fourcc, &num_planes, NULL)) {
> > + &fourcc, &num_planes, &modifier)) {
> > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > return;
> > }
> > - if (num_planes > 1) {
> > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > - return;
> > - }
> > - spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
> > - strides[0], fourcc, false);
> > +
> > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height, offsets, strides,
> > + num_planes, fourcc, modifier, false);
> > }
> > } else {
> > - stride = qemu_dmabuf_get_stride(dmabuf)[0];
> > + int fds[DMABUF_MAX_PLANES];
> > +
> > fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> > y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> > - qemu_dmabuf_dup_fd(dmabuf, &fd);
> > + qemu_dmabuf_dup_fd(dmabuf, fds);
> >
> > trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
> > /* note: spice server will close the fd, so hand over a dup */
> > - spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
> > - stride, fourcc, y_0_top);
> > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height,
> > + qemu_dmabuf_get_offset(dmabuf),
> > + qemu_dmabuf_get_stride(dmabuf),
> > + qemu_dmabuf_get_num_planes(dmabuf),
> > + fourcc,
> > + qemu_dmabuf_get_modifier(dmabuf),
> > + y_0_top);
> > }
> > qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
> > ssd->guest_dmabuf_refresh = false;
> > --
> > 2.43.0
> >
> >
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-24 13:35 ` Qiang Yu
@ 2025-03-24 14:02 ` Marc-André Lureau
2025-03-26 1:45 ` Qiang Yu
0 siblings, 1 reply; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-24 14:02 UTC (permalink / raw)
To: Qiang Yu; +Cc: qemu-devel
On Mon, Mar 24, 2025 at 5:35 PM Qiang Yu <yuq825@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 5:30 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Mon, Mar 24, 2025 at 12:20 PM <yuq825@gmail.com> wrote:
> > >
> > > From: Qiang Yu <yuq825@gmail.com>
> > >
> > > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > > ---
> > > meson.build | 2 +-
> > > ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
> > > 2 files changed, 34 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/meson.build b/meson.build
> > > index 9d9c11731f..b87704a83b 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1329,7 +1329,7 @@ if get_option('spice') \
> > > .require(pixman.found(),
> > > error_message: 'cannot enable SPICE if pixman is not available') \
> > > .allowed()
> > > - spice = dependency('spice-server', version: '>=0.14.0',
> > > + spice = dependency('spice-server', version: '>=0.14.3',
> >
> > I am okay with bumping dependency requirements, but the nicer
> > alternative would be to allow the current version and check the
> > existence of the new API/function instead.
> >
> I'm not familiar with how qemu handle new API dependency, but isn't it much
> convenient to just bump lib version instead of a macro all around? And I can't
> see similar macro in meson.build
Yes it is generally simpler to bump requirements, but as Daniel
hinted, we have policies about supporting older environments.
For your series, I think we could simply have:
if spice.found()
config_host_data.set('HAVE_SPICE_QXL_GL_SCANOUT2',
cc.has_function('spice_qxl_gl_scanout2', dependencies: spice))
endif
>
> >
> > > required: get_option('spice'),
> > > method: 'pkg-config')
> > > endif
> > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > index b7016ece6a..46b6d5dfc9 100644
> > > --- a/ui/spice-display.c
> > > +++ b/ui/spice-display.c
> > > @@ -28,6 +28,8 @@
> > >
> > > #include "ui/spice-display.h"
> > >
> > > +#include "standard-headers/drm/drm_fourcc.h"
> > > +
> > > bool spice_opengl;
> > >
> > > int qemu_spice_rect_is_empty(const QXLRect* r)
> > > @@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > > if (ssd->ds) {
> > > uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
> > > int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
> > > + uint64_t modifier;
> > >
> > > surface_gl_create_texture(ssd->gls, ssd->ds);
> > > if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
> > > - (EGLint *)stride, &fourcc, &num_planes, NULL)) {
> > > - surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > - return;
> > > - }
> > > -
> > > - if (num_planes > 1) {
> > > - fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
> > > + (EGLint *)stride, &fourcc, &num_planes, &modifier)) {
> > > surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > return;
> > > }
> > > @@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > > fourcc);
> > >
> > > /* note: spice server will close the fd */
> > > - spice_qxl_gl_scanout(&ssd->qxl, fd[0],
> > > - surface_width(ssd->ds),
> > > - surface_height(ssd->ds),
> > > - stride[0], fourcc, false);
> > > + spice_qxl_gl_scanout2(&ssd->qxl, fd,
> > > + surface_width(ssd->ds),
> > > + surface_height(ssd->ds),
> > > + offset, stride, num_planes,
> > > + fourcc, modifier, false);
> > > ssd->have_surface = true;
> > > ssd->have_scanout = false;
> > >
> > > @@ -930,7 +928,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > >
> > > trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> > > - spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> > > + spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
> > > + DRM_FORMAT_MOD_INVALID, false);
> > > qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> > > ssd->have_surface = false;
> > > ssd->have_scanout = false;
> > > @@ -948,22 +947,21 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
> > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
> > > int fd[DMABUF_MAX_PLANES], num_planes;
> > > + uint64_t modifier;
> > >
> > > assert(tex_id);
> > > if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
> > > - &num_planes, NULL)) {
> > > + &num_planes, &modifier)) {
> > > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > > return;
> > > }
> > > - if (num_planes > 1) {
> > > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > > - return;
> > > - }
> > > +
> > > trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
> > >
> > > /* note: spice server will close the fd */
> > > - spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
> > > - stride[0], fourcc, y_0_top);
> > > + spice_qxl_gl_scanout2(&ssd->qxl, fd, backing_width, backing_height,
> > > + (uint32_t *)offset, (uint32_t *)stride, num_planes,
> > > + fourcc, modifier, y_0_top);
> > > qemu_spice_gl_monitor_config(ssd, x, y, w, h);
> > > ssd->have_surface = false;
> > > ssd->have_scanout = true;
> > > @@ -1034,11 +1032,10 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> > > {
> > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > - EGLint stride = 0, fourcc = 0;
> > > + EGLint fourcc = 0;
> > > bool render_cursor = false;
> > > bool y_0_top = false; /* FIXME */
> > > uint64_t cookie;
> > > - int fd;
> > > uint32_t width, height, texture;
> > >
> > > if (!ssd->have_scanout) {
> > > @@ -1075,6 +1072,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > ssd->blit_fb.height != height) {
> > > int fds[DMABUF_MAX_PLANES], num_planes;
> > > uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
> > > + uint64_t modifier;
> > >
> > > trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
> > > height);
> > > @@ -1083,27 +1081,30 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > width, height);
> > > if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
> > > (EGLint *)offsets, (EGLint *)strides,
> > > - &fourcc, &num_planes, NULL)) {
> > > + &fourcc, &num_planes, &modifier)) {
> > > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > > return;
> > > }
> > > - if (num_planes > 1) {
> > > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > > - return;
> > > - }
> > > - spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
> > > - strides[0], fourcc, false);
> > > +
> > > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height, offsets, strides,
> > > + num_planes, fourcc, modifier, false);
> > > }
> > > } else {
> > > - stride = qemu_dmabuf_get_stride(dmabuf)[0];
> > > + int fds[DMABUF_MAX_PLANES];
> > > +
> > > fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> > > y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> > > - qemu_dmabuf_dup_fd(dmabuf, &fd);
> > > + qemu_dmabuf_dup_fd(dmabuf, fds);
> > >
> > > trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
> > > /* note: spice server will close the fd, so hand over a dup */
> > > - spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
> > > - stride, fourcc, y_0_top);
> > > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height,
> > > + qemu_dmabuf_get_offset(dmabuf),
> > > + qemu_dmabuf_get_stride(dmabuf),
> > > + qemu_dmabuf_get_num_planes(dmabuf),
> > > + fourcc,
> > > + qemu_dmabuf_get_modifier(dmabuf),
> > > + y_0_top);
> > > }
> > > qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
> > > ssd->guest_dmabuf_refresh = false;
> > > --
> > > 2.43.0
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-24 14:02 ` Marc-André Lureau
@ 2025-03-26 1:45 ` Qiang Yu
2025-03-26 6:38 ` Marc-André Lureau
0 siblings, 1 reply; 25+ messages in thread
From: Qiang Yu @ 2025-03-26 1:45 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
On Mon, Mar 24, 2025 at 10:02 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 5:35 PM Qiang Yu <yuq825@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 5:30 PM Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 12:20 PM <yuq825@gmail.com> wrote:
> > > >
> > > > From: Qiang Yu <yuq825@gmail.com>
> > > >
> > > > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > > > ---
> > > > meson.build | 2 +-
> > > > ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
> > > > 2 files changed, 34 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/meson.build b/meson.build
> > > > index 9d9c11731f..b87704a83b 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -1329,7 +1329,7 @@ if get_option('spice') \
> > > > .require(pixman.found(),
> > > > error_message: 'cannot enable SPICE if pixman is not available') \
> > > > .allowed()
> > > > - spice = dependency('spice-server', version: '>=0.14.0',
> > > > + spice = dependency('spice-server', version: '>=0.14.3',
> > >
> > > I am okay with bumping dependency requirements, but the nicer
> > > alternative would be to allow the current version and check the
> > > existence of the new API/function instead.
> > >
> > I'm not familiar with how qemu handle new API dependency, but isn't it much
> > convenient to just bump lib version instead of a macro all around? And I can't
> > see similar macro in meson.build
>
> Yes it is generally simpler to bump requirements, but as Daniel
> hinted, we have policies about supporting older environments.
>
> For your series, I think we could simply have:
> if spice.found()
> config_host_data.set('HAVE_SPICE_QXL_GL_SCANOUT2',
> cc.has_function('spice_qxl_gl_scanout2', dependencies: spice))
> endif
>
This serial is more like a bug fix instead of feature adding. Because without
new spice-server, qemu spice gl scanout support is completely broken when
run on multi plane GPU driver. If not bumping spice version, user may still
suffer this problem when using new qemu and old spice. Is this OK?
>
> >
> > >
> > > > required: get_option('spice'),
> > > > method: 'pkg-config')
> > > > endif
> > > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > > index b7016ece6a..46b6d5dfc9 100644
> > > > --- a/ui/spice-display.c
> > > > +++ b/ui/spice-display.c
> > > > @@ -28,6 +28,8 @@
> > > >
> > > > #include "ui/spice-display.h"
> > > >
> > > > +#include "standard-headers/drm/drm_fourcc.h"
> > > > +
> > > > bool spice_opengl;
> > > >
> > > > int qemu_spice_rect_is_empty(const QXLRect* r)
> > > > @@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > > > if (ssd->ds) {
> > > > uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
> > > > int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
> > > > + uint64_t modifier;
> > > >
> > > > surface_gl_create_texture(ssd->gls, ssd->ds);
> > > > if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
> > > > - (EGLint *)stride, &fourcc, &num_planes, NULL)) {
> > > > - surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > > - return;
> > > > - }
> > > > -
> > > > - if (num_planes > 1) {
> > > > - fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
> > > > + (EGLint *)stride, &fourcc, &num_planes, &modifier)) {
> > > > surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > > return;
> > > > }
> > > > @@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > > > fourcc);
> > > >
> > > > /* note: spice server will close the fd */
> > > > - spice_qxl_gl_scanout(&ssd->qxl, fd[0],
> > > > - surface_width(ssd->ds),
> > > > - surface_height(ssd->ds),
> > > > - stride[0], fourcc, false);
> > > > + spice_qxl_gl_scanout2(&ssd->qxl, fd,
> > > > + surface_width(ssd->ds),
> > > > + surface_height(ssd->ds),
> > > > + offset, stride, num_planes,
> > > > + fourcc, modifier, false);
> > > > ssd->have_surface = true;
> > > > ssd->have_scanout = false;
> > > >
> > > > @@ -930,7 +928,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> > > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > >
> > > > trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> > > > - spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> > > > + spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
> > > > + DRM_FORMAT_MOD_INVALID, false);
> > > > qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> > > > ssd->have_surface = false;
> > > > ssd->have_scanout = false;
> > > > @@ -948,22 +947,21 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
> > > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > > EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
> > > > int fd[DMABUF_MAX_PLANES], num_planes;
> > > > + uint64_t modifier;
> > > >
> > > > assert(tex_id);
> > > > if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
> > > > - &num_planes, NULL)) {
> > > > + &num_planes, &modifier)) {
> > > > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > > > return;
> > > > }
> > > > - if (num_planes > 1) {
> > > > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > > > - return;
> > > > - }
> > > > +
> > > > trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
> > > >
> > > > /* note: spice server will close the fd */
> > > > - spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
> > > > - stride[0], fourcc, y_0_top);
> > > > + spice_qxl_gl_scanout2(&ssd->qxl, fd, backing_width, backing_height,
> > > > + (uint32_t *)offset, (uint32_t *)stride, num_planes,
> > > > + fourcc, modifier, y_0_top);
> > > > qemu_spice_gl_monitor_config(ssd, x, y, w, h);
> > > > ssd->have_surface = false;
> > > > ssd->have_scanout = true;
> > > > @@ -1034,11 +1032,10 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> > > > {
> > > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > > - EGLint stride = 0, fourcc = 0;
> > > > + EGLint fourcc = 0;
> > > > bool render_cursor = false;
> > > > bool y_0_top = false; /* FIXME */
> > > > uint64_t cookie;
> > > > - int fd;
> > > > uint32_t width, height, texture;
> > > >
> > > > if (!ssd->have_scanout) {
> > > > @@ -1075,6 +1072,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > ssd->blit_fb.height != height) {
> > > > int fds[DMABUF_MAX_PLANES], num_planes;
> > > > uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
> > > > + uint64_t modifier;
> > > >
> > > > trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
> > > > height);
> > > > @@ -1083,27 +1081,30 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > width, height);
> > > > if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
> > > > (EGLint *)offsets, (EGLint *)strides,
> > > > - &fourcc, &num_planes, NULL)) {
> > > > + &fourcc, &num_planes, &modifier)) {
> > > > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > > > return;
> > > > }
> > > > - if (num_planes > 1) {
> > > > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > > > - return;
> > > > - }
> > > > - spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
> > > > - strides[0], fourcc, false);
> > > > +
> > > > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height, offsets, strides,
> > > > + num_planes, fourcc, modifier, false);
> > > > }
> > > > } else {
> > > > - stride = qemu_dmabuf_get_stride(dmabuf)[0];
> > > > + int fds[DMABUF_MAX_PLANES];
> > > > +
> > > > fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> > > > y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> > > > - qemu_dmabuf_dup_fd(dmabuf, &fd);
> > > > + qemu_dmabuf_dup_fd(dmabuf, fds);
> > > >
> > > > trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
> > > > /* note: spice server will close the fd, so hand over a dup */
> > > > - spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
> > > > - stride, fourcc, y_0_top);
> > > > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height,
> > > > + qemu_dmabuf_get_offset(dmabuf),
> > > > + qemu_dmabuf_get_stride(dmabuf),
> > > > + qemu_dmabuf_get_num_planes(dmabuf),
> > > > + fourcc,
> > > > + qemu_dmabuf_get_modifier(dmabuf),
> > > > + y_0_top);
> > > > }
> > > > qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
> > > > ssd->guest_dmabuf_refresh = false;
> > > > --
> > > > 2.43.0
> > > >
> > > >
> > >
> > >
> > > --
> > > Marc-André Lureau
>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
2025-03-26 1:45 ` Qiang Yu
@ 2025-03-26 6:38 ` Marc-André Lureau
0 siblings, 0 replies; 25+ messages in thread
From: Marc-André Lureau @ 2025-03-26 6:38 UTC (permalink / raw)
To: Qiang Yu; +Cc: qemu-devel
Hi
On Wed, Mar 26, 2025 at 5:46 AM Qiang Yu <yuq825@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 10:02 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 5:35 PM Qiang Yu <yuq825@gmail.com> wrote:
> > >
> > > On Mon, Mar 24, 2025 at 5:30 PM Marc-André Lureau
> > > <marcandre.lureau@gmail.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Mon, Mar 24, 2025 at 12:20 PM <yuq825@gmail.com> wrote:
> > > > >
> > > > > From: Qiang Yu <yuq825@gmail.com>
> > > > >
> > > > > Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > > > > ---
> > > > > meson.build | 2 +-
> > > > > ui/spice-display.c | 65 +++++++++++++++++++++++-----------------------
> > > > > 2 files changed, 34 insertions(+), 33 deletions(-)
> > > > >
> > > > > diff --git a/meson.build b/meson.build
> > > > > index 9d9c11731f..b87704a83b 100644
> > > > > --- a/meson.build
> > > > > +++ b/meson.build
> > > > > @@ -1329,7 +1329,7 @@ if get_option('spice') \
> > > > > .require(pixman.found(),
> > > > > error_message: 'cannot enable SPICE if pixman is not available') \
> > > > > .allowed()
> > > > > - spice = dependency('spice-server', version: '>=0.14.0',
> > > > > + spice = dependency('spice-server', version: '>=0.14.3',
> > > >
> > > > I am okay with bumping dependency requirements, but the nicer
> > > > alternative would be to allow the current version and check the
> > > > existence of the new API/function instead.
> > > >
> > > I'm not familiar with how qemu handle new API dependency, but isn't it much
> > > convenient to just bump lib version instead of a macro all around? And I can't
> > > see similar macro in meson.build
> >
> > Yes it is generally simpler to bump requirements, but as Daniel
> > hinted, we have policies about supporting older environments.
> >
> > For your series, I think we could simply have:
> > if spice.found()
> > config_host_data.set('HAVE_SPICE_QXL_GL_SCANOUT2',
> > cc.has_function('spice_qxl_gl_scanout2', dependencies: spice))
> > endif
> >
> This serial is more like a bug fix instead of feature adding. Because without
> new spice-server, qemu spice gl scanout support is completely broken when
> run on multi plane GPU driver. If not bumping spice version, user may still
> suffer this problem when using new qemu and old spice. Is this OK?
Yes, the new QEMU should print a warning or error out on startup in
such situation.
> >
> > >
> > > >
> > > > > required: get_option('spice'),
> > > > > method: 'pkg-config')
> > > > > endif
> > > > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > > > index b7016ece6a..46b6d5dfc9 100644
> > > > > --- a/ui/spice-display.c
> > > > > +++ b/ui/spice-display.c
> > > > > @@ -28,6 +28,8 @@
> > > > >
> > > > > #include "ui/spice-display.h"
> > > > >
> > > > > +#include "standard-headers/drm/drm_fourcc.h"
> > > > > +
> > > > > bool spice_opengl;
> > > > >
> > > > > int qemu_spice_rect_is_empty(const QXLRect* r)
> > > > > @@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > > > > if (ssd->ds) {
> > > > > uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
> > > > > int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
> > > > > + uint64_t modifier;
> > > > >
> > > > > surface_gl_create_texture(ssd->gls, ssd->ds);
> > > > > if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint *)offset,
> > > > > - (EGLint *)stride, &fourcc, &num_planes, NULL)) {
> > > > > - surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > > > - return;
> > > > > - }
> > > > > -
> > > > > - if (num_planes > 1) {
> > > > > - fprintf(stderr, "%s: does not support multi-plane texture\n", __func__);
> > > > > + (EGLint *)stride, &fourcc, &num_planes, &modifier)) {
> > > > > surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > > > return;
> > > > > }
> > > > > @@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
> > > > > fourcc);
> > > > >
> > > > > /* note: spice server will close the fd */
> > > > > - spice_qxl_gl_scanout(&ssd->qxl, fd[0],
> > > > > - surface_width(ssd->ds),
> > > > > - surface_height(ssd->ds),
> > > > > - stride[0], fourcc, false);
> > > > > + spice_qxl_gl_scanout2(&ssd->qxl, fd,
> > > > > + surface_width(ssd->ds),
> > > > > + surface_height(ssd->ds),
> > > > > + offset, stride, num_planes,
> > > > > + fourcc, modifier, false);
> > > > > ssd->have_surface = true;
> > > > > ssd->have_scanout = false;
> > > > >
> > > > > @@ -930,7 +928,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> > > > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > > >
> > > > > trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> > > > > - spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> > > > > + spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
> > > > > + DRM_FORMAT_MOD_INVALID, false);
> > > > > qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> > > > > ssd->have_surface = false;
> > > > > ssd->have_scanout = false;
> > > > > @@ -948,22 +947,21 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
> > > > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > > > EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
> > > > > int fd[DMABUF_MAX_PLANES], num_planes;
> > > > > + uint64_t modifier;
> > > > >
> > > > > assert(tex_id);
> > > > > if (!egl_dmabuf_export_texture(tex_id, fd, offset, stride, &fourcc,
> > > > > - &num_planes, NULL)) {
> > > > > + &num_planes, &modifier)) {
> > > > > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > > > > return;
> > > > > }
> > > > > - if (num_planes > 1) {
> > > > > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > > > > - return;
> > > > > - }
> > > > > +
> > > > > trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
> > > > >
> > > > > /* note: spice server will close the fd */
> > > > > - spice_qxl_gl_scanout(&ssd->qxl, fd[0], backing_width, backing_height,
> > > > > - stride[0], fourcc, y_0_top);
> > > > > + spice_qxl_gl_scanout2(&ssd->qxl, fd, backing_width, backing_height,
> > > > > + (uint32_t *)offset, (uint32_t *)stride, num_planes,
> > > > > + fourcc, modifier, y_0_top);
> > > > > qemu_spice_gl_monitor_config(ssd, x, y, w, h);
> > > > > ssd->have_surface = false;
> > > > > ssd->have_scanout = true;
> > > > > @@ -1034,11 +1032,10 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > > uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> > > > > {
> > > > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > > > - EGLint stride = 0, fourcc = 0;
> > > > > + EGLint fourcc = 0;
> > > > > bool render_cursor = false;
> > > > > bool y_0_top = false; /* FIXME */
> > > > > uint64_t cookie;
> > > > > - int fd;
> > > > > uint32_t width, height, texture;
> > > > >
> > > > > if (!ssd->have_scanout) {
> > > > > @@ -1075,6 +1072,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > > ssd->blit_fb.height != height) {
> > > > > int fds[DMABUF_MAX_PLANES], num_planes;
> > > > > uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
> > > > > + uint64_t modifier;
> > > > >
> > > > > trace_qemu_spice_gl_render_dmabuf(ssd->qxl.id, width,
> > > > > height);
> > > > > @@ -1083,27 +1081,30 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
> > > > > width, height);
> > > > > if (!egl_dmabuf_export_texture(ssd->blit_fb.texture, fds,
> > > > > (EGLint *)offsets, (EGLint *)strides,
> > > > > - &fourcc, &num_planes, NULL)) {
> > > > > + &fourcc, &num_planes, &modifier)) {
> > > > > fprintf(stderr, "%s: failed to export dmabuf for texture\n", __func__);
> > > > > return;
> > > > > }
> > > > > - if (num_planes > 1) {
> > > > > - fprintf(stderr, "%s: does not support multi-plane dmabuf\n", __func__);
> > > > > - return;
> > > > > - }
> > > > > - spice_qxl_gl_scanout(&ssd->qxl, fds[0], width, height,
> > > > > - strides[0], fourcc, false);
> > > > > +
> > > > > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height, offsets, strides,
> > > > > + num_planes, fourcc, modifier, false);
> > > > > }
> > > > > } else {
> > > > > - stride = qemu_dmabuf_get_stride(dmabuf)[0];
> > > > > + int fds[DMABUF_MAX_PLANES];
> > > > > +
> > > > > fourcc = qemu_dmabuf_get_fourcc(dmabuf);
> > > > > y_0_top = qemu_dmabuf_get_y0_top(dmabuf);
> > > > > - qemu_dmabuf_dup_fd(dmabuf, &fd);
> > > > > + qemu_dmabuf_dup_fd(dmabuf, fds);
> > > > >
> > > > > trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id, width, height);
> > > > > /* note: spice server will close the fd, so hand over a dup */
> > > > > - spice_qxl_gl_scanout(&ssd->qxl, fd, width, height,
> > > > > - stride, fourcc, y_0_top);
> > > > > + spice_qxl_gl_scanout2(&ssd->qxl, fds, width, height,
> > > > > + qemu_dmabuf_get_offset(dmabuf),
> > > > > + qemu_dmabuf_get_stride(dmabuf),
> > > > > + qemu_dmabuf_get_num_planes(dmabuf),
> > > > > + fourcc,
> > > > > + qemu_dmabuf_get_modifier(dmabuf),
> > > > > + y_0_top);
> > > > > }
> > > > > qemu_spice_gl_monitor_config(ssd, 0, 0, width, height);
> > > > > ssd->guest_dmabuf_refresh = false;
> > > > > --
> > > > > 2.43.0
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Marc-André Lureau
> >
> >
> >
> > --
> > Marc-André Lureau
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 25+ messages in thread