[PATCH 0/3] virtio-gpu: Enable virglrenderer backend for rutabaga

[PATCH 0/3] virtio-gpu: Enable virglrenderer backend for rutabaga

[Weifeng Liu]

Greetings,

I'd like to introduce you my attempt to enable virglrenderer backend for
rutabaga empowered virtio-gpu device.  I am aware that there have been
effort in supporting venus in virtio-gpu-virgl.c [1], but there is no
reason to prevent us from leveraging the virglrenderer component in
rutabaga_gfx, especially it being not very hard to add this
functionality.

Generally, the gap is the polling capability, i.e., virglrenderer
requires the main thread (namely the GPU command handling thread) to
poll virglrenderer at proper moments, which is not yet supported in
virtio-gpu-rutabaga device. This patch set try to add this so that
virglrenderer backend (including virgl and venus) can work as expected.

Slight change to rutabaga_gfx_ffi is also a requirement, which is
included in [2].

Further effort is required to tune the performance, since copying is
present before the rendered images get displayed. But I still think this
patch set could be a good starting point for the pending work.

For those interested in setting up environment and playing around with
this patch set, here is guideline in brief:

1. Clone the master/main branch of virglrenderer, compile and install it.

	git clone https://gitlab.freedesktop.org/virgl/virglrenderer
	cd virglrenderer
	meson setup builddir \
	  --prefix=$INSTALL_DIR/virglrenderer \
	  -Dvenus=true
	ninja -C builddir install

2. Clone the patched CrosVM, build and install rutabaga_gfx_ffi.

	git clone -b rutabaga_ffi_virgl https://github.com/phreer/crosvm.git
	cd crosvm/rutabaga_gfx/ffi
	export PKG_CONFIG_PATH=$INSTALL_DIR/virglrenderer/lib64/pkgconfig/
	meson setup builddir/ \
	  --prefix $HOME/install/rutabaga_gfx/rutabaga_gfx_ffi/ \
	  -Dvirglrenderer=true
	ninja -C builddir install

3. Applied this patch set to QEMU, build and install it:

	cd qemu	
	# Apply this patch set atop main branch ...
	mkdir builddir; cd builddir
	../configure --prefix=$INSTALL_DIR/qemu \
	  --target-list=x86_64-softmmu \
	  --disable-virglrenderer \
	  --enable-rutabaga_gfx
	ninja -C builddir install

4. If you are lucky and everything goes fine, you are prepared to launch
   VM with virglrenderer backed virtio-gpu-rutabaga device:

	export LD_LIBRARY_PATH=$INSTALL_DIR/virglrenderer/lib64/:$LD_LIBRARY_PATH
	export LD_LIBRARY_PATH=$INSTALL_DIR/rutabaga_gfx_ffi/lib64/:$LD_LIBRARY_PATH
	$INSTALL_DIR/qemu/bin/qemu-system-x86_64
	$QEMU -d guest_errors -enable-kvm -M q35 -smp 4 -m $MEM \
	  -object memory-backend-memfd,id=mem1,size=$MEM \
	  -machine memory-backend=mem1 \
	  -device virtio-vga-rutabaga,venus=on,virgl2=on,wsi=surfaceless,hostmem=$MEM \

Note:

- You might need this patch set [3] to avoid KVM bad address error when
  you are running on a GPU using TTM for memory management.

[1] https://lore.kernel.org/all/dba6eb97-e1d1-4694-bfb6-e72db95715dd@daynix.com/T/
[2] https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5599645/1
[3] https://lore.kernel.org/kvm/20240229025759.1187910-1-stevensd@google.com/

Weifeng Liu (3):
  virtio-gpu: rutabaga: Properly set stride when copying resources
  virtio-gpu: rutabaga: Poll rutabaga upon events
  virtio-gpu: rutabaga: Add options to enable virgl and venus contexts

 hw/display/virtio-gpu-rutabaga.c | 104 ++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-gpu.h   |   1 +
 2 files changed, 104 insertions(+), 1 deletion(-)

-- 
2.45.0

[PATCH 1/3] virtio-gpu: rutabaga: Properly set stride when copying resources

[Weifeng Liu]

The stride is not correctly assigned when copying pixel data, causing
images being displayed incomplete when using 2d component of rutabaga.

Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
---
 hw/display/virtio-gpu-rutabaga.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
