Hi
On Wed, Mar 26, 2025 at 5:46 AM Qiang Yu <yuq...@gmail.com> wrote:
>
> 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?
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
