[PATCH v2] virtio-gpu: support context init multiple timeline

[PATCH v2] virtio-gpu: support context init multiple timeline

[Yiwei Zhang]

Venus and later native contexts have their own fence context along with
multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
the flags must be dispatched to be created on the target context. Fence
signaling also has to be handled on the specific timeline within that
target context.

Before this change, venus fencing is completely broken if the host
driver doesn't support implicit fencing with external memory objects.
Frames can go backwards along with random artifacts on screen if the
host driver doesn't attach an implicit fence to the render target. The
symptom could be hidden by certain guest wsi backend that waits on a
venus native VkFence object for the actual payload with limited present
modes or under special configs. e.g. x11 mailbox or xwayland.

After this change, everything related to venus fencing starts making
sense. Confirmed this via guest and host side perfetto tracing.

Changes since v1:
- Minor commit msg updates based on feedbacks from BALATON

Cc: qemu-stable@nongnu.org
Fixes: 94d0ea1c1928 ("virtio-gpu: Support Venus context")
Signed-off-by: Yiwei Zhang <zzyiwei@gmail.com>
---
 hw/display/virtio-gpu-virgl.c | 44 +++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 145a0b3879..94ddc01f91 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -970,6 +970,15 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
     }
 
     trace_virtio_gpu_fence_ctrl(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
+#if VIRGL_VERSION_MAJOR >= 1
+    if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
+        virgl_renderer_context_create_fence(cmd->cmd_hdr.ctx_id,
+                                            VIRGL_RENDERER_FENCE_FLAG_MERGEABLE,
+                                            cmd->cmd_hdr.ring_idx,
+                                            cmd->cmd_hdr.fence_id);
+        return;
+    }
+#endif
     virgl_renderer_create_fence(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
 }
 
@@ -983,6 +992,11 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
          * the guest can end up emitting fences out of order
          * so we should check all fenced cmds not just the first one.
          */
+#if VIRGL_VERSION_MAJOR >= 1
+        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
+            continue;
+        }
+#endif
         if (cmd->cmd_hdr.fence_id > fence) {
             continue;
         }
@@ -997,6 +1011,29 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
     }
 }
 
+#if VIRGL_VERSION_MAJOR >= 1
+static void virgl_write_context_fence(void *opaque, uint32_t ctx_id,
+                                      uint32_t ring_idx, uint64_t fence_id) {
+    VirtIOGPU *g = opaque;
+    struct virtio_gpu_ctrl_command *cmd, *tmp;
+
+    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
+        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX &&
+            cmd->cmd_hdr.ctx_id == ctx_id && cmd->cmd_hdr.ring_idx == ring_idx &&
+            cmd->cmd_hdr.fence_id <= fence_id) {
+            trace_virtio_gpu_fence_resp(cmd->cmd_hdr.fence_id);
+            virtio_gpu_ctrl_response_nodata(g, cmd, VIRTIO_GPU_RESP_OK_NODATA);
+            QTAILQ_REMOVE(&g->fenceq, cmd, next);
+            g_free(cmd);
+            g->inflight--;
+            if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
+                trace_virtio_gpu_dec_inflight_fences(g->inflight);
+            }
+        }
+    }
+}
+#endif
+
 static virgl_renderer_gl_context
 virgl_create_context(void *opaque, int scanout_idx,
                      struct virgl_renderer_gl_ctx_param *params)
@@ -1031,11 +1068,18 @@ static int virgl_make_context_current(void *opaque, int scanout_idx,
 }
 
 static struct virgl_renderer_callbacks virtio_gpu_3d_cbs = {
+#if VIRGL_VERSION_MAJOR >= 1
+    .version             = 3,
+#else
     .version             = 1,
+#endif
     .write_fence         = virgl_write_fence,
     .create_gl_context   = virgl_create_context,
     .destroy_gl_context  = virgl_destroy_context,
     .make_current        = virgl_make_context_current,
+#if VIRGL_VERSION_MAJOR >= 1
+    .write_context_fence = virgl_write_context_fence,
+#endif
 };
 
 static void virtio_gpu_print_stats(void *opaque)
-- 
2.39.5

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Dmitry Osipenko]