index 17bf701a21..2ba6869606 100644
--- a/hw/display/virtio-gpu-rutabaga.c
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -53,6 +53,7 @@ virtio_gpu_rutabaga_update_cursor(VirtIOGPU *g, struct virtio_gpu_scanout *s,
     transfer.z = 0;
     transfer.w = res->width;
     transfer.h = res->height;
+    transfer.stride = res->width * 4;
     transfer.d = 1;
 
     transfer_iovec.iov_base = s->current_cursor->data;
@@ -273,6 +274,7 @@ rutabaga_cmd_resource_flush(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd)
     transfer.z = 0;
     transfer.w = res->width;
     transfer.h = res->height;
+    transfer.stride = pixman_image_get_stride(res->image);
     transfer.d = 1;
 
     transfer_iovec.iov_base = pixman_image_get_data(res->image);
@@ -382,6 +384,7 @@ rutabaga_cmd_transfer_to_host_2d(VirtIOGPU *g,
     transfer.z = 0;
     transfer.w = t2d.r.width;
     transfer.h = t2d.r.height;
+    transfer.stride = t2d.r.width * 4;
     transfer.d = 1;
 
     result = rutabaga_resource_transfer_write(vr->rutabaga, 0, t2d.resource_id,
-- 
2.45.0

[PATCH 2/3] virtio-gpu: rutabaga: Poll rutabaga upon events

[Weifeng Liu]

To make virglrenderer work properly, we must poll it periodically or
when the event fd is readable, but this functionality is absent now.
This change registers the event fd as a gsource to the main loop and
attaches polling function as callback to the gsource.

Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
---
 hw/display/virtio-gpu-rutabaga.c | 97 +++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-gpu.h   |  1 +
 2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
index 2ba6869606..ecb146315a 100644
--- a/hw/display/virtio-gpu-rutabaga.c
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -899,6 +899,93 @@ virtio_gpu_rutabaga_aio_cb(void *opaque)
     g_free(data);
 }
 
+
+static void
+virtio_gpu_fence_poll(void *opaque)
+{
+    VirtIOGPU *g = opaque;
+    VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
+
+    rutabaga_poll(vr->rutabaga);
+}
+
+typedef void (*vu_watch_cb) (VirtIOGPURutabaga *dev, int condition, void *data);
+
+static void
+event_poll_cb(VirtIOGPURutabaga *dev, int condition, void *data)
+{
+    virtio_gpu_fence_poll(data);
+}
+
+typedef struct VrSrc {
+    GSource parent;
+    VirtIOGPURutabaga *dev;
+    GPollFD gfd;
+} VrSrc;
+
+static gboolean
+vr_src_prepare(GSource *gsrc, gint *timeout)
+{
+    g_assert(timeout);
+
+    *timeout = -1;
+    return FALSE;
+}
+
+static gboolean
+vr_src_check(GSource *gsrc)
+{
+    VrSrc *src = (VrSrc *)gsrc;
+
+    g_assert(src);
+
+    return src->gfd.revents & src->gfd.events;
+}
+
+static gboolean
+vr_src_dispatch(GSource *gsrc, GSourceFunc cb, gpointer data)
+{
+    VrSrc *src = (VrSrc *)gsrc;
+
+    g_assert(src);
+
+    ((vu_watch_cb)cb)(src->dev, src->gfd.revents, data);
+
+    return G_SOURCE_CONTINUE;
+}
+
+static GSourceFuncs vug_src_funcs = {
+    vr_src_prepare,
+    vr_src_check,
+    vr_src_dispatch,
+    NULL
+};
+
+static GSource *
+vr_source_new(VirtIOGPURutabaga *dev, int fd, GIOCondition cond,
+              vu_watch_cb vu_cb, gpointer data)
+{
+    GSource *gsrc;
+    VrSrc *src;
+    guint id;
+
+    g_assert(fd >= 0);
+    g_assert(vu_cb);
+
+    gsrc = g_source_new(&vug_src_funcs, sizeof(VrSrc));
+    g_source_set_callback(gsrc, (GSourceFunc)vu_cb, data, NULL);
+    src = (VrSrc *)gsrc;
+    src->dev = dev;
+    src->gfd.fd = fd;
+    src->gfd.events = cond;
+
+    g_source_add_poll(gsrc, &src->gfd);
+    id = g_source_attach(gsrc, g_main_context_get_thread_default());
+    g_assert(id);
+
+    return gsrc;
+}
+
 static void
 virtio_gpu_rutabaga_fence_cb(uint64_t user_data,
                              const struct rutabaga_fence *fence)
@@ -954,6 +1041,7 @@ virtio_gpu_rutabaga_debug_cb(uint64_t user_data,
 static bool virtio_gpu_rutabaga_init(VirtIOGPU *g, Error **errp)
 {
     int result;
+    int poll_descriptor;
     struct rutabaga_builder builder = { 0 };
     struct rutabaga_channel channel = { 0 };
     struct rutabaga_channels channels = { 0 };
@@ -1031,7 +1119,14 @@ static bool virtio_gpu_rutabaga_init(VirtIOGPU *g, Error **errp)
         error_setg_errno(errp, -result, "Failed to init rutabaga");
         return false;
     }
-
+    result = rutabaga_poll_descriptor(vr->rutabaga, &poll_descriptor);
+    if (result) {
+        error_setg_errno(errp, -result, "Failed to get rutabaga poll descriptor");
+        return false;
+    }
+    if (poll_descriptor >= 0)
+        vr->poll_event_source = vr_source_new(vr, poll_descriptor, G_IO_IN,
+                                              event_poll_cb, g);
     return true;
 }
 
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 7a59379f5a..47a4347a9e 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -261,6 +261,7 @@ struct VirtIOGPURutabaga {
     char *wsi;
     bool headless;
     uint32_t num_capsets;
+    GSource *poll_event_source;
     struct rutabaga *rutabaga;
 };
 
-- 
2.45.0

[PATCH 3/3] virtio-gpu: rutabaga: Add options to enable virgl and venus contexts

[Weifeng Liu]

With this change, people will be able to use parameter like the one
below to add start virglrenderer backed virtio-gpu-rutabaga device:

    -device virtio-vga-rutabaga,venus=on,virgl2=on,wsi=surfaceless

Performance being suboptimal though, this would be the first step.

Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
---
 hw/display/virtio-gpu-rutabaga.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
index ecb146315a..85bc33af8c 100644
--- a/hw/display/virtio-gpu-rutabaga.c
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -1203,6 +1203,10 @@ static Property virtio_gpu_rutabaga_properties[] = {
                       RUTABAGA_CAPSET_GFXSTREAM_GLES, false),
     DEFINE_PROP_BIT64("x-gfxstream-composer", VirtIOGPURutabaga, capset_mask,
                       RUTABAGA_CAPSET_GFXSTREAM_COMPOSER, false),
+    DEFINE_PROP_BIT64("venus", VirtIOGPURutabaga, capset_mask,
+                      RUTABAGA_CAPSET_VENUS, false),
+    DEFINE_PROP_BIT64("virgl2", VirtIOGPURutabaga, capset_mask,
+                      RUTABAGA_CAPSET_VIRGL2, false),
     DEFINE_PROP_STRING("wayland-socket-path", VirtIOGPURutabaga,
                        wayland_socket_path),
     DEFINE_PROP_STRING("wsi", VirtIOGPURutabaga, wsi),
-- 
2.45.0

Re: [PATCH 0/3] virtio-gpu: Enable virglrenderer backend for rutabaga

[Alex Bennée]

Weifeng Liu <weifeng.liu.z@gmail.com> writes:

> Greetings,
>
> I'd like to introduce you my attempt to enable virglrenderer backend for
> rutabaga empowered virtio-gpu device.  I am aware that there have been
> effort in supporting venus in virtio-gpu-virgl.c [1], but there is no
> reason to prevent us from leveraging the virglrenderer component in
> rutabaga_gfx, especially it being not very hard to add this
> functionality.
>
> Generally, the gap is the polling capability, i.e., virglrenderer
> requires the main thread (namely the GPU command handling thread) to
> poll virglrenderer at proper moments, which is not yet supported in
> virtio-gpu-rutabaga device. This patch set try to add this so that
> virglrenderer backend (including virgl and venus) can work as expected.
>
> Slight change to rutabaga_gfx_ffi is also a requirement, which is
> included in [2].
>
> Further effort is required to tune the performance, since copying is
> present before the rendered images get displayed. But I still think this
> patch set could be a good starting point for the pending work.
>
> For those interested in setting up environment and playing around with
> this patch set, here is guideline in brief:
>
> 1. Clone the master/main branch of virglrenderer, compile and install it.
>
> 	git clone https://gitlab.freedesktop.org/virgl/virglrenderer
> 	cd virglrenderer
> 	meson setup builddir \
> 	  --prefix=$INSTALL_DIR/virglrenderer \
> 	  -Dvenus=true
> 	ninja -C builddir install
>
> 2. Clone the patched CrosVM, build and install rutabaga_gfx_ffi.
>
> 	git clone -b rutabaga_ffi_virgl https://github.com/phreer/crosvm.git
> 	cd crosvm/rutabaga_gfx/ffi
> 	export PKG_CONFIG_PATH=$INSTALL_DIR/virglrenderer/lib64/pkgconfig/
> 	meson setup builddir/ \
> 	  --prefix $HOME/install/rutabaga_gfx/rutabaga_gfx_ffi/ \
> 	  -Dvirglrenderer=true
> 	ninja -C builddir install

Is there a PR going in for this? The moving parts for rutabaga are
complex enough I think we need support upstream before merging this.

Is this branch where I should be getting the poll helpers from?

  cc -m64 @qemu-system-arm.rsp
  /usr/bin/ld: libcommon.fa.p/hw_display_virtio-gpu-rutabaga.c.o: in function `virtio_gpu_fence_poll':
  /home/alex/lsrc/qemu.git/builds/vulkan/../../hw/display/virtio-gpu-rutabaga.c:909: undefined reference to `rutabaga_poll'
  /usr/bin/ld: libcommon.fa.p/hw_display_virtio-gpu-rutabaga.c.o: in function `virtio_gpu_rutabaga_init':
  /home/alex/lsrc/qemu.git/builds/vulkan/../../hw/display/virtio-gpu-rutabaga.c:1122: undefined reference to `rutabaga_poll_descriptor'
  collect2: error: ld returned 1 exit status
  ninja: build stopped: subcommand failed.


> 3. Applied this patch set to QEMU, build and install it:
>
> 	cd qemu	
> 	# Apply this patch set atop main branch ...
> 	mkdir builddir; cd builddir
> 	../configure --prefix=$INSTALL_DIR/qemu \
> 	  --target-list=x86_64-softmmu \
> 	  --disable-virglrenderer \
> 	  --enable-rutabaga_gfx
> 	ninja -C builddir install
>
> 4. If you are lucky and everything goes fine, you are prepared to launch
>    VM with virglrenderer backed virtio-gpu-rutabaga device:
>
> 	export LD_LIBRARY_PATH=$INSTALL_DIR/virglrenderer/lib64/:$LD_LIBRARY_PATH
> 	export LD_LIBRARY_PATH=$INSTALL_DIR/rutabaga_gfx_ffi/lib64/:$LD_LIBRARY_PATH
> 	$INSTALL_DIR/qemu/bin/qemu-system-x86_64
> 	$QEMU -d guest_errors -enable-kvm -M q35 -smp 4 -m $MEM \
> 	  -object memory-backend-memfd,id=mem1,size=$MEM \
> 	  -machine memory-backend=mem1 \
> 	  -device virtio-vga-rutabaga,venus=on,virgl2=on,wsi=surfaceless,hostmem=$MEM \
>

This should go into docs/system/devices/virtio-gpu.rst with some
explanation. Is there anything we need on the guest side or does this
skip the encapsulating requirements of wayland?

> Note:
>
> - You might need this patch set [3] to avoid KVM bad address error when
>   you are running on a GPU using TTM for memory management.
>
> [1] https://lore.kernel.org/all/dba6eb97-e1d1-4694-bfb6-e72db95715dd@daynix.com/T/
> [2] https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5599645/1
> [3] https://lore.kernel.org/kvm/20240229025759.1187910-1-stevensd@google.com/
>
> Weifeng Liu (3):
>   virtio-gpu: rutabaga: Properly set stride when copying resources
>   virtio-gpu: rutabaga: Poll rutabaga upon events
>   virtio-gpu: rutabaga: Add options to enable virgl and venus contexts
>
>  hw/display/virtio-gpu-rutabaga.c | 104 ++++++++++++++++++++++++++++++-
>  include/hw/virtio/virtio-gpu.h   |   1 +
>  2 files changed, 104 insertions(+), 1 deletion(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/3] virtio-gpu: Enable virglrenderer backend for rutabaga

[Weifeng Liu]

Hi Alex,

On Thu, 2024-06-06 at 11:43 +0100, Alex Bennée wrote:
> Weifeng Liu <weifeng.liu.z@gmail.com> writes:
> 
> > Greetings,
> > 
> > I'd like to introduce you my attempt to enable virglrenderer backend for
> > rutabaga empowered virtio-gpu device.  I am aware that there have been
> > effort in supporting venus in virtio-gpu-virgl.c [1], but there is no
> > reason to prevent us from leveraging the virglrenderer component in
> > rutabaga_gfx, especially it being not very hard to add this
> > functionality.
> > 
> > Generally, the gap is the polling capability, i.e., virglrenderer
> > requires the main thread (namely the GPU command handling thread) to
> > poll virglrenderer at proper moments, which is not yet supported in
> > virtio-gpu-rutabaga device. This patch set try to add this so that
> > virglrenderer backend (including virgl and venus) can work as expected.
> > 
> > Slight change to rutabaga_gfx_ffi is also a requirement, which is
> > included in [2].
> > 
> > Further effort is required to tune the performance, since copying is
> > present before the rendered images get displayed. But I still think this
> > patch set could be a good starting point for the pending work.
> > 
> > For those interested in setting up environment and playing around with
> > this patch set, here is guideline in brief:
> > 
> > 1. Clone the master/main branch of virglrenderer, compile and install it.
> > 
> > 	git clone https://gitlab.freedesktop.org/virgl/virglrenderer
> > 	cd virglrenderer
> > 	meson setup builddir \
> > 	  --prefix=$INSTALL_DIR/virglrenderer \
> > 	  -Dvenus=true
> > 	ninja -C builddir install
> > 
> > 2. Clone the patched CrosVM, build and install rutabaga_gfx_ffi.
> > 
> > 	git clone -b rutabaga_ffi_virgl https://github.com/phreer/crosvm.git
> > 	cd crosvm/rutabaga_gfx/ffi
> > 	export PKG_CONFIG_PATH=$INSTALL_DIR/virglrenderer/lib64/pkgconfig/
> > 	meson setup builddir/ \
> > 	  --prefix $HOME/install/rutabaga_gfx/rutabaga_gfx_ffi/ \
> > 	  -Dvirglrenderer=true
> > 	ninja -C builddir install
> 
> Is there a PR going in for this? The moving parts for rutabaga are
> complex enough I think we need support upstream before merging this.
> 

It's true that this patch set depends on the change of
rutabaga_gfx_ffi. I am trying get the modifications of
crosvm/rubataga_gfx_ffi merged in upstream, please refer to this link:
https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5599645

> Is this branch where I should be getting the poll helpers from?
> 
>   cc -m64 @qemu-system-arm.rsp
>   /usr/bin/ld: libcommon.fa.p/hw_display_virtio-gpu-rutabaga.c.o: in function `virtio_gpu_fence_poll':
>   /home/alex/lsrc/qemu.git/builds/vulkan/../../hw/display/virtio-gpu-rutabaga.c:909: undefined reference to `rutabaga_poll'
>   /usr/bin/ld: libcommon.fa.p/hw_display_virtio-gpu-rutabaga.c.o: in function `virtio_gpu_rutabaga_init':
>   /home/alex/lsrc/qemu.git/builds/vulkan/../../hw/display/virtio-gpu-rutabaga.c:1122: undefined reference to `rutabaga_poll_descriptor'
>   collect2: error: ld returned 1 exit status
>   ninja: build stopped: subcommand failed.
> 

The required patches are applied to the rutabaga_ffi_virgl branch of my
clone of crosvm already, so please check out to that branch.

> 
> > 3. Applied this patch set to QEMU, build and install it:
> > 
> > 	cd qemu	
> > 	# Apply this patch set atop main branch ...
> > 	mkdir builddir; cd builddir
> > 	../configure --prefix=$INSTALL_DIR/qemu \
> > 	  --target-list=x86_64-softmmu \
> > 	  --disable-virglrenderer \
> > 	  --enable-rutabaga_gfx
> > 	ninja -C builddir install
> > 
> > 4. If you are lucky and everything goes fine, you are prepared to launch
> >    VM with virglrenderer backed virtio-gpu-rutabaga device:
> > 
> > 	export LD_LIBRARY_PATH=$INSTALL_DIR/virglrenderer/lib64/:$LD_LIBRARY_PATH
> > 	export LD_LIBRARY_PATH=$INSTALL_DIR/rutabaga_gfx_ffi/lib64/:$LD_LIBRARY_PATH
> > 	$INSTALL_DIR/qemu/bin/qemu-system-x86_64
> > 	$QEMU -d guest_errors -enable-kvm -M q35 -smp 4 -m $MEM \
> > 	  -object memory-backend-memfd,id=mem1,size=$MEM \
> > 	  -machine memory-backend=mem1 \
> > 	  -device virtio-vga-rutabaga,venus=on,virgl2=on,wsi=surfaceless,hostmem=$MEM \
> > 
> 
> This should go into docs/system/devices/virtio-gpu.rst with some
> explanation. Is there anything we need on the guest side or does this
> skip the encapsulating requirements of wayland?
> 

Yeah, it's a good idea to add doc to explain the usage, thanks!

Best regards,
Weifeng

> > Note:
> > 
> > - You might need this patch set [3] to avoid KVM bad address error when
> >   you are running on a GPU using TTM for memory management.
> > 
> > [1] https://lore.kernel.org/all/dba6eb97-e1d1-4694-bfb6-e72db95715dd@daynix.com/T/
> > [2] https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5599645/1
> > [3] https://lore.kernel.org/kvm/20240229025759.1187910-1-stevensd@google.com/
> > 
> > Weifeng Liu (3):
> >   virtio-gpu: rutabaga: Properly set stride when copying resources
> >   virtio-gpu: rutabaga: Poll rutabaga upon events
> >   virtio-gpu: rutabaga: Add options to enable virgl and venus contexts
> > 
> >  hw/display/virtio-gpu-rutabaga.c | 104 ++++++++++++++++++++++++++++++-
> >  include/hw/virtio/virtio-gpu.h   |   1 +
> >  2 files changed, 104 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/3] virtio-gpu: Enable virglrenderer backend for rutabaga

[Alex Bennée]

Weifeng Liu <weifeng.liu.z@gmail.com> writes:

> Hi Alex,
>
> On Thu, 2024-06-06 at 11:43 +0100, Alex Bennée wrote:
>> Weifeng Liu <weifeng.liu.z@gmail.com> writes:
>> 
>> > Greetings,
>> > 
>> > I'd like to introduce you my attempt to enable virglrenderer backend for
>> > rutabaga empowered virtio-gpu device.  I am aware that there have been
>> > effort in supporting venus in virtio-gpu-virgl.c [1], but there is no
>> > reason to prevent us from leveraging the virglrenderer component in
>> > rutabaga_gfx, especially it being not very hard to add this
>> > functionality.
>> > 
>> > Generally, the gap is the polling capability, i.e., virglrenderer
>> > requires the main thread (namely the GPU command handling thread) to
>> > poll virglrenderer at proper moments, which is not yet supported in
>> > virtio-gpu-rutabaga device. This patch set try to add this so that
>> > virglrenderer backend (including virgl and venus) can work as expected.
>> > 
>> > Slight change to rutabaga_gfx_ffi is also a requirement, which is
>> > included in [2].
>> > 
>> > Further effort is required to tune the performance, since copying is
>> > present before the rendered images get displayed. But I still think this
>> > patch set could be a good starting point for the pending work.
>> > 
>> > For those interested in setting up environment and playing around with
>> > this patch set, here is guideline in brief:
>> > 
>> > 1. Clone the master/main branch of virglrenderer, compile and install it.
>> > 
>> > 	git clone https://gitlab.freedesktop.org/virgl/virglrenderer
>> > 	cd virglrenderer
>> > 	meson setup builddir \
>> > 	  --prefix=$INSTALL_DIR/virglrenderer \
>> > 	  -Dvenus=true
>> > 	ninja -C builddir install
>> > 
>> > 2. Clone the patched CrosVM, build and install rutabaga_gfx_ffi.
>> > 
>> > 	git clone -b rutabaga_ffi_virgl https://github.com/phreer/crosvm.git
>> > 	cd crosvm/rutabaga_gfx/ffi
>> > 	export PKG_CONFIG_PATH=$INSTALL_DIR/virglrenderer/lib64/pkgconfig/
>> > 	meson setup builddir/ \
>> > 	  --prefix $HOME/install/rutabaga_gfx/rutabaga_gfx_ffi/ \
>> > 	  -Dvirglrenderer=true
>> > 	ninja -C builddir install
>> 
>> Is there a PR going in for this? The moving parts for rutabaga are
>> complex enough I think we need support upstream before merging this.
>> 
>
> It's true that this patch set depends on the change of
> rutabaga_gfx_ffi. I am trying get the modifications of
> crosvm/rubataga_gfx_ffi merged in upstream, please refer to this link:
> https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5599645
>
>> Is this branch where I should be getting the poll helpers from?
>> 
>>   cc -m64 @qemu-system-arm.rsp
>>   /usr/bin/ld: libcommon.fa.p/hw_display_virtio-gpu-rutabaga.c.o: in function `virtio_gpu_fence_poll':
>>   /home/alex/lsrc/qemu.git/builds/vulkan/../../hw/display/virtio-gpu-rutabaga.c:909: undefined reference to `rutabaga_poll'
>>   /usr/bin/ld: libcommon.fa.p/hw_display_virtio-gpu-rutabaga.c.o: in function `virtio_gpu_rutabaga_init':
>>   /home/alex/lsrc/qemu.git/builds/vulkan/../../hw/display/virtio-gpu-rutabaga.c:1122: undefined reference to `rutabaga_poll_descriptor'
>>   collect2: error: ld returned 1 exit status
>>   ninja: build stopped: subcommand failed.
>> 
>
> The required patches are applied to the rutabaga_ffi_virgl branch of my
> clone of crosvm already, so please check out to that branch.

I thought I had - maybe my install got polluted. I shall try again.

>
>> 
>> > 3. Applied this patch set to QEMU, build and install it:
>> > 
>> > 	cd qemu	
>> > 	# Apply this patch set atop main branch ...
>> > 	mkdir builddir; cd builddir
>> > 	../configure --prefix=$INSTALL_DIR/qemu \
>> > 	  --target-list=x86_64-softmmu \
>> > 	  --disable-virglrenderer \
>> > 	  --enable-rutabaga_gfx
>> > 	ninja -C builddir install
>> > 
>> > 4. If you are lucky and everything goes fine, you are prepared to launch
>> >    VM with virglrenderer backed virtio-gpu-rutabaga device:
>> > 
>> > 	export LD_LIBRARY_PATH=$INSTALL_DIR/virglrenderer/lib64/:$LD_LIBRARY_PATH
>> > 	export LD_LIBRARY_PATH=$INSTALL_DIR/rutabaga_gfx_ffi/lib64/:$LD_LIBRARY_PATH
>> > 	$INSTALL_DIR/qemu/bin/qemu-system-x86_64
>> > 	$QEMU -d guest_errors -enable-kvm -M q35 -smp 4 -m $MEM \
>> > 	  -object memory-backend-memfd,id=mem1,size=$MEM \
>> > 	  -machine memory-backend=mem1 \
>> > 	  -device virtio-vga-rutabaga,venus=on,virgl2=on,wsi=surfaceless,hostmem=$MEM \
>> > 
>> 
>> This should go into docs/system/devices/virtio-gpu.rst with some
>> explanation. Is there anything we need on the guest side or does this
>> skip the encapsulating requirements of wayland?
>> 
>
> Yeah, it's a good idea to add doc to explain the usage, thanks!
>
> Best regards,
> Weifeng
>
>> > Note:
>> > 
>> > - You might need this patch set [3] to avoid KVM bad address error when
>> >   you are running on a GPU using TTM for memory management.
>> > 
>> > [1] https://lore.kernel.org/all/dba6eb97-e1d1-4694-bfb6-e72db95715dd@daynix.com/T/
>> > [2] https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5599645/1
>> > [3] https://lore.kernel.org/kvm/20240229025759.1187910-1-stevensd@google.com/
>> > 
>> > Weifeng Liu (3):
>> >   virtio-gpu: rutabaga: Properly set stride when copying resources
>> >   virtio-gpu: rutabaga: Poll rutabaga upon events
>> >   virtio-gpu: rutabaga: Add options to enable virgl and venus contexts
>> > 
>> >  hw/display/virtio-gpu-rutabaga.c | 104 ++++++++++++++++++++++++++++++-
>> >  include/hw/virtio/virtio-gpu.h   |   1 +
>> >  2 files changed, 104 insertions(+), 1 deletion(-)
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/3] virtio-gpu: rutabaga: Properly set stride when copying resources

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

