Re: [PATCH v6 09/11] virtio-gpu: Support Venus capset

[Pierre-Eric Pelloux-Prayer]

Hi Ray, Antonio,


Le 19/12/2023 à 08:53, Huang Rui a écrit :
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>
> Add support for the Venus capset, which enables Vulkan support through
> the Venus Vulkan driver for virtio-gpu.
>
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> No change in v6.
>
>   hw/display/virtio-gpu-virgl.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index be9da6e780..f35a751824 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -506,6 +506,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
>           virgl_renderer_get_cap_set(resp.capset_id,
>                                      &resp.capset_max_version,
>                                      &resp.capset_max_size);
> +    } else if (info.capset_index == 2) {
> +        resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
> +        virgl_renderer_get_cap_set(resp.capset_id,
> +                                   &resp.capset_max_version,
> +                                   &resp.capset_max_size);
>       } else {
>           resp.capset_max_version = 0;
>           resp.capset_max_size = 0;
> @@ -978,10 +983,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>   
>   int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>   {
> -    uint32_t capset2_max_ver, capset2_max_size;
> +    uint32_t capset2_max_ver, capset2_max_size, num_capsets;
> +    num_capsets = 1;
> +
>       virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
> -                              &capset2_max_ver,
> -                              &capset2_max_size);
> +                               &capset2_max_ver,
> +                               &capset2_max_size);
> +    num_capsets += capset2_max_ver ? 1 : 0;
> +
> +    virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
> +                               &capset2_max_ver,
> +                               &capset2_max_size);
> +    num_capsets += capset2_max_size ? 1 : 0;

IMHO the logic here doesn't work.

The kernel will use num_capset like this:

        for (i = 0; i < num_capsets; i++) {
                virtio_gpu_cmd_get_capset_info(vgdev, i);

So if num_capset = 2, it will query the capset info of index 0 and 1.
Capset 0 is alway VIRGL so it's fine. But since VIRL2 support is optional,
QEMU has no way to know if index 1 is VIRGL2 (if it's supported)
or VENUS (if VIRGL2 support is missing).
And it'll get worse when we will want to support CAPSET_DRM.

Ray: we have a patch internally for this (virtio-gpu: fix capset query), you
may want to add it to this series, before this patch.

Regards,
Pierre-Eric



>   
> -    return capset2_max_ver ? 2 : 1;
> +    return num_capsets;
>   }