Re: [PATCH v3 5/6] virtio-gpu: Support asynchronous fencing

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 10/31/24 10:44, Akihiko Odaki wrote:
> On 2024/10/25 8:33, Dmitry Osipenko wrote:
>> Support asynchronous fencing feature of virglrenderer. It allows Qemu to
>> handle fence as soon as it's signalled instead of periodically polling
>> the fence status. This feature is required for enabling DRM context
>> support in Qemu because legacy fencing mode isn't supported for DRM
>> contexts in virglrenderer.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>   hw/display/virtio-gpu-gl.c     |   3 +
>>   hw/display/virtio-gpu-virgl.c  | 138 +++++++++++++++++++++++++++------
>>   include/hw/virtio/virtio-gpu.h |  13 ++++
>>   3 files changed, 132 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index 7c0e448b4661..53d938f23f20 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -170,6 +170,9 @@ static void
>> virtio_gpu_gl_device_unrealize(DeviceState *qdev)
>>       if (gl->renderer_state >= RS_INITED) {
>>   #if VIRGL_VERSION_MAJOR >= 1
>>           qemu_bh_delete(gl->cmdq_resume_bh);
>> +
>> +        virtio_gpu_virgl_reset_async_fences(g);
>> +        qemu_bh_delete(gl->async_fence_bh);
>>   #endif
>>           if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
>>               timer_free(gl->print_stats);
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-
>> virgl.c
>> index 3c564683820b..37b40e258398 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -891,6 +891,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>                                         struct virtio_gpu_ctrl_command
>> *cmd)
>>   {
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>       bool cmd_suspended = false;
>>       int ret;
>>   @@ -992,34 +993,71 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>         trace_virtio_gpu_fence_ctrl(cmd->cmd_hdr.fence_id, cmd-
>> >cmd_hdr.type);
>>   -    /*
>> -     * Unlike other virglrenderer functions, this one returns a positive
>> -     * error code.
>> -     */
>> -    ret = virgl_renderer_create_fence(cmd->cmd_hdr.fence_id, 0);
>> -    if (ret)
>> -        qemu_log_mask(LOG_GUEST_ERROR,
>> -                      "%s: virgl_renderer_create_fence error: %s",
>> -                      __func__, strerror(ret));
>> +    if (gl->context_fence_enabled &&
>> +        (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX)) {
>> +        uint32_t flags = 0;
>> +
>> +        ret = virgl_renderer_context_create_fence(cmd-
>> >cmd_hdr.ctx_id, flags,
>> +                                                  cmd->cmd_hdr.ring_idx,
>> +                                                  cmd-
>> >cmd_hdr.fence_id);
>> +        if (ret)
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "%s: virgl_renderer_context_create_fence
>> error: %s",
>> +                          __func__, strerror(-ret));
>> +    } else {
>> +        /*
>> +         * Unlike other virglrenderer functions, this one returns a
>> positive
>> +         * error code.
>> +         */
>> +        ret = virgl_renderer_create_fence(cmd->cmd_hdr.fence_id, 0);
>> +        if (ret)
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "%s: virgl_renderer_create_fence error: %s",
>> +                          __func__, strerror(ret));
>> +    }
>>   }
>>   -static void virgl_write_fence(void *opaque, uint32_t fence)
>> +static void virtio_gpu_virgl_async_fence_bh(void *opaque)
>>   {
>> -    VirtIOGPU *g = opaque;
>> +    QSLIST_HEAD(, virtio_gpu_virgl_context_fence) async_fenceq;
>>       struct virtio_gpu_ctrl_command *cmd, *tmp;
>> +    struct virtio_gpu_virgl_context_fence *f;
>> +    VirtIOGPU *g = opaque;
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>   -    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
>> -        /*
>> -         * the guest can end up emitting fences out of order
>> -         * so we should check all fenced cmds not just the first one.
>> -         */
>> -        if (cmd->cmd_hdr.fence_id > fence) {
>> -            continue;
>> +    QSLIST_MOVE_ATOMIC(&async_fenceq, &gl->async_fenceq);
>> +
>> +    while (!QSLIST_EMPTY(&async_fenceq)) {
>> +        f = QSLIST_FIRST(&async_fenceq);
>> +
>> +        QSLIST_REMOVE_HEAD(&async_fenceq, next);
>> +
>> +        QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
>> +            /*
>> +             * the guest can end up emitting fences out of order
>> +             * so we should check all fenced cmds not just the first
>> one.
>> +             */
>> +            if (cmd->cmd_hdr.fence_id > f->fence_id) {
>> +                continue;
>> +            }
>> +            if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
>> +                if (cmd->cmd_hdr.ring_idx != f->ring_idx) {
>> +                    continue;
>> +                }
>> +                if (cmd->cmd_hdr.ctx_id != f->ctx_id) {
>> +                    continue;
>> +                }
>> +            } else if (f->ring_idx >= 0) {
>> +                /* ctx0 GL-query fences don't have ring info */
>> +                continue;
>> +            }
>> +            virtio_gpu_ctrl_response_nodata(g, cmd,
>> VIRTIO_GPU_RESP_OK_NODATA);
>> +            QTAILQ_REMOVE(&g->fenceq, cmd, next);
>> +            g_free(cmd);
>>           }
>> -        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);
>> +
>> +        trace_virtio_gpu_fence_resp(f->fence_id);
>> +        g_free(f);
>>           g->inflight--;
>>           if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
>>               trace_virtio_gpu_dec_inflight_fences(g->inflight);
>> @@ -1027,6 +1065,50 @@ static void virgl_write_fence(void *opaque,
>> uint32_t fence)
>>       }
>>   }
>>   +void virtio_gpu_virgl_reset_async_fences(VirtIOGPU *g)
>> +{
>> +    struct virtio_gpu_virgl_context_fence *f, *f_tmp;
>> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>> +
>> +    QSLIST_FOREACH_SAFE(f, &gl->async_fenceq, next, f_tmp) {
>> +        QSLIST_REMOVE(&gl->async_fenceq, f,
>> virtio_gpu_virgl_context_fence,
>> +                      next);
> 
> Replace QSLIST_FOREACH_SAFE() and QSLIST_REMOVE() with QSLIST_EMPTY(),
> QSLIST_FIRST() and QSLIST_REMOVE_HEAD() as you have done for
> virtio_gpu_virgl_async_fence_bh().

Ack

-- 
Best regards,
Dmitry