On 5/18/25 18:26, Yiwei Zhang wrote:
> Venus and later native contexts have their own fence context along with
> multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
> the flags must be dispatched to be created on the target context. Fence
> signaling also has to be handled on the specific timeline within that
> target context.
> 
> Before this change, venus fencing is completely broken if the host
> driver doesn't support implicit fencing with external memory objects.
> Frames can go backwards along with random artifacts on screen if the
> host driver doesn't attach an implicit fence to the render target. The
> symptom could be hidden by certain guest wsi backend that waits on a
> venus native VkFence object for the actual payload with limited present
> modes or under special configs. e.g. x11 mailbox or xwayland.
> 
> After this change, everything related to venus fencing starts making
> sense. Confirmed this via guest and host side perfetto tracing.

Thanks for the patch, looks good. Haven't realized that ring idx support
should've been a part of the context_init addition. Interestingly, it
never showed a problem during venus testing in the past.

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

There could be multiple sources of a problem RE frames going backwards.
I'm always testing venus in conjunction with async-cb patch that adds
per-context fencing support and it doesn't help the X11 WSI issue
discussed on [1]. Hence the Mesa WSI fixes still needed.

[1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/34283

-- 
Best regards,
Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Dmitry Osipenko]

On 5/18/25 18:26, Yiwei Zhang wrote:
> Venus and later native contexts have their own fence context along with
> multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
> the flags must be dispatched to be created on the target context. Fence
> signaling also has to be handled on the specific timeline within that
> target context.
> 
> Before this change, venus fencing is completely broken if the host
> driver doesn't support implicit fencing with external memory objects.
> Frames can go backwards along with random artifacts on screen if the
> host driver doesn't attach an implicit fence to the render target. The
> symptom could be hidden by certain guest wsi backend that waits on a
> venus native VkFence object for the actual payload with limited present
> modes or under special configs. e.g. x11 mailbox or xwayland.
> 
> After this change, everything related to venus fencing starts making
> sense. Confirmed this via guest and host side perfetto tracing.
> 
> Changes since v1:
> - Minor commit msg updates based on feedbacks from BALATON
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 94d0ea1c1928 ("virtio-gpu: Support Venus context")
> Signed-off-by: Yiwei Zhang <zzyiwei@gmail.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 44 +++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 145a0b3879..94ddc01f91 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -970,6 +970,15 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>      }
>  
>      trace_virtio_gpu_fence_ctrl(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
> +#if VIRGL_VERSION_MAJOR >= 1
> +    if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
> +        virgl_renderer_context_create_fence(cmd->cmd_hdr.ctx_id,
> +                                            VIRGL_RENDERER_FENCE_FLAG_MERGEABLE,
> +                                            cmd->cmd_hdr.ring_idx,
> +                                            cmd->cmd_hdr.fence_id);
> +        return;
> +    }
> +#endif
>      virgl_renderer_create_fence(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
>  }
>  
> @@ -983,6 +992,11 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
>           * the guest can end up emitting fences out of order
>           * so we should check all fenced cmds not just the first one.
>           */
> +#if VIRGL_VERSION_MAJOR >= 1
> +        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
> +            continue;
> +        }
> +#endif

Is it possible that virglrenderer will ever write a context fence in
virgl_renderer_create_fence()? Do we really need this check?

-- 
Best regards,
Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Yiwei Zhang]

On Mon, May 19, 2025 at 6:12 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 5/18/25 18:26, Yiwei Zhang wrote:
> > Venus and later native contexts have their own fence context along with
> > multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
> > the flags must be dispatched to be created on the target context. Fence
> > signaling also has to be handled on the specific timeline within that
> > target context.
> >
> > Before this change, venus fencing is completely broken if the host
> > driver doesn't support implicit fencing with external memory objects.
> > Frames can go backwards along with random artifacts on screen if the
> > host driver doesn't attach an implicit fence to the render target. The
> > symptom could be hidden by certain guest wsi backend that waits on a
> > venus native VkFence object for the actual payload with limited present
> > modes or under special configs. e.g. x11 mailbox or xwayland.
> >
> > After this change, everything related to venus fencing starts making
> > sense. Confirmed this via guest and host side perfetto tracing.
> >
> > Changes since v1:
> > - Minor commit msg updates based on feedbacks from BALATON
> >
> > Cc: qemu-stable@nongnu.org
> > Fixes: 94d0ea1c1928 ("virtio-gpu: Support Venus context")
> > Signed-off-by: Yiwei Zhang <zzyiwei@gmail.com>
> > ---
> >  hw/display/virtio-gpu-virgl.c | 44 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 145a0b3879..94ddc01f91 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -970,6 +970,15 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >      }
> >
> >      trace_virtio_gpu_fence_ctrl(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
> > +#if VIRGL_VERSION_MAJOR >= 1
> > +    if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
> > +        virgl_renderer_context_create_fence(cmd->cmd_hdr.ctx_id,
> > +                                            VIRGL_RENDERER_FENCE_FLAG_MERGEABLE,
> > +                                            cmd->cmd_hdr.ring_idx,
> > +                                            cmd->cmd_hdr.fence_id);
> > +        return;
> > +    }
> > +#endif
> >      virgl_renderer_create_fence(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
> >  }
> >
> > @@ -983,6 +992,11 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
> >           * the guest can end up emitting fences out of order
> >           * so we should check all fenced cmds not just the first one.
> >           */
> > +#if VIRGL_VERSION_MAJOR >= 1
> > +        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
> > +            continue;
> > +        }
> > +#endif
>
> Is it possible that virglrenderer will ever write a context fence in
> virgl_renderer_create_fence()? Do we really need this check?

