Re: [Qemu-devel] [PATCH 5/8] spice: add opengl/virgl/dmabuf support

[Marc-André Lureau]

Hi

On Thu, Feb 18, 2016 at 10:26 AM, Gerd Hoffmann <address@hidden> wrote:
> This adds support for dma-buf passing to spice.  This makes virtio-gpu
> with 3d acceleration work with spice.
>
> Workflow:
>  * virglrenderer renders the guest command stream into a texture.
>  * qemu exports the texture as dma-buf and passes on that dma-buf
>    to spice-server.
>  * spice-server passes the dma-buf to spice-client, using unix
>    socket file descriptor passing.
>  * spice-client asks the window systems composer to render the
>    dma-buf to the screen.
>
> Requires cutting edge spice (server) and spice-gtk (client) builds,
> from git master branch.
>
> Also requires libvirt managing your qemu instance, and using
> "virt-viewer --attach $guest".  libvirt will connect spice-server and
> spice-client using unix sockets instead of tcp sockets then, which
> is required for file descriptor passing.
>
> Works for the local case (spice server and client on the same machine)
> only.  Supporting remote too is planned (by feeding the dma-bufs into
> gpu-assisted video encoder), but not there yet.
>
> gl mode is turned off by default, use "-spice gl=on,$otherargs" to
> enable it.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>

I did the initial implementation of this patch, and I spent
considerable time testing and fixing yours. Even if you don't always
take my patch or fixes, I think it'd be fair adding me as Signed-off.

> ---
>  include/ui/spice-display.h |  18 +++++++
>  qemu-options.hx            |   4 ++
>  ui/spice-core.c            |  18 ++++++-
>  ui/spice-display.c         | 129 
> +++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 165 insertions(+), 4 deletions(-)
>
> diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> index b25328a..5c5726a 100644
> --- a/include/ui/spice-display.h
> +++ b/include/ui/spice-display.h
> @@ -23,6 +23,16 @@
>  #include "ui/qemu-pixman.h"
>  #include "ui/console.h"
>  #include "sysemu/sysemu.h"
> +#ifdef CONFIG_OPENGL_DMABUF
> +#include "ui/egl-helpers.h"
> +#include "ui/egl-context.h"
> +#endif
> +
> +#if defined(CONFIG_OPENGL_DMABUF)
> +# if SPICE_SERVER_VERSION >= 0x000d00 /* release 0.13.0 */

It will be most likely 0.13.1, at least not 0.13.0.

> +#  define HAVE_SPICE_GL 1
> +# endif
> +#endif
>
>  #define NUM_MEMSLOTS 8
>  #define MEMSLOT_GENERATION_BITS 8
> @@ -50,6 +60,7 @@ enum {
>      QXL_COOKIE_TYPE_IO,
>      QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
>      QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
> +    QXL_COOKIE_TYPE_GL_DRAW_DONE,
>  };
>
>  typedef struct QXLCookie {
> @@ -104,6 +115,13 @@ struct SimpleSpiceDisplay {
>      QEMUCursor *cursor;
>      int mouse_x, mouse_y;
>      QEMUBH *cursor_bh;
> +
> +#ifdef HAVE_SPICE_GL
> +    /* opengl rendering */
> +    QEMUBH *gl_unblock_bh;
> +    QEMUTimer *gl_unblock_timer;

I think it would make sense if this helper timer would be a seperate
patch (we may want to revert or change it). It would make the patch
easier to read.

> +    int dmabuf_fd;
> +#endif
>  };
>
>  struct SimpleSpiceUpdate {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 2f0465e..8cc367e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1051,6 +1051,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
>      "       [,streaming-video=[off|all|filter]][,disable-copy-paste]\n"
>      "       [,disable-agent-file-xfer][,agent-mouse=[on|off]]\n"
>      "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
> +    "       [,gl=[on|off]\n"

missing ]

>      "   enable spice\n"
>      "   at least one of {port, tls-port} is mandatory\n",
>      QEMU_ARCH_ALL)
> @@ -1142,6 +1143,9 @@ Enable/disable audio stream compression (using celt 
> 0.5.1).  Default is on.
>  @item seamless-migration=[on|off]
>  Enable/disable spice seamless migration. Default is off.
>
> address@hidden gl=[on|off]
> +Enable/disable OpenGL context. Default is off.
> +
>  @end table
>  ETEXI
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 4dbd99a..2c9aba5 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -39,6 +39,9 @@
>  #include "hw/hw.h"
>  #include "ui/spice-display.h"
>  #include "qapi-event.h"
> +#ifdef CONFIG_OPENGL_DMABUF