Hi

On Wed, Jun 5, 2024 at 7:30 PM Weifeng Liu <weifeng.liu.z@gmail.com> wrote:

> The stride is not correctly assigned when copying pixel data, causing
> images being displayed incomplete when using 2d component of rutabaga.
>
> Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
> ---
>  hw/display/virtio-gpu-rutabaga.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/display/virtio-gpu-rutabaga.c
> b/hw/display/virtio-gpu-rutabaga.c
> index 17bf701a21..2ba6869606 100644
> --- a/hw/display/virtio-gpu-rutabaga.c
> +++ b/hw/display/virtio-gpu-rutabaga.c
> @@ -53,6 +53,7 @@ virtio_gpu_rutabaga_update_cursor(VirtIOGPU *g, struct
> virtio_gpu_scanout *s,
>      transfer.z = 0;
>      transfer.w = res->width;
>      transfer.h = res->height;
> +    transfer.stride = res->width * 4;
>

ok, stride defined by QEMUCursor layout


>      transfer.d = 1;
>
>      transfer_iovec.iov_base = s->current_cursor->data;
> @@ -273,6 +274,7 @@ rutabaga_cmd_resource_flush(VirtIOGPU *g, struct
> virtio_gpu_ctrl_command *cmd)
>      transfer.z = 0;
>      transfer.w = res->width;
>      transfer.h = res->height;
> +    transfer.stride = pixman_image_get_stride(res->image);
>      transfer.d = 1;
>

ok (destination image stride)


>      transfer_iovec.iov_base = pixman_image_get_data(res->image);
> @@ -382,6 +384,7 @@ rutabaga_cmd_transfer_to_host_2d(VirtIOGPU *g,
>      transfer.z = 0;
>      transfer.w = t2d.r.width;
>      transfer.h = t2d.r.height;
> +    transfer.stride = t2d.r.width * 4;
>

here however, it's unclear to me what the stride could be, I think it could
depend on resource format (virgl doesn't set stride either).

Gurchetan?



>      transfer.d = 1;
>
>      result = rutabaga_resource_transfer_write(vr->rutabaga, 0,
> t2d.resource_id,
> --
> 2.45.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3085 bytes --]

Re: [PATCH 1/3] virtio-gpu: rutabaga: Properly set stride when copying resources

[Gurchetan Singh]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

On Mon, Jun 10, 2024 at 12:44 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Wed, Jun 5, 2024 at 7:30 PM Weifeng Liu <weifeng.liu.z@gmail.com>
> wrote:
>
>> The stride is not correctly assigned when copying pixel data, causing
>> images being displayed incomplete when using 2d component of rutabaga.
>>
>> Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
>> ---
>>  hw/display/virtio-gpu-rutabaga.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/display/virtio-gpu-rutabaga.c
>> b/hw/display/virtio-gpu-rutabaga.c
>> index 17bf701a21..2ba6869606 100644
>> --- a/hw/display/virtio-gpu-rutabaga.c
>> +++ b/hw/display/virtio-gpu-rutabaga.c
>> @@ -53,6 +53,7 @@ virtio_gpu_rutabaga_update_cursor(VirtIOGPU *g, struct
>> virtio_gpu_scanout *s,
>>      transfer.z = 0;
>>      transfer.w = res->width;
>>      transfer.h = res->height;
>> +    transfer.stride = res->width * 4;
>>
>
> ok, stride defined by QEMUCursor layout
>
>
>>      transfer.d = 1;
>>
>>      transfer_iovec.iov_base = s->current_cursor->data;
>> @@ -273,6 +274,7 @@ rutabaga_cmd_resource_flush(VirtIOGPU *g, struct
>> virtio_gpu_ctrl_command *cmd)
>>      transfer.z = 0;
>>      transfer.w = res->width;
>>      transfer.h = res->height;
>> +    transfer.stride = pixman_image_get_stride(res->image);
>>      transfer.d = 1;
>>
>
> ok (destination image stride)
>
>
>>      transfer_iovec.iov_base = pixman_image_get_data(res->image);
>> @@ -382,6 +384,7 @@ rutabaga_cmd_transfer_to_host_2d(VirtIOGPU *g,
>>      transfer.z = 0;
>>      transfer.w = t2d.r.width;
>>      transfer.h = t2d.r.height;
>> +    transfer.stride = t2d.r.width * 4;
>>
>
> here however, it's unclear to me what the stride could be, I think it
> could depend on resource format (virgl doesn't set stride either).
>
> Gurchetan?
>

gfxstream does more or less the same thing internally to deal with the lack
of stride.  Since virtio-gpu KMS only supports 4-bpp formats, this is fine,
so r-b for this particular patch.

That said, we actually don't use virtio-gpu-rutabaga for 2D mode or VirGL
mode, preferring to use the other more supported modes in QEMU for those
use cases.  I think there's actually some CI support for those in many
places.  So suggest just waiting until virglrenderer uprev lands in QEMU.



>
>
>
>>      transfer.d = 1;
>>
>>      result = rutabaga_resource_transfer_write(vr->rutabaga, 0,
>> t2d.resource_id,
>> --
>> 2.45.0
>>
>>
>>
>
> --
> Marc-André Lureau
>

[-- Attachment #2: Type: text/html, Size: 4285 bytes --]