I assume you were referring to whether a context fence can be written
in virgl_write_fence(). Yes, the fenceq contains both the global
fences and context specific fences. Without the check,
virgl_write_fence() will signal any context fence with a fence id
smaller than the latest signaled virgl global fence.

>
> --
> Best regards,
> Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Dmitry Osipenko]

On 5/20/25 01:20, Yiwei Zhang wrote:
>> Is it possible that virglrenderer will ever write a context fence in
>> virgl_renderer_create_fence()? Do we really need this check?
> I assume you were referring to whether a context fence can be written
> in virgl_write_fence(). Yes, the fenceq contains both the global
> fences and context specific fences. Without the check,
> virgl_write_fence() will signal any context fence with a fence id
> smaller than the latest signaled virgl global fence.

Indeed, I was comparing with the async-cb variant and forgot that async
one uses separate fence list.

-- 
Best regards,
Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Dmitry Osipenko]

On 5/18/25 18:26, Yiwei Zhang wrote:
> +#if VIRGL_VERSION_MAJOR >= 1
> +static void virgl_write_context_fence(void *opaque, uint32_t ctx_id,
> +                                      uint32_t ring_idx, uint64_t fence_id) {
> +    VirtIOGPU *g = opaque;
> +    struct virtio_gpu_ctrl_command *cmd, *tmp;
> +
> +    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
> +        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX &&

What if guest kernel version is very old and doesn't support ring_idx?
Wouldn't this write_context_fence() cb be used by vrend for signalling
fences without ring_idx info?

-- 
Best regards,
Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Yiwei Zhang]

On Mon, May 19, 2025 at 7:29 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 5/18/25 18:26, Yiwei Zhang wrote:
> > +#if VIRGL_VERSION_MAJOR >= 1
> > +static void virgl_write_context_fence(void *opaque, uint32_t ctx_id,
> > +                                      uint32_t ring_idx, uint64_t fence_id) {
> > +    VirtIOGPU *g = opaque;
> > +    struct virtio_gpu_ctrl_command *cmd, *tmp;
> > +
> > +    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
> > +        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX &&
>
> What if guest kernel version is very old and doesn't support ring_idx?
> Wouldn't this write_context_fence() cb be used by vrend for signalling
> fences without ring_idx info?

Old kernels without CONTEXT_INIT don't have the uapi to create context
fences. So only ctx0 fences can be created, which are retired only
with the ctx0 specific write_fence() callback. The newer
write_context_fence() callback is dedicated to retire context fences.

>
> --
> Best regards,
> Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Alex Bennée]

Yiwei Zhang <zzyiwei@gmail.com> writes:

> Venus and later native contexts have their own fence context along with
> multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
> the flags must be dispatched to be created on the target context. Fence
> signaling also has to be handled on the specific timeline within that
> target context.
>
> Before this change, venus fencing is completely broken if the host
> driver doesn't support implicit fencing with external memory objects.
> Frames can go backwards along with random artifacts on screen if the
> host driver doesn't attach an implicit fence to the render target. The
> symptom could be hidden by certain guest wsi backend that waits on a
> venus native VkFence object for the actual payload with limited present
> modes or under special configs. e.g. x11 mailbox or xwayland.
>
> After this change, everything related to venus fencing starts making
> sense. Confirmed this via guest and host side perfetto tracing.
>
> Changes since v1:
> - Minor commit msg updates based on feedbacks from BALATON
>
> Cc: qemu-stable@nongnu.org
> Fixes: 94d0ea1c1928 ("virtio-gpu: Support Venus context")
> Signed-off-by: Yiwei Zhang <zzyiwei@gmail.com>

