Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

2023-10-17 Thread Dmitry Osipenko 
On 10/10/23 14:51, Daniel P. Berrangé wrote:
> On Tue, Oct 10, 2023 at 02:41:22PM +0300, Dmitry Osipenko wrote:
>> On 9/15/23 14:11, Huang Rui wrote:
>>> Configure context init feature flag for virglrenderer.
>>>
>>> Originally-by: Antonio Caggiano 
>>> Signed-off-by: Huang Rui 
>>> ---
>>>
>>> V4 -> V5:
>>> - Inverted patch 5 and 6 because we should configure
>>>   HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>
>>>  meson.build | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 98e68ef0b1..ff20d3c249 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
>>> have_system or have_vhost_user_gpu
>>> prefix: '#include 
>>> ',
>>> dependencies: virgl))
>>>endif
>>> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
>>> +   
>>> cc.has_function('virgl_renderer_context_create_with_flags',
>>> +   prefix: '#include 
>>> ',
>>> +   dependencies: virgl))
>> The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores 
>> the the given pkg and uses system includes. Antonio was aware about that 
>> problem [1].
>>
>> [1] 
>> https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd
>>
>> Given that virglrenderer 1.0 has been released couple weeks ago,
>> can we make the v1.0 a mandatory requirement for qemu and remove
>> all the ifdefs? I doubt that anyone is going to test newer qemu
>> using older libviglrenderer, all that ifdef code will be bitrotting.
> 
> We cannot do that. If is is only weeks old, no distro will
> have virglrenderer 1.0 present. QEMU has a defined set of
> platforms that it targets compatibility with:
> 
>   https://www.qemu.org/docs/master/about/build-platforms.html
> 
> For newly added functionality we can set the min version to
> something newer than the oldest QEMU target platform.
> 
> For existing functionality though, we must not regress wrt
> the currently supported set of target platforms. So we can
> only bump the min version to that present in the oldest
> platform we target.

Well, then alternatively we could specify supported libvirglrender
features based on the lib version to avoid those pkgconfig+meson troubles.

-- 
Best regards,
Dmitry

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

2023-10-10 Thread Daniel P . Berrangé 
On Tue, Oct 10, 2023 at 02:41:22PM +0300, Dmitry Osipenko wrote:
> On 9/15/23 14:11, Huang Rui wrote:
> > Configure context init feature flag for virglrenderer.
> > 
> > Originally-by: Antonio Caggiano 
> > Signed-off-by: Huang Rui 
> > ---
> > 
> > V4 -> V5:
> > - Inverted patch 5 and 6 because we should configure
> >   HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> > 
> >  meson.build | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/meson.build b/meson.build
> > index 98e68ef0b1..ff20d3c249 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
> > have_system or have_vhost_user_gpu
> > prefix: '#include 
> > ',
> > dependencies: virgl))
> >endif
> > +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> > +   
> > cc.has_function('virgl_renderer_context_create_with_flags',
> > +   prefix: '#include 
> > ',
> > +   dependencies: virgl))
> The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores 
> the the given pkg and uses system includes. Antonio was aware about that 
> problem [1].
> 
> [1] 
> https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd
> 
> Given that virglrenderer 1.0 has been released couple weeks ago,
> can we make the v1.0 a mandatory requirement for qemu and remove
> all the ifdefs? I doubt that anyone is going to test newer qemu
> using older libviglrenderer, all that ifdef code will be bitrotting.

We cannot do that. If is is only weeks old, no distro will
have virglrenderer 1.0 present. QEMU has a defined set of
platforms that it targets compatibility with:

  https://www.qemu.org/docs/master/about/build-platforms.html

For newly added functionality we can set the min version to
something newer than the oldest QEMU target platform.

For existing functionality though, we must not regress wrt
the currently supported set of target platforms. So we can
only bump the min version to that present in the oldest
platform we target.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

