Re: [QEMU PATCH v4 12/13] virtio-gpu: Initialize Venus

[Huang Rui <ray.huang@xxxxxxx>]

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@xxxxxxxxxxxxx>
> > 
> > Request Venus when initializing VirGL.
> > 
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@xxxxxxxxxxxxx>
> > Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> > ---
> > 
> > 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