subprojects: add a wrapper for libvirglrenderer

[RFC PATCH] subprojects: add a wrapper for libvirglrenderer

Posted by Alex Bennée 5 months, 3 weeks ago

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)
+{
+#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

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

Posted by Akihiko Odaki 5 months, 3 weeks ago

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

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

Posted by Alex Bennée 5 months, 1 week ago

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Posted by Akihiko Odaki 5 months, 1 week ago

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.

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

Posted by Manos Pitsidianakis 5 months, 3 weeks ago

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
>

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

Posted by Alex Bennée 5 months, 3 weeks ago

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
>>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Posted by Marc-André Lureau 5 months, 3 weeks ago

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

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

Posted by Alex Bennée 5 months, 3 weeks ago

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. 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro