Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

Hi Marc-André Lureau

On 2023/6/12 20:42, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jun 8, 2023 at 6:26 AM Jiqian Chen <Jiqian.Chen@amd.com 
> <mailto:Jiqian.Chen@amd.com>> wrote:
> 
>     After suspending and resuming guest VM, you will get
>     a black screen, and the display can't come back.
> 
>     This is because when guest did suspending, it called
>     into qemu to call virtio_gpu_gl_reset. In function
>     virtio_gpu_gl_reset, it destroyed resources and reset
>     renderer, which were used for display. As a result,
>     guest's screen can't come back to the time when it was
>     suspended and only showed black.
> 
>     So, this patch adds a new ctrl message
>     VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
>     guest. If guest is during suspending, it sets freezing
>     status of virtgpu to true, this will prevent destroying
>     resources and resetting renderer when guest calls into
>     virtio_gpu_gl_reset. If guest is during resuming, it sets
>     freezing to false, and then virtio_gpu_gl_reset will keep
>     its origin actions and has no other impaction.
> 
>     Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com 
> <mailto:Jiqian.Chen@amd.com>>
>     ---
>      hw/display/virtio-gpu-gl.c                  |  9 ++++++-
>      hw/display/virtio-gpu-virgl.c               |  3 +++
>      hw/display/virtio-gpu.c                     | 26 +++++++++++++++++++--
>      include/hw/virtio/virtio-gpu.h              |  3 +++
>      include/standard-headers/linux/virtio_gpu.h |  9 +++++++
>      5 files changed, 47 insertions(+), 3 deletions(-)
> 
>     diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>     index e06be60dfb..e11ad233eb 100644
>     --- a/hw/display/virtio-gpu-gl.c
>     +++ b/hw/display/virtio-gpu-gl.c
>     @@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>           */
>          if (gl->renderer_inited && !gl->renderer_reset) {
>              virtio_gpu_virgl_reset_scanout(g);
>     -        gl->renderer_reset = true;
>     +        /*
>     +         * If guest is suspending, we shouldn't reset renderer,
>     +         * otherwise, the display can't come back to the time when
>     +         * it was suspended after guest resumed.
>     +         */
>     +        if (!g->freezing) {
>     +            gl->renderer_reset = true;
>     +        }
>          }
>      }
> 
>     diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>     index 73cb92c8d5..183ec92d53 100644
>     --- a/hw/display/virtio-gpu-virgl.c
>     +++ b/hw/display/virtio-gpu-virgl.c
>     @@ -464,6 +464,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>          case VIRTIO_GPU_CMD_GET_EDID:
>              virtio_gpu_get_edid(g, cmd);
>              break;
>     +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>     +        virtio_gpu_cmd_status_freezing(g, cmd);
>     +        break;
>          default:
>              cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>              break;
>     diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>     index 5e15c79b94..8f235d7848 100644
>     --- a/hw/display/virtio-gpu.c
>     +++ b/hw/display/virtio-gpu.c
>     @@ -373,6 +373,16 @@ static void 
> virtio_gpu_resource_create_blob(VirtIOGPU *g,
>          QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>      }
> 
>     +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
>     +                         struct virtio_gpu_ctrl_command *cmd)
>     +{
>     +    struct virtio_gpu_status_freezing sf;
>     +
>     +    VIRTIO_GPU_FILL_CMD(sf);
>     +    virtio_gpu_bswap_32(&sf, sizeof(sf));
>     +    g->freezing = sf.freezing;
>     +}
>     +
>      static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>      {
>          struct virtio_gpu_scanout *scanout = 
> &g->parent_obj.scanout[scanout_id];
>     @@ -986,6 +996,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>          case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>              virtio_gpu_resource_detach_backing(g, cmd);
>              break;
>     +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>     +        virtio_gpu_cmd_status_freezing(g, cmd);
>     +        break;
>          default:
>              cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>              break;
>     @@ -1344,6 +1357,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, 
> Error **errp)
>          QTAILQ_INIT(&g->reslist);
>          QTAILQ_INIT(&g->cmdq);
>          QTAILQ_INIT(&g->fenceq);
>     +
>     +    g->freezing = false;
>      }
> 
>      void virtio_gpu_reset(VirtIODevice *vdev)
>     @@ -1352,8 +1367,15 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>          struct virtio_gpu_simple_resource *res, *tmp;
>          struct virtio_gpu_ctrl_command *cmd;
> 
>     -    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
>     -        virtio_gpu_resource_destroy(g, res);
>     +    /*
>     +     * If guest is suspending, we shouldn't destroy resources,
>     +     * otherwise, the display can't come back to the time when
>     +     * it was suspended after guest resumed.
>     +     */
>     +    if (!g->freezing) {
>     +        QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
>     +            virtio_gpu_resource_destroy(g, res);
>     +        }
>          }
> 
>          while (!QTAILQ_EMPTY(&g->cmdq)) {
>     diff --git a/include/hw/virtio/virtio-gpu.h 
> b/include/hw/virtio/virtio-gpu.h
>     index 2e28507efe..c21c2990fb 100644
>     --- a/include/hw/virtio/virtio-gpu.h
>     +++ b/include/hw/virtio/virtio-gpu.h
>     @@ -173,6 +173,7 @@ struct VirtIOGPU {
> 
>          uint64_t hostmem;
> 
>     +    bool freezing;
>          bool processing_cmdq;
>          QEMUTimer *fence_poll;
>          QEMUTimer *print_stats;
>     @@ -284,5 +285,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
>      void virtio_gpu_virgl_reset(VirtIOGPU *g);
>      int virtio_gpu_virgl_init(VirtIOGPU *g);
>      int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
>     +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
>     +                         struct virtio_gpu_ctrl_command *cmd);
> 
>      #endif
>     diff --git a/include/standard-headers/linux/virtio_gpu.h 
> b/include/standard-headers/linux/virtio_gpu.h
>     index 2da48d3d4c..aefffbd751 100644
>     --- a/include/standard-headers/linux/virtio_gpu.h
>     +++ b/include/standard-headers/linux/virtio_gpu.h
>     @@ -116,6 +116,9 @@ enum virtio_gpu_ctrl_type {
>             VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID,
>             VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID,
>             VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER,
>     +
>     +       /* status */
>     +       VIRTIO_GPU_CMD_STATUS_FREEZING = 0x1300,
>      };
> 
> 

Thank you for reviewing!

> Adding a new command requires new feature flag (and maybe it should be in the 
> <0x1000 range instead)
> 
> But I am not sure we need a new message at the virtio-gpu level. Gerd, wdyt?

I added this message to get notifications from guest. I don't know what level 
is more suitable. Could you please give some advice?

> 
> Maybe it's not a good place to reset all GPU resources during QEMU reset() 
> after all, if it's called during s3 and there is no mechanism to restore it. 
> Damien?

Yes, during s3, virtio_gpu_reset() is called, and all resources are destroyed. 
If there is a better place to destroy resources, I think the display problem 
may disappear and the new message I added may be not necessary.

> 
> thanks
> 
> 
> ...
> 
>      enum virtio_gpu_shm_id {
>     @@ -453,4 +456,10 @@ struct virtio_gpu_resource_unmap_blob {
>             uint32_t padding;
>      };
> 
>     +/* VIRTIO_GPU_CMD_STATUS_FREEZING */
>     +struct virtio_gpu_status_freezing {
>     +       struct virtio_gpu_ctrl_hdr hdr;
>     +       __u32 freezing;
>     +};
>     +
>      #endif
>     -- 
>     2.34.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau

-- 
Best regards,
Jiqian Chen.