Re: [RFC PATCH] subprojects: add a wrapper for libvirglrenderer

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/06/05 22:35, Alex Bennée wrote:
> As the latest features for virtio-gpu need a pretty recent version of
> libvirglrenderer. When it is not available on the system we can use a
> meson wrapper and provide it when --download is specified in
> configure.
> 
> We have to take some additional care as currently QEMU will hang
> libvirglrenderer fails to exec the render server. As the error isn't
> back propagated we make sure we at least test we have a path to an
> executable before tweaking the environment.

Hi,

The intent of this patch sounds good to me. It is the responsibility of 
users to set up virglrenderer in principle, but we can just be kind for 
them.

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   meson.build                    |  7 ++++++-
>   hw/display/virtio-gpu-virgl.c  | 24 ++++++++++++++++++++++++
>   subprojects/virglrenderer.wrap |  6 ++++++
>   3 files changed, 36 insertions(+), 1 deletion(-)
>   create mode 100644 subprojects/virglrenderer.wrap
> 
> diff --git a/meson.build b/meson.build
> index 1d7346b703..e4e270df78 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1203,7 +1203,8 @@ have_vhost_user_gpu = have_tools and host_os == 'linux' and pixman.found()
>   if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>     virgl = dependency('virglrenderer',
>                        method: 'pkg-config',
> -                     required: get_option('virglrenderer'))
> +                     required: get_option('virglrenderer'),
> +                     default_options: ['default_library=static', 'render-server=true', 'venus=true'])

meson_options.txt of virglrenderer says:
 > DEPRECATED: render server is enabled by venus automatically

I'm also a bit concerned to enable Venus by default when the upstream 
virglrenderer doesn't. Why is it disabled by the upstream? Perhaps is it 
time for upstream to enable it by default?

>   endif
>   rutabaga = not_found
>   if not get_option('rutabaga_gfx').auto() or have_system or have_vhost_user_gpu
> @@ -2314,6 +2315,10 @@ if virgl.version().version_compare('>=1.0.0')
>     config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB', 1)
>     config_host_data.set('HAVE_VIRGL_VENUS', 1)
>   endif
> +if virgl.type_name().contains('internal')
> +  config_host_data.set('HAVE_BUNDLED_VIRGL_SERVER', 1)
> +endif
> +
>   config_host_data.set('CONFIG_VIRTFS', have_virtfs)
>   config_host_data.set('CONFIG_VTE', vte.found())
>   config_host_data.set('CONFIG_XKBCOMMON', xkbcommon.found())
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index c9d20a8a60..53d6742e79 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -14,6 +14,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/error-report.h"
>   #include "qemu/iov.h"
> +#include "qemu/cutils.h"
>   #include "trace.h"
>   #include "hw/virtio/virtio.h"
>   #include "hw/virtio/virtio-gpu.h"
> @@ -1122,6 +1123,26 @@ void virtio_gpu_virgl_reset(VirtIOGPU *g)
>       virgl_renderer_reset();
>   }
>   
> +/*
> + * If we fail to spawn the render server things tend to hang so it is
> + * important to do our due diligence before then. If QEMU has bundled
> + * the virgl server we want to ensure we can run it from the build
> + * directory and if installed.

This comment sounds a bit misleading. The following code does not ensure 
the render server exists; it just opportunistically sets the path to the 
bundled render server or let it fail otherwise.

It also sounds like virgl_set_render_env() does an extra step to prevent 
hangs, but it is actually mandatory for relocated scenarios; the lack of 
render server always results in a non-functional Venus setup even if the 
hang is fixed.

The hang is better to be noted in subprojects/virglrenderer.wrap since 
that is the reason we would want to wrap the project.

> + *
> + * The principle way we can override the libvirglrenders behaviour is
> + * by setting environment variables.
> + */
> +static void virgl_set_render_env(void)
> +{
> +#ifdef HAVE_BUNDLED_VIRGL_SERVER
> +    g_autofree char *file = get_relocated_path(CONFIG_QEMU_HELPERDIR "/virgl_render_server");
> +    if (g_file_test(file, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_EXECUTABLE)) {

I think this g_file_test() should be removed; we would not want to let 
virglrenderer pick a random render server when the bundled server exists 
since the ABI between them can be different in theory.

> +        g_setenv("RENDER_SERVER_EXEC_PATH", file, false);
> +    }
> +#endif
> +}
> +
> +
>   int virtio_gpu_virgl_init(VirtIOGPU *g)
>   {
>       int ret;
> @@ -1145,6 +1166,9 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>       }
>   #endif
>   
> +    /* Ensure we can find the render server */
> +    virgl_set_render_env();
> +
>       ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
>       if (ret != 0) {
>           error_report("virgl could not be initialized: %d", ret);
> diff --git a/subprojects/virglrenderer.wrap b/subprojects/virglrenderer.wrap
> new file mode 100644
> index 0000000000..3656a478c4
> --- /dev/null
> +++ b/subprojects/virglrenderer.wrap
> @@ -0,0 +1,6 @@
> +[wrap-git]
> +url = https://gitlab.freedesktop.org/virgl/virglrenderer.git
> +revision = virglrenderer-1.0.1
> +
> +[provide]
> +virglrenderer = libvirglrenderer_dep