On Mon, Mar 24, 2025 at 10:02 PM Marc-André Lureau
<marcandre.lur...@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 5:35 PM Qiang Yu <yuq...@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 5:30 PM Marc-André Lureau
> > <marcandre.lur...@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 12:20 PM <yuq...@gmail.com> wrote:
> > > >
> > > > From: Qiang Yu <yuq...@gmail.com>
> > > >
> > > > Signed-off-by: Qiang Yu <yuq...@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
