| Message ID | 20230831093252.2461282-13-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
Hi Huang, Thank you for pushing this forward! On 31/08/2023 11:32, Huang Rui wrote: > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > Request Venus when initializing VirGL. > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > v1->v2: > - Rebase to latest version > > hw/display/virtio-gpu-virgl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 83cd8c8fd0..c5a62665bd 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -887,6 +887,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g) > } > #endif > > + flags |= VIRGL_RENDERER_VENUS; > + VIRGL_RENDERER_VENUS is a symbol only available from virglrenderer 0.9.1 [0] and only if VIRGL_RENDERER_UNSTABLE_APIS is defined. Luckily for us, VIRGL_RENDERER_UNSTABLE_APIS is defined unconditionally from virglrenderer 0.9.0 [1], so we could check for that in qemu/meson.build e.g. if virgl.version().version_compare('>= 0.9.0') message('Enabling virglrenderer unstable APIs') virgl = declare_dependency(compile_args: '-DVIRGL_RENDERER_UNSTABLE_APIS', dependencies: virgl) endif Also, while testing this with various versions of virglrenderer, I realized there are no guarantees for Venus backend to be available in the linked library. Virglrenderer should be built with -Dvenus_experimental=true, and if that is not the case, the following virgl_renderer_init would fail for previous versions of virglrenderer or in case it has not been built with venus support. I would suggest another approach for that which tries initializing Venus only if VIRGL_RENDERER_VENUS is actually defined. Then, if it fails cause virglrenderer has not been built with venus support, try again falling back to virgl only. e.g. #ifdef VIRGL_RENDERER_VENUS ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs); if (ret != 0) { warn_report("Failed to initialize virglrenderer with venus: %d", ret); warn_report("Falling back to virgl only"); ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs); } #else ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs); #endif > ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs); > if (ret != 0) { > error_report("virgl could not be initialized: %d", ret); [0] https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/6c31f85330bb4c5aba8b82eba606971e598c6e25 [1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/491afdc42a49ec6a1b8d7cbc5c97360229002d41 Best regards, Antonio Caggiano
On 8/31/23 13:40, Antonio Caggiano wrote: > Hi Huang, > > Thank you for pushing this forward! > > On 31/08/2023 11:32, Huang Rui wrote: >> From: Antonio Caggiano <antonio.caggiano@collabora.com> >> >> Request Venus when initializing VirGL. >> >> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> --- >> >> v1->v2: >> - Rebase to latest version >> >> hw/display/virtio-gpu-virgl.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/display/virtio-gpu-virgl.c >> b/hw/display/virtio-gpu-virgl.c >> index 83cd8c8fd0..c5a62665bd 100644 >> --- a/hw/display/virtio-gpu-virgl.c >> +++ b/hw/display/virtio-gpu-virgl.c >> @@ -887,6 +887,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g) >> } >> #endif >> + flags |= VIRGL_RENDERER_VENUS; >> + > > VIRGL_RENDERER_VENUS is a symbol only available from virglrenderer 0.9.1 > [0] and only if VIRGL_RENDERER_UNSTABLE_APIS is defined. > > Luckily for us, VIRGL_RENDERER_UNSTABLE_APIS is defined unconditionally > from virglrenderer 0.9.0 [1], so we could check for that in > qemu/meson.build > > e.g. > > > if virgl.version().version_compare('>= 0.9.0') > message('Enabling virglrenderer unstable APIs') > virgl = declare_dependency(compile_args: > '-DVIRGL_RENDERER_UNSTABLE_APIS', > dependencies: virgl) > endif > > > Also, while testing this with various versions of virglrenderer, I > realized there are no guarantees for Venus backend to be available in > the linked library. Virglrenderer should be built with > -Dvenus_experimental=true, and if that is not the case, the following > virgl_renderer_init would fail for previous versions of virglrenderer or > in case it has not been built with venus support. > > I would suggest another approach for that which tries initializing Venus > only if VIRGL_RENDERER_VENUS is actually defined. Then, if it fails > cause virglrenderer has not been built with venus support, try again > falling back to virgl only. All the APIs will be stabilized with the upcoming virglrender 1.0 release that will happen soon. There is also a venus protocol bump, qemu will have to bump virglrenderer's version dependency to 1.0 for venus and other new features.
On Thu, Aug 31, 2023 at 06:40:11PM +0800, Antonio Caggiano wrote: > Hi Huang, > > Thank you for pushing this forward! > My pleasure! :-) > On 31/08/2023 11:32, Huang Rui wrote: > > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > > > Request Venus when initializing VirGL. > > > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > > > v1->v2: > > - Rebase to latest version > > > > hw/display/virtio-gpu-virgl.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > > index 83cd8c8fd0..c5a62665bd 100644 > > --- a/hw/display/virtio-gpu-virgl.c > > +++ b/hw/display/virtio-gpu-virgl.c > > @@ -887,6 +887,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g) > > } > > #endif > > > > + flags |= VIRGL_RENDERER_VENUS; > > + > > VIRGL_RENDERER_VENUS is a symbol only available from virglrenderer 0.9.1 > [0] and only if VIRGL_RENDERER_UNSTABLE_APIS is defined. > > Luckily for us, VIRGL_RENDERER_UNSTABLE_APIS is defined unconditionally > from virglrenderer 0.9.0 [1], so we could check for that in qemu/meson.build > > e.g. > > > if virgl.version().version_compare('>= 0.9.0') > message('Enabling virglrenderer unstable APIs') > virgl = declare_dependency(compile_args: > '-DVIRGL_RENDERER_UNSTABLE_APIS', > dependencies: virgl) > endif > > > Also, while testing this with various versions of virglrenderer, I > realized there are no guarantees for Venus backend to be available in > the linked library. Virglrenderer should be built with > -Dvenus_experimental=true, and if that is not the case, the following > virgl_renderer_init would fail for previous versions of virglrenderer or > in case it has not been built with venus support. > > I would suggest another approach for that which tries initializing Venus > only if VIRGL_RENDERER_VENUS is actually defined. Then, if it fails > cause virglrenderer has not been built with venus support, try again > falling back to virgl only. > > e.g. > > #ifdef VIRGL_RENDERER_VENUS > ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs); > if (ret != 0) { > warn_report("Failed to initialize virglrenderer with venus: > %d", ret); > warn_report("Falling back to virgl only"); > ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs); > } > #else > ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs); > #endif > Thanks a lot to explain so clearly. Yes, it's reasonable for me. I will modify it in the next version. And agree, we should take care of different virglrenderer versions. Thanks, Ray > > > ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs); > > if (ret != 0) { > > error_report("virgl could not be initialized: %d", ret); > > [0] > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/6c31f85330bb4c5aba8b82eba606971e598c6e25 > [1] > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/491afdc42a49ec6a1b8d7cbc5c97360229002d41 > > Best regards, > Antonio Caggiano
On Thu, Aug 31, 2023 at 11:51:50PM +0800, Dmitry Osipenko wrote: > On 8/31/23 13:40, Antonio Caggiano wrote: > > Hi Huang, > > > > Thank you for pushing this forward! > > > > On 31/08/2023 11:32, Huang Rui wrote: > >> From: Antonio Caggiano <antonio.caggiano@collabora.com> > >> > >> Request Venus when initializing VirGL. > >> > >> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > >> Signed-off-by: Huang Rui <ray.huang@amd.com> > >> --- > >> > >> v1->v2: > >> - Rebase to latest version > >> > >> hw/display/virtio-gpu-virgl.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/hw/display/virtio-gpu-virgl.c > >> b/hw/display/virtio-gpu-virgl.c > >> index 83cd8c8fd0..c5a62665bd 100644 > >> --- a/hw/display/virtio-gpu-virgl.c > >> +++ b/hw/display/virtio-gpu-virgl.c > >> @@ -887,6 +887,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g) > >> } > >> #endif > >> + flags |= VIRGL_RENDERER_VENUS; > >> + > > > > VIRGL_RENDERER_VENUS is a symbol only available from virglrenderer 0.9.1 > > [0] and only if VIRGL_RENDERER_UNSTABLE_APIS is defined. > > > > Luckily for us, VIRGL_RENDERER_UNSTABLE_APIS is defined unconditionally > > from virglrenderer 0.9.0 [1], so we could check for that in > > qemu/meson.build > > > > e.g. > > > > > > if virgl.version().version_compare('>= 0.9.0') > > message('Enabling virglrenderer unstable APIs') > > virgl = declare_dependency(compile_args: > > '-DVIRGL_RENDERER_UNSTABLE_APIS', > > dependencies: virgl) > > endif > > > > > > Also, while testing this with various versions of virglrenderer, I > > realized there are no guarantees for Venus backend to be available in > > the linked library. Virglrenderer should be built with > > -Dvenus_experimental=true, and if that is not the case, the following > > virgl_renderer_init would fail for previous versions of virglrenderer or > > in case it has not been built with venus support. > > > > I would suggest another approach for that which tries initializing Venus > > only if VIRGL_RENDERER_VENUS is actually defined. Then, if it fails > > cause virglrenderer has not been built with venus support, try again > > falling back to virgl only. > > All the APIs will be stabilized with the upcoming virglrender 1.0 > release that will happen soon. There is also a venus protocol bump, qemu > will have to bump virglrenderer's version dependency to 1.0 for venus > and other new features. > Dmitry, do you know the timeline of virglrender 1.0? Thanks, Ray
On 9/9/23 13:53, Huang Rui wrote: > On Thu, Aug 31, 2023 at 11:51:50PM +0800, Dmitry Osipenko wrote: >> On 8/31/23 13:40, Antonio Caggiano wrote: >>> Hi Huang, >>> >>> Thank you for pushing this forward! >>> >>> On 31/08/2023 11:32, Huang Rui wrote: >>>> From: Antonio Caggiano <antonio.caggiano@collabora.com> >>>> >>>> Request Venus when initializing VirGL. >>>> >>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> >>>> v1->v2: >>>> - Rebase to latest version >>>> >>>> hw/display/virtio-gpu-virgl.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/hw/display/virtio-gpu-virgl.c >>>> b/hw/display/virtio-gpu-virgl.c >>>> index 83cd8c8fd0..c5a62665bd 100644 >>>> --- a/hw/display/virtio-gpu-virgl.c >>>> +++ b/hw/display/virtio-gpu-virgl.c >>>> @@ -887,6 +887,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g) >>>> } >>>> #endif >>>> + flags |= VIRGL_RENDERER_VENUS; >>>> + >>> >>> VIRGL_RENDERER_VENUS is a symbol only available from virglrenderer 0.9.1 >>> [0] and only if VIRGL_RENDERER_UNSTABLE_APIS is defined. >>> >>> Luckily for us, VIRGL_RENDERER_UNSTABLE_APIS is defined unconditionally >>> from virglrenderer 0.9.0 [1], so we could check for that in >>> qemu/meson.build >>> >>> e.g. >>> >>> >>> if virgl.version().version_compare('>= 0.9.0') >>> message('Enabling virglrenderer unstable APIs') >>> virgl = declare_dependency(compile_args: >>> '-DVIRGL_RENDERER_UNSTABLE_APIS', >>> dependencies: virgl) >>> endif >>> >>> >>> Also, while testing this with various versions of virglrenderer, I >>> realized there are no guarantees for Venus backend to be available in >>> the linked library. Virglrenderer should be built with >>> -Dvenus_experimental=true, and if that is not the case, the following >>> virgl_renderer_init would fail for previous versions of virglrenderer or >>> in case it has not been built with venus support. >>> >>> I would suggest another approach for that which tries initializing Venus >>> only if VIRGL_RENDERER_VENUS is actually defined. Then, if it fails >>> cause virglrenderer has not been built with venus support, try again >>> falling back to virgl only. >> >> All the APIs will be stabilized with the upcoming virglrender 1.0 >> release that will happen soon. There is also a venus protocol bump, qemu >> will have to bump virglrenderer's version dependency to 1.0 for venus >> and other new features. >> > > Dmitry, do you know the timeline of virglrender 1.0? Should be end of this week or next week
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index 83cd8c8fd0..c5a62665bd 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -887,6 +887,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g) } #endif + flags |= VIRGL_RENDERER_VENUS; + ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs); if (ret != 0) { error_report("virgl could not be initialized: %d", ret);