2023-10-10 Thread Dmitry Osipenko 
On 9/15/23 14:11, Huang Rui wrote:
> Configure context init feature flag for virglrenderer.
> 
> Originally-by: Antonio Caggiano 
> Signed-off-by: Huang Rui 
> ---
> 
> V4 -> V5:
> - Inverted patch 5 and 6 because we should configure
>   HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> 
>  meson.build | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 98e68ef0b1..ff20d3c249 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
> have_system or have_vhost_user_gpu
> prefix: '#include ',
> dependencies: virgl))
>endif
> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> +   
> cc.has_function('virgl_renderer_context_create_with_flags',
> +   prefix: '#include ',
> +   dependencies: virgl))
The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores 
the the given pkg and uses system includes. Antonio was aware about that 
problem [1].

[1] 
https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd

Given that virglrenderer 1.0 has been released couple weeks ago, can we make 
the v1.0 a mandatory requirement for qemu and remove all the ifdefs? I doubt 
that anyone is going to test newer qemu using older libviglrenderer, all that 
ifdef code will be bitrotting.

@@ -1060,6 +1060,7 @@ virgl = not_found
 have_vhost_user_gpu = have_tools and targetos == 'linux' and pixman.found()
 if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
   virgl = dependency('virglrenderer',
+ version: '>=1.0.0',
  method: 'pkg-config',
  required: get_option('virglrenderer'))
   if virgl.found()


Best regards,
Dmitry

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

2023-09-20 Thread Huang Rui 
On Tue, Sep 19, 2023 at 04:17:43PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 15, 2023 at 6:16 PM Huang Rui  wrote:
> >
> > Configure context init feature flag for virglrenderer.
> >
> > Originally-by: Antonio Caggiano 
> > Signed-off-by: Huang Rui 
> > ---
> >
> > V4 -> V5:
> > - Inverted patch 5 and 6 because we should configure
> >   HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> >
> >  meson.build | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/meson.build b/meson.build
> > index 98e68ef0b1..ff20d3c249 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
> > have_system or have_vhost_user_gpu
> > prefix: '#include 
> > ',
> > dependencies: virgl))
> >endif
> > +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> > +   
> > cc.has_function('virgl_renderer_context_create_with_flags',
> > +   prefix: '#include 
> > ',
> > +   dependencies: virgl))
> 
> Move it under the "if virgl.found()" block above.
> 
> I suggest to name it after what is actually checked:
> HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS for ex
> 

OK, will update it in V6.

Thanks,
Ray

> >  endif
> >  blkio = not_found
> >  if not get_option('blkio').auto() or have_block
> > --
> > 2.34.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

2023-09-19 Thread Marc-André Lureau 
Hi

On Fri, Sep 15, 2023 at 6:16 PM Huang Rui  wrote:
>
> Configure context init feature flag for virglrenderer.
>
> Originally-by: Antonio Caggiano 
> Signed-off-by: Huang Rui 
> ---
>
> V4 -> V5:
> - Inverted patch 5 and 6 because we should configure
>   HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>
>  meson.build | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/meson.build b/meson.build
> index 98e68ef0b1..ff20d3c249 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
> have_system or have_vhost_user_gpu
> prefix: '#include ',
> dependencies: virgl))
>endif
> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> +   
> cc.has_function('virgl_renderer_context_create_with_flags',
> +   prefix: '#include ',
> +   dependencies: virgl))

Move it under the "if virgl.found()" block above.

I suggest to name it after what is actually checked:
HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS for ex

>  endif
>  blkio = not_found
>  if not get_option('blkio').auto() or have_block
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

[QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

2023-09-15 Thread Huang Rui 
Configure context init feature flag for virglrenderer.

Originally-by: Antonio Caggiano 
Signed-off-by: Huang Rui 
---

V4 -> V5:
- Inverted patch 5 and 6 because we should configure
  HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)

 meson.build | 4 
 1 file changed, 4 insertions(+)

diff --git a/meson.build b/meson.build
index 98e68ef0b1..ff20d3c249 100644
--- a/meson.build
+++ b/meson.build
@@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system 
or have_vhost_user_gpu
prefix: '#include ',
dependencies: virgl))
   endif
+  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
+   
cc.has_function('virgl_renderer_context_create_with_flags',
+   prefix: '#include ',
+   dependencies: virgl))
 endif
 blkio = not_found
 if not get_option('blkio').auto() or have_block
-- 
2.34.1