we may want to use HAVE_SPICE_GL to simplify things (what can we
expect with dmabuf && !spice-gl here?)

> +#include "ui/egl-helpers.h"
> +#endif
>
>  /* core bits */
>
> @@ -494,9 +497,14 @@ static QemuOptsList qemu_spice_opts = {
>          },{
>              .name = "playback-compression",
>              .type = QEMU_OPT_BOOL,
> -        }, {
> +        },{
>              .name = "seamless-migration",
>              .type = QEMU_OPT_BOOL,
> +#ifdef CONFIG_OPENGL_DMABUF
> +        },{
> +            .name = "gl",
> +            .type = QEMU_OPT_BOOL,
> +#endif
>          },
>          { /* end of list */ }
>      },
> @@ -819,6 +827,14 @@ void qemu_spice_init(void)
>  #if SPICE_SERVER_VERSION >= 0x000c02
>      qemu_spice_register_ports();
>  #endif
> +
> +#ifdef CONFIG_OPENGL_DMABUF
> +    if (qemu_opt_get_bool(opts, "gl", 0)) {
> +        if (egl_rendernode_init() == 0) {
> +            display_opengl = 1;
> +        }
> +    }
> +#endif
>  }
>
>  int qemu_spice_add_interface(SpiceBaseInstance *sin)
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 2a2a7c1..17a71ce 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -650,9 +650,23 @@ static void interface_update_area_complete(QXLInstance 
> *sin,
>  /* called from spice server thread context only */
>  static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
>  {
> -    /* should never be called, used in qxl native mode only */
> -    fprintf(stderr, "%s: abort()\n", __func__);
> -    abort();
> +    QXLCookie *cookie = (QXLCookie *)(uintptr_t)cookie_token;
> +
> +    switch (cookie->type) {
> +#ifdef HAVE_SPICE_GL
> +    case QXL_COOKIE_TYPE_GL_DRAW_DONE:
> +    {
> +        SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> +        qemu_bh_schedule(ssd->gl_unblock_bh);
> +        break;
> +    }
> +#endif
> +    default:
> +        /* should never be called, used in qxl native mode only */
> +        fprintf(stderr, "%s: abort()\n", __func__);
> +        abort();
> +    }
> +    g_free(cookie);
>  }
>
>  static void interface_set_client_capabilities(QXLInstance *sin,
> @@ -779,6 +793,106 @@ static const DisplayChangeListenerOps 
> display_listener_ops = {
>      .dpy_cursor_define    = display_mouse_define,
>  };
>
> +#ifdef HAVE_SPICE_GL
> +
> +static void qemu_spice_gl_block(SimpleSpiceDisplay *ssd, bool block)
> +{
> +    uint64_t timeout;
> +
> +    if (block) {
> +        timeout = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +        timeout += 1000; /* one sec */
> +        timer_mod(ssd->gl_unblock_timer, timeout);
> +    } else {
> +        timer_del(ssd->gl_unblock_timer);
> +    }
> +    graphic_hw_gl_block(ssd->dcl.con, block);
> +}
> +
> +static void qemu_spice_gl_unblock_bh(void *opaque)
> +{
> +    SimpleSpiceDisplay *ssd = opaque;
> +
> +    qemu_spice_gl_block(ssd, false);
> +}
> +
> +static void qemu_spice_gl_block_timer(void *opaque)
> +{
> +    fprintf(stderr, "WARNING: spice: no gl-draw-done within one second\n");
> +}
> +
> +static QEMUGLContext qemu_spice_gl_create_context(DisplayChangeListener *dcl,
> +                                                  QEMUGLParams *params)
> +{
> +    eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
> +                   qemu_egl_rn_ctx);
> +    return qemu_egl_create_context(dcl, params);
> +}
> +
> +static void qemu_spice_gl_scanout(DisplayChangeListener *dcl,
> +                                  uint32_t tex_id,
> +                                  bool y_0_top,
> +                                  uint32_t x, uint32_t y,
> +                                  uint32_t w, uint32_t h)
> +{
> +    SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> +    EGLint stride = 0, fourcc = 0;
> +    int fd = -1;
> +
> +    if (tex_id) {
> +        fd = egl_get_fd_for_texture(tex_id, &stride, &fourcc);
> +        if (fd < 0) {
> +            fprintf(stderr, "%s: failed to get fd for texture\n", __func__);
> +            return;
> +        }
> +    }
> +    dprint(0, "%s: %dx%d (stride %d, fourcc 0x%x)\n", __func__,
> +           w, h, stride, fourcc);
> +
> +    assert(!tex_id || fd >= 0);
> +    spice_qxl_gl_scanout(&ssd->qxl, fd,
> +                         surface_width(ssd->ds),
> +                         surface_height(ssd->ds),
> +                         stride, fourcc, y_0_top);
> +

The server and client need to know where is the monitor within the
scanout. I am surprised you deliberately dropped the monitor config
from previous patches. It seems it's not necessary to get things
working today, but I am afraid this is not what virgl expects and it
will likely break.

> +    if (ssd->dmabuf_fd != -1) {
> +        close(ssd->dmabuf_fd);
> +    }

This is incompatible with Spice server API, it is responsible for
closing it when no longer needed or outdated (otherwise finding out
whether the fd is valid in spice server will be broken)

> +    ssd->dmabuf_fd = fd;
> +}
> +
> +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);
> +    uint64_t cookie;
> +
> +    dprint(1, "%s\n", __func__);
> +    qemu_spice_gl_block(ssd, true);
> +    cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> +    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> +}
> +
> +static const DisplayChangeListenerOps display_listener_gl_ops = {
> +    .dpy_name             = "spice-egl",
> +    .dpy_gfx_update       = display_update,
> +    .dpy_gfx_switch       = display_switch,
> +    .dpy_gfx_check_format = qemu_pixman_check_format,
> +    .dpy_refresh          = display_refresh,
> +    .dpy_mouse_set        = display_mouse_set,
> +    .dpy_cursor_define    = display_mouse_define,
> +
> +    .dpy_gl_ctx_create       = qemu_spice_gl_create_context,
> +    .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
> +    .dpy_gl_ctx_make_current = qemu_egl_make_context_current,
> +    .dpy_gl_ctx_get_current  = qemu_egl_get_current_context,
> +
> +    .dpy_gl_scanout          = qemu_spice_gl_scanout,
> +    .dpy_gl_update           = qemu_spice_gl_update,
> +};
> +
> +#endif /* HAVE_SPICE_GL */
> +
>  static void qemu_spice_display_init_one(QemuConsole *con)
>  {
>      SimpleSpiceDisplay *ssd = g_new0(SimpleSpiceDisplay, 1);
> @@ -792,6 +906,15 @@ static void qemu_spice_display_init_one(QemuConsole *con)
>      qemu_spice_create_host_memslot(ssd);
>
>      ssd->dcl.ops = &display_listener_ops;
> +#ifdef HAVE_SPICE_GL
> +    if (display_opengl) {
> +        ssd->dcl.ops = &display_listener_gl_ops;
> +        ssd->dmabuf_fd = -1;
> +        ssd->gl_unblock_bh = qemu_bh_new(qemu_spice_gl_unblock_bh, ssd);
> +        ssd->gl_unblock_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> +                                             qemu_spice_gl_block_timer, ssd);
> +    }
> +#endif
>      ssd->dcl.con = con;
>      register_displaychangelistener(&ssd->dcl);
>  }
> --
> 1.8.3.1
>

After this patch, spice+virgl works :)


-- 
Marc-André Lureau