From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
virtio_gpu_virgl_get_num_capsets will return "num_capsets", but we can't
assume that capset_index 1 is always VIRGL2 once we'll support more capsets,
like Venus and DRM capsets. Register capsets dynamically to avoid that problem.
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
hw/display/virtio-gpu-virgl.c | 34 +++++++++++++++++++++++-----------
include/hw/virtio/virtio-gpu.h | 2 ++
2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index de788df155bf..9aa1fd78f1e1 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -597,19 +597,13 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
VIRTIO_GPU_FILL_CMD(info);
memset(&resp, 0, sizeof(resp));
- if (info.capset_index == 0) {
- resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
- virgl_renderer_get_cap_set(resp.capset_id,
- &resp.capset_max_version,
- &resp.capset_max_size);
- } else if (info.capset_index == 1) {
- resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL2;
+
+ if (info.capset_index < g->capset_ids->len) {
+ resp.capset_id = g_array_index(g->capset_ids, uint32_t,
+ info.capset_index);
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;
}
resp.hdr.type = VIRTIO_GPU_RESP_OK_CAPSET_INFO;
virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
@@ -1159,12 +1153,30 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
return 0;
}
+static void virtio_gpu_virgl_add_capset(VirtIOGPU *g, uint32_t capset_id)
+{
+ g_array_append_val(g->capset_ids, capset_id);
+}
+
int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
{
uint32_t capset2_max_ver, capset2_max_size;
+
+ if (g->capset_ids) {
+ return g->capset_ids->len;
+ }
+
+ g->capset_ids = g_array_new(false, false, sizeof(uint32_t));
+
+ /* VIRGL is always supported. */
+ virtio_gpu_virgl_add_capset(g, VIRTIO_GPU_CAPSET_VIRGL);
+
virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
&capset2_max_ver,
&capset2_max_size);
+ if (capset2_max_ver) {
+ virtio_gpu_virgl_add_capset(g, VIRTIO_GPU_CAPSET_VIRGL2);
+ }
- return capset2_max_ver ? 2 : 1;
+ return g->capset_ids->len;
}
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index dc24360656ce..32f38d86c908 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -211,6 +211,8 @@ struct VirtIOGPU {
QTAILQ_HEAD(, VGPUDMABuf) bufs;
VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
} dmabuf;
+
+ GArray *capset_ids;
};
struct VirtIOGPUClass {
--
2.44.0On 2024/04/26 0:45, Dmitry Osipenko wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>
> virtio_gpu_virgl_get_num_capsets will return "num_capsets", but we can't
> assume that capset_index 1 is always VIRGL2 once we'll support more capsets,
> like Venus and DRM capsets. Register capsets dynamically to avoid that problem.
>
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
> hw/display/virtio-gpu-virgl.c | 34 +++++++++++++++++++++++-----------
> include/hw/virtio/virtio-gpu.h | 2 ++
> 2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index de788df155bf..9aa1fd78f1e1 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -597,19 +597,13 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
> VIRTIO_GPU_FILL_CMD(info);
>
> memset(&resp, 0, sizeof(resp));
> - if (info.capset_index == 0) {
> - resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
> - virgl_renderer_get_cap_set(resp.capset_id,
> - &resp.capset_max_version,
> - &resp.capset_max_size);
> - } else if (info.capset_index == 1) {
> - resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL2;
> +
> + if (info.capset_index < g->capset_ids->len) {
> + resp.capset_id = g_array_index(g->capset_ids, uint32_t,
> + info.capset_index);
> 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;
> }
> resp.hdr.type = VIRTIO_GPU_RESP_OK_CAPSET_INFO;
> virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> @@ -1159,12 +1153,30 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> return 0;
> }
>
> +static void virtio_gpu_virgl_add_capset(VirtIOGPU *g, uint32_t capset_id)
> +{
> + g_array_append_val(g->capset_ids, capset_id);
> +}
> +
> int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
> {
> uint32_t capset2_max_ver, capset2_max_size;
> +
> + if (g->capset_ids) {
Move capset_ids initialization to virtio_gpu_virgl_init() to save this
conditional. capset_ids also needs to be freed when the device gets
unrealized.
On 4/27/24 10:12, Akihiko Odaki wrote:
>> int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>> {
>> uint32_t capset2_max_ver, capset2_max_size;
>> +
>> + if (g->capset_ids) {
>
> Move capset_ids initialization to virtio_gpu_virgl_init() to save this
> conditional.
Capsets are used before virgl is inited. At first guest queries virtio
device features and then enables virgl only if capset is available.
While virgl itself is initialized when first virtio command is
processed. I.e. it's not possible to move to virtio_gpu_virgl_init.
> capset_ids also needs to be freed when the device gets
> unrealized.
ACK
--
Best regards,
Dmitry
On 5/1/24 22:31, Dmitry Osipenko wrote:
> On 4/27/24 10:12, Akihiko Odaki wrote:
>>> int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>>> {
>>> uint32_t capset2_max_ver, capset2_max_size;
>>> +
>>> + if (g->capset_ids) {
>>
>> Move capset_ids initialization to virtio_gpu_virgl_init() to save this
>> conditional.
>
> Capsets are used before virgl is inited. At first guest queries virtio
> device features and then enables virgl only if capset is available.
> While virgl itself is initialized when first virtio command is
> processed. I.e. it's not possible to move to virtio_gpu_virgl_init.
Though no, capsets aren't part of device features. I'll move it to
virtio_gpu_virgl_init, thanks.
--
Best regards,
Dmitry
On 5/1/24 22:38, Dmitry Osipenko wrote:
> On 5/1/24 22:31, Dmitry Osipenko wrote:
>> On 4/27/24 10:12, Akihiko Odaki wrote:
>>>> int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>>>> {
>>>> uint32_t capset2_max_ver, capset2_max_size;
>>>> +
>>>> + if (g->capset_ids) {
>>>
>>> Move capset_ids initialization to virtio_gpu_virgl_init() to save this
>>> conditional.
>>
>> Capsets are used before virgl is inited. At first guest queries virtio
>> device features and then enables virgl only if capset is available.
>> While virgl itself is initialized when first virtio command is
>> processed. I.e. it's not possible to move to virtio_gpu_virgl_init.
>
> Though no, capsets aren't part of device features. I'll move it to
> virtio_gpu_virgl_init, thanks.
>
Number of capsets actually is a part of generic virtio device cfg
descriptor. Capsets initialization can't be moved without probing
capsets twice, i.e. not worthwhile.
--
Best regards,
Dmitry
On 2024/05/02 4:52, Dmitry Osipenko wrote:
> On 5/1/24 22:38, Dmitry Osipenko wrote:
>> On 5/1/24 22:31, Dmitry Osipenko wrote:
>>> On 4/27/24 10:12, Akihiko Odaki wrote:
>>>>> int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>>>>> {
>>>>> uint32_t capset2_max_ver, capset2_max_size;
>>>>> +
>>>>> + if (g->capset_ids) {
>>>>
>>>> Move capset_ids initialization to virtio_gpu_virgl_init() to save this
>>>> conditional.
>>>
>>> Capsets are used before virgl is inited. At first guest queries virtio
>>> device features and then enables virgl only if capset is available.
>>> While virgl itself is initialized when first virtio command is
>>> processed. I.e. it's not possible to move to virtio_gpu_virgl_init.
>>
>> Though no, capsets aren't part of device features. I'll move it to
>> virtio_gpu_virgl_init, thanks.
>>
>
> Number of capsets actually is a part of generic virtio device cfg
> descriptor. Capsets initialization can't be moved without probing
> capsets twice, i.e. not worthwhile.
>
I see. Then I suggest replacing virtio_gpu_virgl_get_num_capsets() with
a function that returns GArray of capset IDs.
virtio_gpu_gl_device_realize() will assign the returned GArray to
g->capset_ids. virtio_gpu_gl_device_unrealize(), which doesn't exist
yet, will free g->capset_ids later.
This way, you won't need the conditional, and it will be clear that a
GArray allocation happens in virtio_gpu_gl_device_realize() and is
matched with the deallocation in virtio_gpu_gl_device_unrealize().
© 2016 - 2026 Red Hat, Inc.