diff mbox series

[RFC,QEMU,08/18] virtio-gpu: Initialize Venus

Message ID 20230312092244.451465-9-ray.huang@amd.com (mailing list archive)
State New, archived
Headers show
Series Add VirtIO GPU and Passthrough GPU support on Xen | expand

Commit Message

Huang Rui March 12, 2023, 9:22 a.m. UTC 
From: Antonio Caggiano <antonio.caggiano@collabora.com>

Request Venus when initializing VirGL.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dmitry Osipenko March 12, 2023, 5:51 p.m. UTC | #1 
On 3/12/23 12:22, Huang Rui wrote:
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> Request Venus when initializing VirGL.
> 
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index fe03dc916f..f5ce206b93 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>  {
>      int ret;
>  
> +#ifdef VIRGL_RENDERER_VENUS
> +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
> +#else
>      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
> +#endif

Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
set. Please test the patches with the latest virglrenderer and etc.

The #ifdef also doesn't allow adding new flags, it should look like:

#ifdef VIRGL_RENDERER_VENUS
    flags |= VIRGL_RENDERER_RENDER_SERVER;
#endif

    ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
Dmitry Osipenko March 13, 2023, 2:22 a.m. UTC | #2 
On 3/12/23 20:51, Dmitry Osipenko wrote:
> On 3/12/23 12:22, Huang Rui wrote:
>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>
>> Request Venus when initializing VirGL.
>>
>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>> ---
>>  hw/display/virtio-gpu-virgl.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index fe03dc916f..f5ce206b93 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>>  {
>>      int ret;
>>  
>> +#ifdef VIRGL_RENDERER_VENUS
>> +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
>> +#else
>>      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
>> +#endif
> 
> Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
> set. Please test the patches with the latest virglrenderer and etc.
> 
> The #ifdef also doesn't allow adding new flags, it should look like:
> 
> #ifdef VIRGL_RENDERER_VENUS
>     flags |= VIRGL_RENDERER_RENDER_SERVER;
> #endif
> 
>     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
> 

BTW, Alex reviewed the Venus v3 patches a month ago [1] and the review
comments need to be addressed. AFAICS, you're actually using the very
old Venus patches here that stopped working about a year ago, so again
you're using a very outdated virglrenderer version.

Please take it all into account if you'll beat me to posting the next
version of Venus patches ;)

[1]
https://lore.kernel.org/qemu-devel/20220926142422.22325-1-antonio.caggiano@collabora.com/
Huang Rui March 13, 2023, 3:55 p.m. UTC | #3 
On Mon, Mar 13, 2023 at 01:51:03AM +0800, Dmitry Osipenko wrote:
> On 3/12/23 12:22, Huang Rui wrote:
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> > 
> > Request Venus when initializing VirGL.
> > 
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > ---
> >  hw/display/virtio-gpu-virgl.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index fe03dc916f..f5ce206b93 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> >  {
> >      int ret;
> >  
> > +#ifdef VIRGL_RENDERER_VENUS
> > +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
> > +#else
> >      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
> > +#endif
> 
> Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
> set. Please test the patches with the latest virglrenderer and etc.
> 
> The #ifdef also doesn't allow adding new flags, it should look like:
> 
> #ifdef VIRGL_RENDERER_VENUS
>     flags |= VIRGL_RENDERER_RENDER_SERVER;
> #endif
> 
>     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);

In fact, we have rebased to the latest virglrenderer:

We check both VIRGL_RENDERER_RENDER_SERVER or VIRGL_RENDERER_VENUS in
virglrenderer, alternative of them works.

https://gitlab.freedesktop.org/rui/virglrenderer/-/commit/c1322a8a84379b1ef7939f56c6761b0114716f45

Thanks,
Ray
Huang Rui March 13, 2023, 3:57 p.m. UTC | #4 
On Mon, Mar 13, 2023 at 10:22:24AM +0800, Dmitry Osipenko wrote:
> On 3/12/23 20:51, Dmitry Osipenko wrote:
> > On 3/12/23 12:22, Huang Rui wrote:
> >> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>
> >> Request Venus when initializing VirGL.
> >>
> >> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> >> ---
> >>  hw/display/virtio-gpu-virgl.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >> index fe03dc916f..f5ce206b93 100644
> >> --- a/hw/display/virtio-gpu-virgl.c
> >> +++ b/hw/display/virtio-gpu-virgl.c
> >> @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> >>  {
> >>      int ret;
> >>  
> >> +#ifdef VIRGL_RENDERER_VENUS
> >> +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
> >> +#else
> >>      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
> >> +#endif
> > 
> > Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
> > set. Please test the patches with the latest virglrenderer and etc.
> > 
> > The #ifdef also doesn't allow adding new flags, it should look like:
> > 
> > #ifdef VIRGL_RENDERER_VENUS
> >     flags |= VIRGL_RENDERER_RENDER_SERVER;
> > #endif
> > 
> >     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
> > 
> 
> BTW, Alex reviewed the Venus v3 patches a month ago [1] and the review
> comments need to be addressed. AFAICS, you're actually using the very
> old Venus patches here that stopped working about a year ago, so again
> you're using a very outdated virglrenderer version.
> 
> Please take it all into account if you'll beat me to posting the next
> version of Venus patches ;)
> 
> [1]
> https://lore.kernel.org/qemu-devel/20220926142422.22325-1-antonio.caggiano@collabora.com/
> 

Thanks Dmitry point it out, I will use the latest v3 patches, and try to
address the comments in next version. :-)

