| Message ID | 20240425154539.2680550-10-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
On 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
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.
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.
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().
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 {