[PATCH 0/1] virtio-gpu: CONTEXT_INIT feature

[PATCH 0/1] virtio-gpu: CONTEXT_INIT feature

[Antonio Caggiano]

This is a different attempt at upstreaming the work I have been doing to
enable support for the Venus Virtio-GPU Vulkan driver.

I believe the previous one [0] was a bit too much stuff in one place,
therefore with this I would like to try a more fine-grained approach.

I will just start by the CONTEXT_INIT feature as it was the first commit
of the series aforementioned and the virtio-spec has been updated
recently on that regard [1]. Hopefully this would also answer Gerd's
comment on the previous patch [2].

[0] https://www.mail-archive.com/qemu-devel@nongnu.org/msg826897.html
[1] https://github.com/oasis-tcs/virtio-spec/commit/aad2b6f3620ec0c9d16aaf046db8c282c24cce3e
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg827304.html

Antonio Caggiano (1):
  virtio-gpu: CONTEXT_INIT feature

 hw/display/virtio-gpu-base.c                |  2 ++
 hw/display/virtio-gpu-virgl.c               | 10 ++++++++--
 include/hw/virtio/virtio-gpu-bswap.h        |  2 +-
 include/standard-headers/linux/virtio_gpu.h |  9 +++++++--
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/1] virtio-gpu: CONTEXT_INIT feature

[Antonio Caggiano]

Create virgl renderer context with flags using context_id when valid.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
---
 hw/display/virtio-gpu-base.c                |  2 ++
 hw/display/virtio-gpu-virgl.c               | 10 ++++++++--
 include/hw/virtio/virtio-gpu-bswap.h        |  2 +-
 include/standard-headers/linux/virtio_gpu.h |  9 +++++++--
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index c8da4806e0..619185a9d2 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -212,6 +212,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
         features |= (1 << VIRTIO_GPU_F_RESOURCE_BLOB);
     }
 
+    features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
+
     return features;
 }
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 18d054922f..5a184cf445 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -97,8 +97,14 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
                                     cc.debug_name);
 
-    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
-                                  cc.debug_name);
+    if (cc.context_init) {
+        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
+                                                 cc.context_init,
+                                                 cc.nlen,
+                                                 cc.debug_name);
+    } else {
+        virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
+    }
 }
 
 static void virgl_cmd_context_destroy(VirtIOGPU *g,
diff --git a/include/hw/virtio/virtio-gpu-bswap.h b/include/hw/virtio/virtio-gpu-bswap.h
index e2bee8f595..6267cb57e5 100644
--- a/include/hw/virtio/virtio-gpu-bswap.h
+++ b/include/hw/virtio/virtio-gpu-bswap.h
@@ -24,7 +24,7 @@ virtio_gpu_ctrl_hdr_bswap(struct virtio_gpu_ctrl_hdr *hdr)
     le32_to_cpus(&hdr->flags);
     le64_to_cpus(&hdr->fence_id);
     le32_to_cpus(&hdr->ctx_id);
-    le32_to_cpus(&hdr->padding);
+    le32_to_cpus(&hdr->info);
 }
 
 static inline void
diff --git a/include/standard-headers/linux/virtio_gpu.h b/include/standard-headers/linux/virtio_gpu.h
index 1357e4774e..c9f9c24d6a 100644
--- a/include/standard-headers/linux/virtio_gpu.h
+++ b/include/standard-headers/linux/virtio_gpu.h
@@ -59,6 +59,11 @@
  * VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB
  */
 #define VIRTIO_GPU_F_RESOURCE_BLOB       3
+/*
+ * VIRTIO_GPU_CMD_CREATE_CONTEXT with
+ * context_init
+ */
+#define VIRTIO_GPU_F_CONTEXT_INIT        4
 
 enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_UNDEFINED = 0,
@@ -129,7 +134,7 @@ struct virtio_gpu_ctrl_hdr {
 	uint32_t flags;
 	uint64_t fence_id;
 	uint32_t ctx_id;
-	uint32_t padding;
+	uint32_t info;
 };
 
 /* data passed in the cursor vq */
