| Message ID | 20231219075320.165227-12-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
On 2023/12/19 16:53, Huang Rui wrote: > From: Robert Beckett <bob.beckett@collabora.com> > > This relies on a virglrenderer change to include the dmabuf fd when > returning resource info. > > Signed-off-by: Robert Beckett <bob.beckett@collabora.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > Changes in v6: > - Add scanout_blob function for virtio-gpu-virgl. > - Update for new virgl_gpu_resource. > > hw/display/virtio-gpu-virgl.c | 104 +++++++++++++++++++++++++++++++++ > hw/display/virtio-gpu.c | 4 +- > include/hw/virtio/virtio-gpu.h | 6 ++ > 3 files changed, 112 insertions(+), 2 deletions(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index c523a6717a..c384225a98 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -18,6 +18,7 @@ > #include "hw/virtio/virtio.h" > #include "hw/virtio/virtio-gpu.h" > #include "hw/virtio/virtio-gpu-bswap.h" > +#include "hw/virtio/virtio-gpu-pixman.h" > > #include "ui/egl-helpers.h" > > @@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > object_unparent(OBJECT(mr)); > } > > +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)) { Shouldn't OpenGL always be available for virgl? > + 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; > + } > + if (!vres->res.dmabuf_fd && info.fd) > + vres->res.dmabuf_fd = info.fd; > + > + fb.format = virtio_gpu_get_pixman_format(ss.format); > + if (!fb.format) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: host couldn't handle guest format %d\n", > + __func__, ss.format); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; > + return; > + } > + > + fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8); > + fb.width = ss.width; > + fb.height = ss.height; > + fb.stride = ss.strides[0]; > + fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride; > + > + fbend = fb.offset; > + fbend += fb.stride * (ss.r.height - 1); > + fbend += fb.bytes_pp * ss.r.width; > + if (fbend > vres->res.blob_size) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: fb end out of range\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; > + return; > + } > + > + g->parent_obj.enable = 1; > + if (virtio_gpu_update_dmabuf(g, ss.scanout_id, &vres->res, > + &fb, &ss.r)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: failed to update dmabuf\n", __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; > + return; > + } > + virtio_gpu_update_scanout(g, ss.scanout_id, &vres->res, &ss.r); > +} > + > #endif /* HAVE_VIRGL_RESOURCE_BLOB */ > > void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > @@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > virgl_cmd_resource_unmap_blob(g, cmd); > break; > + case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB: VIRTIO_GPU_CMD_SET_SCANOUT_BLOB support should be added before allowing the user to enable the resource blob support. You should also check if virtio_vdev_has_feature(VIRTIO_DEVICE(g), VIRTIO_GPU_F_RESOURCE_BLOB). It also applies to other resource blob commands though I failed to note that for the earlier patch.
Le 21/12/2023 à 07:25, Akihiko Odaki a écrit : > On 2023/12/19 16:53, Huang Rui wrote: >> From: Robert Beckett <bob.beckett@collabora.com> >> >> This relies on a virglrenderer change to include the dmabuf fd when >> returning resource info. >> >> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> --- >> >> Changes in v6: >> - Add scanout_blob function for virtio-gpu-virgl. >> - Update for new virgl_gpu_resource. >> >> hw/display/virtio-gpu-virgl.c | 104 +++++++++++++++++++++++++++++++++ >> hw/display/virtio-gpu.c | 4 +- >> include/hw/virtio/virtio-gpu.h | 6 ++ >> 3 files changed, 112 insertions(+), 2 deletions(-) >> >> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >> index c523a6717a..c384225a98 100644 >> --- a/hw/display/virtio-gpu-virgl.c >> +++ b/hw/display/virtio-gpu-virgl.c >> @@ -18,6 +18,7 @@ >> #include "hw/virtio/virtio.h" >> #include "hw/virtio/virtio-gpu.h" >> #include "hw/virtio/virtio-gpu-bswap.h" >> +#include "hw/virtio/virtio-gpu-pixman.h" >> #include "ui/egl-helpers.h" >> @@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, >> object_unparent(OBJECT(mr)); >> } >> +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)) { > > Shouldn't OpenGL always be available for virgl? > >> + 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; >> + } >> + if (!vres->res.dmabuf_fd && info.fd) >> + vres->res.dmabuf_fd = info.fd; >> + >> + fb.format = virtio_gpu_get_pixman_format(ss.format); >> + if (!fb.format) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: host couldn't handle guest format %d\n", >> + __func__, ss.format); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; >> + return; >> + } >> + >> + fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8); >> + fb.width = ss.width; >> + fb.height = ss.height; >> + fb.stride = ss.strides[0]; >> + fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride; >> + >> + fbend = fb.offset; >> + fbend += fb.stride * (ss.r.height - 1); >> + fbend += fb.bytes_pp * ss.r.width; >> + if (fbend > vres->res.blob_size) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: fb end out of range\n", >> + __func__); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; >> + return; >> + } >> + >> + g->parent_obj.enable = 1; >> + if (virtio_gpu_update_dmabuf(g, ss.scanout_id, &vres->res, >> + &fb, &ss.r)) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: failed to update dmabuf\n", __func__); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; >> + return; >> + } >> + virtio_gpu_update_scanout(g, ss.scanout_id, &vres->res, &ss.r); >> +} >> + >> #endif /* HAVE_VIRGL_RESOURCE_BLOB */ >> void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, >> @@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, >> case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: >> virgl_cmd_resource_unmap_blob(g, cmd); >> break; >> + case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB: > > VIRTIO_GPU_CMD_SET_SCANOUT_BLOB support should be added before allowing the user to enable the resource blob support. It seems that two patches were squashed together: - "virtio-gpu: Handle set scanout blob command" by Antonio Caggiano - "virtio-gpu: Handle set scanout blob command" by Bob Beckett Restoring the original patches with the appropriate S-o-b tags seems the right thing to do. Thanks, Pierre-Eric > > You should also check if virtio_vdev_has_feature(VIRTIO_DEVICE(g), VIRTIO_GPU_F_RESOURCE_BLOB). It also applies to other resource blob commands though I failed to note that for the earlier patch. >
Huang Rui <ray.huang@amd.com> writes: > From: Robert Beckett <bob.beckett@collabora.com> > > 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.
Alex Bennée <alex.bennee@linaro.org> writes: > Huang Rui <ray.huang@amd.com> writes: > >> From: Robert Beckett <bob.beckett@collabora.com> >> >> 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))
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index c523a6717a..c384225a98 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -18,6 +18,7 @@ #include "hw/virtio/virtio.h" #include "hw/virtio/virtio-gpu.h" #include "hw/virtio/virtio-gpu-bswap.h" +#include "hw/virtio/virtio-gpu-pixman.h" #include "ui/egl-helpers.h" @@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, object_unparent(OBJECT(mr)); } +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; + } + if (!vres->res.dmabuf_fd && info.fd) + vres->res.dmabuf_fd = info.fd; + + fb.format = virtio_gpu_get_pixman_format(ss.format); + if (!fb.format) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: host couldn't handle guest format %d\n", + __func__, ss.format); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; + return; + } + + fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8); + fb.width = ss.width; + fb.height = ss.height; + fb.stride = ss.strides[0]; + fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride; + + fbend = fb.offset; + fbend += fb.stride * (ss.r.height - 1); + fbend += fb.bytes_pp * ss.r.width; + if (fbend > vres->res.blob_size) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: fb end out of range\n", + __func__); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; + return; + } + + g->parent_obj.enable = 1; + if (virtio_gpu_update_dmabuf(g, ss.scanout_id, &vres->res, + &fb, &ss.r)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: failed to update dmabuf\n", __func__); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; + return; + } + virtio_gpu_update_scanout(g, ss.scanout_id, &vres->res, &ss.r); +} + #endif /* HAVE_VIRGL_RESOURCE_BLOB */ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, @@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: virgl_cmd_resource_unmap_blob(g, cmd); break; + case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB: + virgl_cmd_set_scanout_blob(g, cmd); + break; #endif /* HAVE_VIRGL_RESOURCE_BLOB */ default: cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 466debb256..492f578b4b 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -380,7 +380,7 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g, QTAILQ_INSERT_HEAD(&g->reslist, res, next); } -static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id) +void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id) { struct virtio_gpu_scanout *scanout = &g->parent_obj.scanout[scanout_id]; struct virtio_gpu_simple_resource *res; @@ -592,7 +592,7 @@ static void virtio_unref_resource(pixman_image_t *image, void *data) pixman_image_unref(data); } -static void virtio_gpu_update_scanout(VirtIOGPU *g, +void virtio_gpu_update_scanout(VirtIOGPU *g, uint32_t scanout_id, struct virtio_gpu_simple_resource *res, struct virtio_gpu_rect *r) diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h index 76b410fe91..ac2adfb607 100644 --- a/include/hw/virtio/virtio-gpu.h +++ b/include/hw/virtio/virtio-gpu.h @@ -332,6 +332,12 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g, struct virtio_gpu_framebuffer *fb, struct virtio_gpu_rect *r); +void virtio_gpu_update_scanout(VirtIOGPU *g, + uint32_t scanout_id, + struct virtio_gpu_simple_resource *res, + struct virtio_gpu_rect *r); +void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id); + /* virtio-gpu-3d.c */ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd);