| Message ID | 20240605133527.529950-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] subprojects: add a wrapper for libvirglrenderer | expand |
Hi On Wed, Jun 5, 2024 at 5:35 PM Alex Bennée <alex.bennee@linaro.org> 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. > > 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']) > So the subproject won't be used unless virgl-devel is missing on the system. Is it really so useful? maybe, I am just used to installing my bleeding edge libraries with stow.. > > 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. > + * > + * 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)) { > + 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 > -- > 2.39.2 > > >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Wed, Jun 5, 2024 at 5:35 PM Alex Bennée <alex.bennee@linaro.org> 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. > > 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']) > > So the subproject won't be used unless virgl-devel is missing on the system. Is it really so useful? maybe, I am just used to > installing my bleeding edge libraries with stow.. While I've been messing with the VirtIO GPU stuff I've had a special build dir with the latest version of the various components and suitable PKG_CONFIG_PATH and LD_LIBRARY_PATH hacks to use them. It did however keep breaking when there was a re-configure and I'd forgotten to set the env variables. Judging by the number of questions I keep seeing from people about getting virtio-gpu working it does seem a bit wobly (and thats before you start talking about the extra libs rutabaga requires). Bundling the library with a wrapper (which I think it only does when it can't find a new enough system one) seems a lot simpler. So yes this is a build from source convenience that mostly helps developers and early adopters but we can get rid of it once our baseline distros have caught up with the latest libvirglrenderer.
On Wed, 05 Jun 2024 16:35, Alex Bennée <alex.bennee@linaro.org> 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. > >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']) > 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. >+ * >+ * The principle way we can override the libvirglrenders behaviour is >+ * by setting environment variables. >+ */ >+static void virgl_set_render_env(void) >+{ Since it's a few lines we could also inline this in virtio_gpu_virgl_init() >+#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)) { >+ g_setenv("RENDER_SERVER_EXEC_PATH", file, false); Return value Type: gboolean FALSE if the environment variable couldn’t be set. Worth adding a check here. Offtopic, but it feels weird to set our environment without creating the process ourselves. There's also an option to launch the server in threads, so that if it crashes it pulls qemu down with it. Would that work with our thread setup? >+ } >+#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 Can we say "at least 1.0.1" here? Should we? Dunno. >+ >+[provide] >+virglrenderer = libvirglrenderer_dep >-- >2.39.2 >
Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > On Wed, 05 Jun 2024 16:35, Alex Bennée <alex.bennee@linaro.org> 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. >> >>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']) >> 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. >>+ * >>+ * The principle way we can override the libvirglrenders behaviour is >>+ * by setting environment variables. >>+ */ >>+static void virgl_set_render_env(void) >>+{ > > Since it's a few lines we could also inline this in > virtio_gpu_virgl_init() I mainly put it in a helper function to avoid putting a big comment block inline ;-) > >>+#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)) { >>+ g_setenv("RENDER_SERVER_EXEC_PATH", file, false); > > > Return value > > Type: gboolean > > FALSE if the environment variable couldn’t be set. > > Worth adding a check here. ok. > Offtopic, but it feels weird to set our environment without creating > the process ourselves. yeah I'm not overly happy with the way the proxy server stuff works because its a bit spooky action at a distance (as noted by the failure to detect it not working). Not sure if we want to replicate all the proxy login in QEMU though. > There's also an option to launch the server in threads, so that if it > crashes it pulls qemu down with it. Would that work with our thread > setup? Hmm not sure. We could try but I suspect we want to insulate the main process from the vagaries of the helper. > > >>+ } >>+#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 > > > Can we say "at least 1.0.1" here? Should we? Dunno. > >>+ >>+[provide] >>+virglrenderer = libvirglrenderer_dep >> -- 2.39.2 >>
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
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > 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. I was thinking mainly of validating it was built, maybe that should be an assert instead because if we failed to build the server we got the bundling wrong? > >> + 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
On 2024/06/17 19:35, Alex Bennée wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> 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. > > I was thinking mainly of validating it was built, maybe that should be > an assert instead because if we failed to build the server we got the > bundling wrong? virglrenderer should emit errors and refuse to function in such a case so we don't have to do that ourselves.
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']) 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. + * + * 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)) { + 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
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. 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