@@ -272,7 +277,7 @@ struct virtio_gpu_resource_create_3d {
 struct virtio_gpu_ctx_create {
 	struct virtio_gpu_ctrl_hdr hdr;
 	uint32_t nlen;
-	uint32_t padding;
+	uint32_t context_init;
 	char debug_name[64];
 };
 
-- 
2.30.2

Re: [PATCH 1/1] virtio-gpu: CONTEXT_INIT feature

[Gerd Hoffmann]

> @@ -212,6 +212,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>          features |= (1 << VIRTIO_GPU_F_RESOURCE_BLOB);
>      }
>  
> +    features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);

This needs a config option, simliar to the other features.  It is a
guest-visible change so we must be able to turn it off for live
migration compatibility reasons.  We also need a compat property to
turn it off by default for 6.1 + older machine types.

> +    if (cc.context_init) {
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);

This requires a minimum virglrenderer version I guess?

> --- a/include/standard-headers/linux/virtio_gpu.h
> +++ b/include/standard-headers/linux/virtio_gpu.h

Separate patch please.
Also use scripts/update-linux-headers.sh for this.

take care,
  Gerd

Re: [PATCH 1/1] virtio-gpu: CONTEXT_INIT feature

[Antonio Caggiano]

On 28/09/21 07:13, Gerd Hoffmann wrote:
>> @@ -212,6 +212,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>>           features |= (1 << VIRTIO_GPU_F_RESOURCE_BLOB);
>>       }
>>   
>> +    features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
> 
> This needs a config option, simliar to the other features.  It is a
> guest-visible change so we must be able to turn it off for live
> migration compatibility reasons.  We also need a compat property to
> turn it off by default for 6.1 + older machine types.

Could you give me a hint on how to add this compat property?



>> +    if (cc.context_init) {
>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>> +                                                 cc.context_init,
>> +                                                 cc.nlen,
>> +                                                 cc.debug_name);
> 
> This requires a minimum virglrenderer version I guess?


Definitely, that is going to be >= 0.9.0



>> --- a/include/standard-headers/linux/virtio_gpu.h
>> +++ b/include/standard-headers/linux/virtio_gpu.h
> 
> Separate patch please.
> Also use scripts/update-linux-headers.sh for this.
Well, then I believe we will need to wait for this patch series:

https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg367531.html





Cheers,

Antonio

Re: [PATCH 1/1] virtio-gpu: CONTEXT_INIT feature

[Gerd Hoffmann]

  Hi,

> > This needs a config option, simliar to the other features.  It is a
> > guest-visible change so we must be able to turn it off for live
> > migration compatibility reasons.  We also need a compat property to
> > turn it off by default for 6.1 + older machine types.
> 
> Could you give me a hint on how to add this compat property?

No need to do that for now, see below.  But "git log" or "git blame"
should find the patches doing the same for the other features).

> > > +    if (cc.context_init) {
> > > +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> > > +                                                 cc.context_init,
> > > +                                                 cc.nlen,
> > > +                                                 cc.debug_name);
> > 
> > This requires a minimum virglrenderer version I guess?
> 
> Definitely, that is going to be >= 0.9.0

... because we can hardly enable that by default if it isn't even
released.  We'll need #ifdefs so qemu continues to build with older
virglrenderer versions for a while.  It also must stay disabled by
default so you don't get different qemu behavior depending on the
version compiled against.

Then, in 1-2 years, when distributions have picked up the new version,
we can consider to raise the minimal required version to 0.9.0 and flip
the default to enabled.

> > > --- a/include/standard-headers/linux/virtio_gpu.h
> > > +++ b/include/standard-headers/linux/virtio_gpu.h
> > 
> > Separate patch please.
> > Also use scripts/update-linux-headers.sh for this.
> Well, then I believe we will need to wait for this patch series:
> 
> https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg367531.html

Ah, right.  Too much stuff on my todo list :(

take care,
  Gerd