Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd

[Alex Bennée <alex.bennee@xxxxxxxxxx>]

Alex Bennée <alex.bennee@xxxxxxxxxx> writes:

> Huang Rui <ray.huang@xxxxxxx> writes:
>
>> From: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
>>
>> This relies on a virglrenderer change to include the dmabuf fd when
>> returning resource info.
>>
> <snip>
>> +static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>> +                                       struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virgl_gpu_resource *vres;
>> +    struct virtio_gpu_framebuffer fb = { 0 };
>> +    struct virtio_gpu_set_scanout_blob ss;
>> +    struct virgl_renderer_resource_info info;
>> +    uint64_t fbend;
>> +
>> +    VIRTIO_GPU_FILL_CMD(ss);
>> +    virtio_gpu_scanout_blob_bswap(&ss);
>> +    trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
>> +                                          ss.r.width, ss.r.height, ss.r.x,
>> +                                          ss.r.y);
>> +
>> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified 
>> %d",
>> +                      __func__, ss.scanout_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
>> +        return;
>> +    }
>> +
>> +    if (ss.resource_id == 0) {
>> +        virtio_gpu_disable_scanout(g, ss.scanout_id);
>> +        return;
>> +    }
>> +
>> +    if (ss.width < 16 ||
>> +        ss.height < 16 ||
>> +        ss.r.x + ss.r.width > ss.width ||
>> +        ss.r.y + ss.r.height > ss.height) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
>> +                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
>> +                      __func__, ss.scanout_id, ss.resource_id,
>> +                      ss.r.x, ss.r.y, ss.r.width, ss.r.height,
>> +                      ss.width, ss.height);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without 
>> GL!\n", __func__);
>> +        return;
>> +    }
>> +
>> +    vres = virgl_gpu_find_resource(g, ss.resource_id);
>> +    if (!vres) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: illegal resource specified %d\n",
>> +                      __func__, ss.resource_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>> +    if (virgl_renderer_resource_get_info(ss.resource_id, &info)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: illegal virgl resource specified %d\n",
>> +                      __func__, ss.resource_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>
> Minor nit, the format of the following needs to include braces.
>
>> +    if (!vres->res.dmabuf_fd && info.fd)
>> +        vres->res.dmabuf_fd = info.fd;
>
> However I'm seeing:
>
>   cc -m64 -mcx16 -Ilibcommon.fa.p -I../../common-user/host/x86_64 
> -I../../linux-user/include/host/x86_64 -I../../linux-user/include -Iui 
> -I../../ui -I/usr/include/capstone -I/usr/include/p11-kit-1 
> -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server 
> -I/usr/include/spice-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 
> -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include 
> -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 
> -I/usr/include/slirp -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 
> -I/usr/include/harfbuzz -I/usr/include/freetype2 -I/usr/include/fribidi 
> -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 
> -I/usr/include/x86_64-linux-gnu -I/usr/include/atk-1.0 
> -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 
> -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include 
> -I/usr/include/vte-2.91 -I/usr/include/virgl 
> -I/home/alex/lsrc/qemu.git/builds/extra.libs/install/include 
> -I/usr/include/cacard -I/usr/include/nss -I/usr/include/nspr 
> -I/usr/include/PCSC -I/usr/include/pipewire-0.3 -I/usr/include/spa-0.2 
> -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g 
> -fstack-protector-strong -Wundef -Wwrite-strings -Wmissing-prototypes 
> -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k 
> -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels 
> -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute 
> -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -Wshadow=local 
> -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers 
> -iquote . -iquote /home/alex/lsrc/qemu.git -iquote 
> /home/alex/lsrc/qemu.git/include -iquote 
> /home/alex/lsrc/qemu.git/host/include/x86_64 -iquote 
> /home/alex/lsrc/qemu.git/host/include/generic -iquote 
> /home/alex/lsrc/qemu.git/tcg/i386 -pthread -D_GNU_SOURCE 
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common 
> -fwrapv -fPIE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -DNCURSES_WIDECHAR=1 
> -D_REENTRANT -DSTRUCT_IOVEC_DEFINED -MD -MQ 
> libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o -MF 
> libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o.d -o 
> libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o -c 
> ../../hw/display/virtio-gpu-virgl.c
>   ../../hw/display/virtio-gpu-virgl.c: In function 
> ‘virgl_cmd_set_scanout_blob’:
>   ../../hw/display/virtio-gpu-virgl.c:790:37: error: ‘struct 
> virgl_renderer_resource_info’ has no member named ‘fd’
>     790 |     if (!vres->res.dmabuf_fd && info.fd)
>         |                                     ^
>   ../../hw/display/virtio-gpu-virgl.c:791:35: error: ‘struct 
> virgl_renderer_resource_info’ has no member named ‘fd’
>     791 |         vres->res.dmabuf_fd = info.fd;
>         |                                   ^
>
> But searching my extra libs (for aemu/gfstream/rutabaga_ffi) I can see
> the bindings.rs but nothing generated a header:
>
>   $ ag -r "virgl_renderer_resource_info" 
>   crosvm.git/rutabaga_gfx/src/generated/virgl_renderer_bindings.rs
>   33:pub const VIRGL_RENDERER_RESOURCE_INFO_EXT_VERSION: u32 = 0;
>   337:pub struct virgl_renderer_resource_info {
>   351:pub struct virgl_renderer_resource_info_ext {
>   353:    pub base: virgl_renderer_resource_info,
>   359:impl Default for virgl_renderer_resource_info_ext {
>   373:        info: *mut virgl_renderer_resource_info,
>   379:        info: *mut virgl_renderer_resource_info_ext,
>
> Which makes me think a) its picked up the older virgl headers and b) the
> crosvm/rutabaf_gfx install needs a fix.

Actually it was libvirglrenderer was too old (I got it the wrong way
round, the rust bindings come from libvirglrenderer). As we want to
build with older libvirglrenderers on older systems I think this needs a
tweak to meson.build, maybe something like:

    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
                         cc.has_function('virgl_renderer_resource_create_blob',
                                          prefix: '#include <virglrenderer.h>',
                                          dependencies: virgl)
                         and
                         cc.has_member('struct virgl_renderer_resource_info', 
'fd',
                                       prefix: '#include <virglrenderer.h>',
                                       dependencies: virgl))

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro