Re: [PATCH v11 09/10] virtio-gpu: Register capsets dynamically

[Akihiko Odaki]

On 2024/05/12 3:22, Dmitry Osipenko wrote:

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-pra...@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-pra...@amd.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipe...@collabora.com>
---
  hw/display/virtio-gpu-gl.c     |  6 ++++--
  hw/display/virtio-gpu-virgl.c  | 33 +++++++++++++++++++++------------
  include/hw/virtio/virtio-gpu.h |  4 +++-
  3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 95806999189e..431fc2881a00 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -124,8 +124,8 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, 
Error **errp)
      }

g->parent_obj.conf.flags |= (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);

-    VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =
-        virtio_gpu_virgl_get_num_capsets(g);
+    g->capset_ids = virtio_gpu_virgl_get_capsets(g);
+    VIRTIO_GPU_BASE(g)->virtio_config.num_capsets = g->capset_ids->len;

#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS

      g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED;
@@ -148,6 +148,8 @@ static void virtio_gpu_gl_device_unrealize(DeviceState 
*qdev)
      if (gl->renderer_inited) {
          virtio_gpu_virgl_deinit(g);
      }
+
+    g_array_unref(g->capset_ids);
  }

static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 3f2e406be3a4..135974431492 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -590,19 +590,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));
@@ -1123,14 +1117,29 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
      return 0;
  }

-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)

+static void virtio_gpu_virgl_add_capset(GArray *capset_ids, uint32_t capset_id)
+{
+    g_array_append_val(capset_ids, capset_id);
+}

Is it worthwhile to have a function for this?