Queued to virtio-gpu/next (in maintainer/may-2025), thanks.
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Dmitry Osipenko]

On 5/20/25 18:44, Alex Bennée wrote:
> Yiwei Zhang <zzyiwei@gmail.com> writes:
> 
>> Venus and later native contexts have their own fence context along with
>> multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
>> the flags must be dispatched to be created on the target context. Fence
>> signaling also has to be handled on the specific timeline within that
>> target context.
>>
>> Before this change, venus fencing is completely broken if the host
>> driver doesn't support implicit fencing with external memory objects.
>> Frames can go backwards along with random artifacts on screen if the
>> host driver doesn't attach an implicit fence to the render target. The
>> symptom could be hidden by certain guest wsi backend that waits on a
>> venus native VkFence object for the actual payload with limited present
>> modes or under special configs. e.g. x11 mailbox or xwayland.
>>
>> After this change, everything related to venus fencing starts making
>> sense. Confirmed this via guest and host side perfetto tracing.
>>
>> Changes since v1:
>> - Minor commit msg updates based on feedbacks from BALATON
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: 94d0ea1c1928 ("virtio-gpu: Support Venus context")
>> Signed-off-by: Yiwei Zhang <zzyiwei@gmail.com>
> 
> Queued to virtio-gpu/next (in maintainer/may-2025), thanks.
> <snip>
> 

Wanted to notify you about the patch after giving it a test, but you
beat me to it. Thanks for taking the care. Tested now, Steam games work.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Dmitry Osipenko]

On 5/20/25 11:26, Yiwei Zhang wrote:
> On Mon, May 19, 2025 at 7:29 PM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> On 5/18/25 18:26, Yiwei Zhang wrote:
>>> +#if VIRGL_VERSION_MAJOR >= 1
>>> +static void virgl_write_context_fence(void *opaque, uint32_t ctx_id,
>>> +                                      uint32_t ring_idx, uint64_t fence_id) {
>>> +    VirtIOGPU *g = opaque;
>>> +    struct virtio_gpu_ctrl_command *cmd, *tmp;
>>> +
>>> +    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
>>> +        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX &&
>>
>> What if guest kernel version is very old and doesn't support ring_idx?
>> Wouldn't this write_context_fence() cb be used by vrend for signalling
>> fences without ring_idx info?
> 
> Old kernels without CONTEXT_INIT don't have the uapi to create context
> fences. So only ctx0 fences can be created, which are retired only
> with the ctx0 specific write_fence() callback. The newer
> write_context_fence() callback is dedicated to retire context fences.

All should be good then, thanks.

-- 
Best regards,
Dmitry

Re: [PATCH v2] virtio-gpu: support context init multiple timeline

[Yiwei Zhang]

On Tue, May 20, 2025 at 9:31 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 5/20/25 11:26, Yiwei Zhang wrote:
> > On Mon, May 19, 2025 at 7:29 PM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 5/18/25 18:26, Yiwei Zhang wrote:
> >>> +#if VIRGL_VERSION_MAJOR >= 1
> >>> +static void virgl_write_context_fence(void *opaque, uint32_t ctx_id,
> >>> +                                      uint32_t ring_idx, uint64_t fence_id) {
> >>> +    VirtIOGPU *g = opaque;
> >>> +    struct virtio_gpu_ctrl_command *cmd, *tmp;
> >>> +
> >>> +    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
> >>> +        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX &&
> >>
> >> What if guest kernel version is very old and doesn't support ring_idx?
> >> Wouldn't this write_context_fence() cb be used by vrend for signalling
> >> fences without ring_idx info?
> >
> > Old kernels without CONTEXT_INIT don't have the uapi to create context
> > fences. So only ctx0 fences can be created, which are retired only
> > with the ctx0 specific write_fence() callback. The newer
> > write_context_fence() callback is dedicated to retire context fences.
>
> All should be good then, thanks.
>
> --
> Best regards,
> Dmitry

Thank you all for the prompt reviews!

Best,
Yiwei