diff mbox series

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

Message ID 20230831093252.2461282-13-ray.huang@amd.com (mailing list archive)
State New, archived
Headers show
Series Support blob memory and venus on qemu | expand

Commit Message

Huang Rui Aug. 31, 2023, 9:32 a.m. UTC 
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(+)

Comments

Antonio Caggiano Aug. 31, 2023, 10:40 a.m. UTC | #1 
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
Dmitry Osipenko Aug. 31, 2023, 3:51 p.m. UTC | #2 
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.
Huang Rui Sept. 9, 2023, 10:52 a.m. UTC | #3 
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
Huang Rui Sept. 9, 2023, 10:53 a.m. UTC | #4 
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
Dmitry Osipenko Sept. 12, 2023, 1:44 p.m. UTC | #5 
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 mbox series

Patch

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