Thanks,
Ray
Dmitry Osipenko March 15, 2023, 11:14 p.m. UTC | #5 
On 3/13/23 18:55, Huang Rui wrote:
> On Mon, Mar 13, 2023 at 01:51:03AM +0800, Dmitry Osipenko wrote:
>> On 3/12/23 12:22, Huang Rui wrote:
>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>
>>> Request Venus when initializing VirGL.
>>>
>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>> ---
>>>  hw/display/virtio-gpu-virgl.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>> index fe03dc916f..f5ce206b93 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>>>  {
>>>      int ret;
>>>  
>>> +#ifdef VIRGL_RENDERER_VENUS
>>> +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
>>> +#else
>>>      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
>>> +#endif
>>
>> Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
>> set. Please test the patches with the latest virglrenderer and etc.
>>
>> The #ifdef also doesn't allow adding new flags, it should look like:
>>
>> #ifdef VIRGL_RENDERER_VENUS
>>     flags |= VIRGL_RENDERER_RENDER_SERVER;
>> #endif
>>
>>     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
> 
> In fact, we have rebased to the latest virglrenderer:
> 
> We check both VIRGL_RENDERER_RENDER_SERVER or VIRGL_RENDERER_VENUS in
> virglrenderer, alternative of them works.
> 
> https://gitlab.freedesktop.org/rui/virglrenderer/-/commit/c1322a8a84379b1ef7939f56c6761b0114716f45

All the extra changes you made to virglrenderer that Qemu depends on
need to go upstream. Please open all the relevant merge requests. Thanks!
Huang Rui March 24, 2023, 1:22 p.m. UTC | #6 
On Thu, Mar 16, 2023 at 07:14:47AM +0800, Dmitry Osipenko wrote:
> On 3/13/23 18:55, Huang Rui wrote:
> > On Mon, Mar 13, 2023 at 01:51:03AM +0800, Dmitry Osipenko wrote:
> >> On 3/12/23 12:22, Huang Rui wrote:
> >>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>>
> >>> Request Venus when initializing VirGL.
> >>>
> >>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>> ---
> >>>  hw/display/virtio-gpu-virgl.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>> index fe03dc916f..f5ce206b93 100644
> >>> --- a/hw/display/virtio-gpu-virgl.c
> >>> +++ b/hw/display/virtio-gpu-virgl.c
> >>> @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> >>>  {
> >>>      int ret;
> >>>  
> >>> +#ifdef VIRGL_RENDERER_VENUS
> >>> +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
> >>> +#else
> >>>      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
> >>> +#endif
> >>
> >> Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
> >> set. Please test the patches with the latest virglrenderer and etc.
> >>
> >> The #ifdef also doesn't allow adding new flags, it should look like:
> >>
> >> #ifdef VIRGL_RENDERER_VENUS
> >>     flags |= VIRGL_RENDERER_RENDER_SERVER;
> >> #endif
> >>
> >>     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
> > 
> > In fact, we have rebased to the latest virglrenderer:
> > 
> > We check both VIRGL_RENDERER_RENDER_SERVER or VIRGL_RENDERER_VENUS in
> > virglrenderer, alternative of them works.
> > 
> > https://gitlab.freedesktop.org/rui/virglrenderer/-/commit/c1322a8a84379b1ef7939f56c6761b0114716f45
> 
> All the extra changes you made to virglrenderer that Qemu depends on
> need to go upstream. Please open all the relevant merge requests. Thanks!
> 

Dmitry, sorry to late response, I have created relevant merge requests
below:

Virglrenderer:
https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1068

Mesa:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22108

I'd appreciate any comments. :-)

Thanks,
Ray
Dmitry Osipenko April 3, 2023, 9:03 p.m. UTC | #7 
On 3/24/23 16:22, Huang Rui wrote:
> On Thu, Mar 16, 2023 at 07:14:47AM +0800, Dmitry Osipenko wrote:
>> On 3/13/23 18:55, Huang Rui wrote:
>>> On Mon, Mar 13, 2023 at 01:51:03AM +0800, Dmitry Osipenko wrote:
>>>> On 3/12/23 12:22, Huang Rui wrote:
>>>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>>>
>>>>> Request Venus when initializing VirGL.
>>>>>
>>>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>>> ---
>>>>>  hw/display/virtio-gpu-virgl.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>>> index fe03dc916f..f5ce206b93 100644
>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>> @@ -803,7 +803,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>>>>>  {
>>>>>      int ret;
>>>>>  
>>>>> +#ifdef VIRGL_RENDERER_VENUS
>>>>> +    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
>>>>> +#else
>>>>>      ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
>>>>> +#endif
>>>>
>>>> Note that Venus now requires VIRGL_RENDERER_RENDER_SERVER flag to be
>>>> set. Please test the patches with the latest virglrenderer and etc.
>>>>
>>>> The #ifdef also doesn't allow adding new flags, it should look like:
>>>>
>>>> #ifdef VIRGL_RENDERER_VENUS
>>>>     flags |= VIRGL_RENDERER_RENDER_SERVER;
>>>> #endif
>>>>
>>>>     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
>>>
>>> In fact, we have rebased to the latest virglrenderer:
>>>
>>> We check both VIRGL_RENDERER_RENDER_SERVER or VIRGL_RENDERER_VENUS in
>>> virglrenderer, alternative of them works.
>>>
>>> https://gitlab.freedesktop.org/rui/virglrenderer/-/commit/c1322a8a84379b1ef7939f56c6761b0114716f45
>>
>> All the extra changes you made to virglrenderer that Qemu depends on
>> need to go upstream. Please open all the relevant merge requests. Thanks!
>>
> 
> Dmitry, sorry to late response, I have created relevant merge requests
> below:
> 
> Virglrenderer:
> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1068
> 
> Mesa:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22108
> 
> I'd appreciate any comments. :-)

Thanks, Ray. I'll try to get to the patches soon.
diff mbox series

Patch

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index fe03dc916f..f5ce206b93 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -803,7 +803,11 @@  int virtio_gpu_virgl_init(VirtIOGPU *g)
 {
     int ret;
 
+#ifdef VIRGL_RENDERER_VENUS
+    ret = virgl_renderer_init(g, VIRGL_RENDERER_VENUS, &virtio_gpu_3d_cbs);
+#else
     ret = virgl_renderer_init(g, 0, &virtio_gpu_3d_cbs);
+#endif
     if (ret != 0) {
         error_report("virgl could not be initialized: %d", ret);
         return ret;