[LINUX KERNEL PATCH v3 0/1] add S3 support for virtgpu

[LINUX KERNEL PATCH v3 0/1] add S3 support for virtgpu

[Jiqian Chen]

v3:
Hi all,
Thanks for Gerd Hoffmann's advice. V3 makes below changes:
* Use enum for freeze mode, so this can be extended with more
  modes in the future.
* Rename functions and paratemers with "_S3" postfix.
And no functional changes.

Best regards,
Jiqian Chen.


v2:

Hi all,
Thanks to Marc-André Lureau, Robert Beckett and Gerd Hoffmann for
their advice and guidance. V2 makes below changes:
* Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000)
* Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and
  host can negotiate whenever freezing is supported or not.

V2 of Qemu patch:
https://lore.kernel.org/qemu-devel/20230630070016.841459-1-Jiqian.Chen@amd.com/T/#t


v1:

link,
https://lore.kernel.org/lkml/20230608063857.1677973-1-Jiqian.Chen@amd.com/

Hi all,
I am working to implement virtgpu S3 function on Xen.

Currently on Xen, if we start a guest who enables virtgpu, and then
run "echo mem > /sys/power/state" to suspend guest. And run
"sudo xl trigger <guest id> s3resume" to resume guest. We can find that
the guest kernel comes back, but the display doesn't. It just shows a
black screen.

In response to the above phenomenon, I have found two problems.

First, if we move mouse on the black screen, guest kernel still sends a
cursor request to Qemu, but Qemu doesn't response. Because when guest
is suspending, it calls device_suspend, and then call into Qemu to call
virtio_reset->__virtio_queue_reset. In __virtio_queue_reset, it clears
all virtqueue information on Qemu end. So, after guest resumes, Qemu
can't get message from virtqueue.

Second, the reason why display can't come back is that when guest is
suspending, it calls into Qemu to call virtio_reset->virtio_gpu_gl_reset.
In virtio_gpu_gl_reset, it destroys all resources and resets renderer,
which are used for display. So after guest resumes, the display can't
come back to the status when guest is suspended.

This patch initializes virtqueue when guest is resuming to solve first
problem. And it notifies Qemu that guest is suspending to prevent Qemu
destroying resources, this is to solve second problem. And then, I can
bring the display back, and everything continues their actions after
guest resumes.

Modifications on Qemu end:
https://lore.kernel.org/qemu-devel/20230608025655.1674357-2-Jiqian.Chen@amd.com/

Jiqian Chen (1):
  virtgpu: init vq during resume and notify qemu guest status

 drivers/gpu/drm/virtio/virtgpu_debugfs.c |  1 +
 drivers/gpu/drm/virtio/virtgpu_drv.c     | 39 ++++++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  5 +++
 drivers/gpu/drm/virtio/virtgpu_kms.c     | 36 ++++++++++++++++------
 drivers/gpu/drm/virtio/virtgpu_vq.c      | 16 ++++++++++
 include/uapi/linux/virtio_gpu.h          | 19 ++++++++++++
 6 files changed, 107 insertions(+), 9 deletions(-)

-- 
2.34.1

[LINUX KERNEL PATCH v3 1/1] virtgpu: init vq during resume and notify qemu guest status

[Jiqian Chen]

This patch solves two problem:

First, when we suspended guest VM, it called into Qemu to call
virtio_reset->__virtio_queue_reset, this cleared all virtuqueue
information of virtgpu on Qemu end. As a result, after guest
resumed, guest sended ctrl/cursor requests to Qemu through
virtqueue, but Qemu can't get requests from the virtqueue now.
In function virtio_queue_notify, vq->vring.desc is NULL.

So, this patch add freeze and restore function for virtgpu driver.
In freeze function, it flushes all virtqueue works and deletes
virtqueues. In restore function, it initializes virtqueues. And
then, Qemu and guest can communicate normally.

Second, when we suspended guest VM, it called into Qemu to call
virtio_reset->virtio_gpu_gl_reset, this destroyed resources and
reset renderer which were used for display. As a result, after
guest resumed, the display can't come back and we only saw a black
screen.

So, this patch add a new ctrl message VIRTIO_GPU_CMD_SET_FREEZE_MODE.
When guest is during suspending, we set freeze mode to freeze_S3 to
notify Qemu that guest entered suspending, and then Qemu will not
destroy resources. When guest is during resuming, we set freeze mode
to unfreeze to notify Qemu that guest exited suspending, and then
Qemu will keep its origin actions. As a result, the display can come
back and everything of guest can come back to the time when guest
was suspended.

Due to this implemention needs cooperation with host Qemu, so it
added a new feature flag VIRTIO_GPU_F_FREEZE_S3, so that guest and
host can negotiate whenever freeze_S3 is supported or not.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 drivers/gpu/drm/virtio/virtgpu_debugfs.c |  1 +
 drivers/gpu/drm/virtio/virtgpu_drv.c     | 39 ++++++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  5 +++
 drivers/gpu/drm/virtio/virtgpu_kms.c     | 36 ++++++++++++++++------
 drivers/gpu/drm/virtio/virtgpu_vq.c      | 16 ++++++++++
 include/uapi/linux/virtio_gpu.h          | 19 ++++++++++++
 6 files changed, 107 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index 853dd9aa397e..c84fd6d7f5f3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -55,6 +55,7 @@ static int virtio_gpu_features(struct seq_file *m, void *data)
 
 	virtio_gpu_add_bool(m, "blob resources", vgdev->has_resource_blob);
 	virtio_gpu_add_bool(m, "context init", vgdev->has_context_init);
+	virtio_gpu_add_bool(m, "freeze_S3", vgdev->has_freeze_S3);
 	virtio_gpu_add_int(m, "cap sets", vgdev->num_capsets);
 	virtio_gpu_add_int(m, "scanouts", vgdev->num_scanouts);
 	if (vgdev->host_visible_region.len) {
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index add075681e18..83ad0ac82b94 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -130,6 +130,40 @@ static void virtio_gpu_config_changed(struct virtio_device *vdev)
 	schedule_work(&vgdev->config_changed_work);
 }
 
+#ifdef CONFIG_PM
+static int virtio_gpu_freeze(struct virtio_device *dev)
+{
+	struct drm_device *ddev = dev->priv;
+	struct virtio_gpu_device *vgdev = ddev->dev_private;
+	int ret = 0;
+
+	if (vgdev->has_freeze_S3) {
+		ret = virtio_gpu_cmd_set_freeze_mode(vgdev,
+					VIRTIO_GPU_FREEZE_MODE_FREEZE_S3);
+	}
+	if (!ret) {
+		flush_work(&vgdev->ctrlq.dequeue_work);
+		flush_work(&vgdev->cursorq.dequeue_work);
+		vgdev->vdev->config->del_vqs(vgdev->vdev);
+	}
+	return ret;
+}
+
+static int virtio_gpu_restore(struct virtio_device *dev)
+{
+	struct drm_device *ddev = dev->priv;
+	struct virtio_gpu_device *vgdev = ddev->dev_private;
+	int ret;
+
+	ret = virtio_gpu_init_vqs(dev);
+	if (!ret && vgdev->has_freeze_S3) {
+		ret = virtio_gpu_cmd_set_freeze_mode(vgdev,
+					VIRTIO_GPU_FREEZE_MODE_UNFREEZE);
+	}
+	return ret;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_GPU, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -148,6 +182,7 @@ static unsigned int features[] = {
 	VIRTIO_GPU_F_RESOURCE_UUID,
 	VIRTIO_GPU_F_RESOURCE_BLOB,
 	VIRTIO_GPU_F_CONTEXT_INIT,
+	VIRTIO_GPU_F_FREEZE_S3,
 };
 static struct virtio_driver virtio_gpu_driver = {
 	.feature_table = features,
@@ -156,6 +191,10 @@ static struct virtio_driver virtio_gpu_driver = {
 	.driver.owner = THIS_MODULE,
 	.id_table = id_table,
 	.probe = virtio_gpu_probe,
+#ifdef CONFIG_PM
+	.freeze = virtio_gpu_freeze,
+	.restore = virtio_gpu_restore,
+#endif
 	.remove = virtio_gpu_remove,
 	.config_changed = virtio_gpu_config_changed
 };
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 4126c384286b..231b5a6138b2 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -246,6 +246,7 @@ struct virtio_gpu_device {
 	bool has_resource_blob;
 	bool has_host_visible;
 	bool has_context_init;
+	bool has_freeze_S3;
 	struct virtio_shm_region host_visible_region;
 	struct drm_mm host_visible_mm;
 
@@ -282,6 +283,7 @@ extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
 
 /* virtgpu_kms.c */
+int virtio_gpu_init_vqs(struct virtio_device *vdev);
 int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev);
 void virtio_gpu_deinit(struct drm_device *dev);
 void virtio_gpu_release(struct drm_device *dev);
@@ -425,6 +427,9 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
 				uint32_t width, uint32_t height,
 				uint32_t x, uint32_t y);
 
+int virtio_gpu_cmd_set_freeze_mode(struct virtio_gpu_device *vgdev,
+				virtio_gpu_freeze_mode_t freeze_mode);
+
 /* virtgpu_display.c */
 int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
 void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 5a3b5aaed1f3..a762a762f907 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -114,16 +114,33 @@ static void virtio_gpu_get_capsets(struct virtio_gpu_device *vgdev,
 	vgdev->num_capsets = num_capsets;
 }
 
-int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
+int virtio_gpu_init_vqs(struct virtio_device *vdev)
 {
 	static vq_callback_t *callbacks[] = {
 		virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack
 	};
 	static const char * const names[] = { "control", "cursor" };
+	struct drm_device *dev = vdev->priv;
+	struct virtio_gpu_device *vgdev = dev->dev_private;
+	struct virtqueue *vqs[2];
+	int ret;
+
+	virtio_gpu_init_vq(&vgdev->ctrlq, virtio_gpu_dequeue_ctrl_func);
+	virtio_gpu_init_vq(&vgdev->cursorq, virtio_gpu_dequeue_cursor_func);
+
+	ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
+	if (ret) {
+		DRM_ERROR("failed to find virt queues\n");
+		return ret;
+	}
+	vgdev->ctrlq.vq = vqs[0];
+	vgdev->cursorq.vq = vqs[1];
+	return 0;
+}
 
+int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
+{
 	struct virtio_gpu_device *vgdev;
-	/* this will expand later */
-	struct virtqueue *vqs[2];
 	u32 num_scanouts, num_capsets;
 	int ret = 0;
 
@@ -144,8 +161,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 	ida_init(&vgdev->ctx_id_ida);
 	ida_init(&vgdev->resource_ida);
 	init_waitqueue_head(&vgdev->resp_wq);
-	virtio_gpu_init_vq(&vgdev->ctrlq, virtio_gpu_dequeue_ctrl_func);
-	virtio_gpu_init_vq(&vgdev->cursorq, virtio_gpu_dequeue_cursor_func);
 
 	vgdev->fence_drv.context = dma_fence_context_alloc(1);
 	spin_lock_init(&vgdev->fence_drv.lock);
@@ -197,6 +212,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 	if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_CONTEXT_INIT)) {
 		vgdev->has_context_init = true;
 	}
+	if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_FREEZE_S3)) {
+		vgdev->has_freeze_S3 = true;
+	}
 
 	DRM_INFO("features: %cvirgl %cedid %cresource_blob %chost_visible",
 		 vgdev->has_virgl_3d    ? '+' : '-',
@@ -207,13 +225,13 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 	DRM_INFO("features: %ccontext_init\n",
 		 vgdev->has_context_init ? '+' : '-');
 
-	ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
+	DRM_INFO("features: %cfreeze_S3\n",
+		 vgdev->has_freeze_S3 ? '+' : '-');
+
+	ret = virtio_gpu_init_vqs(vdev);
 	if (ret) {
-		DRM_ERROR("failed to find virt queues\n");
 		goto err_vqs;
 	}
-	vgdev->ctrlq.vq = vqs[0];
-	vgdev->cursorq.vq = vqs[1];
 	ret = virtio_gpu_alloc_vbufs(vgdev);
 	if (ret) {
 		DRM_ERROR("failed to alloc vbufs\n");
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index b1a00c0c25a7..74b34951e4fa 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -1302,3 +1302,19 @@ void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
 
 	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
 }
+
+int virtio_gpu_cmd_set_freeze_mode(struct virtio_gpu_device *vgdev,
+				virtio_gpu_freeze_mode_t freeze_mode)
+{
+	struct virtio_gpu_set_freeze_mode *cmd_p;
+	struct virtio_gpu_vbuffer *vbuf;
+
+	cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+	memset(cmd_p, 0, sizeof(*cmd_p));
+
+	cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_SET_FREEZE_MODE);
+	cmd_p->freeze_mode = freeze_mode;
+	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+	virtio_gpu_notify(vgdev);
+	return 0;
+}
\ No newline at end of file
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index f556fde07b76..21ca87bf7417 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -65,6 +65,11 @@
  */
 #define VIRTIO_GPU_F_CONTEXT_INIT        4
 
+/*
+ * VIRTIO_GPU_CMD_SET_FREEZE_MODE
+ */
+#define VIRTIO_GPU_F_FREEZE_S3            5
+
 enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_UNDEFINED = 0,
 
@@ -100,6 +105,9 @@ enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
 	VIRTIO_GPU_CMD_MOVE_CURSOR,
 
+	/* freeze mode */
+	VIRTIO_GPU_CMD_SET_FREEZE_MODE = 0x0400,
+
 	/* success responses */
 	VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
 	VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
@@ -453,4 +461,15 @@ struct virtio_gpu_resource_unmap_blob {
 	__le32 padding;
 };
 
+/* VIRTIO_GPU_CMD_SET_FREEZE_MODE */
+typedef enum {
+	VIRTIO_GPU_FREEZE_MODE_UNFREEZE = 0,
+	VIRTIO_GPU_FREEZE_MODE_FREEZE_S3 = 3,
+} virtio_gpu_freeze_mode_t;
+
+struct virtio_gpu_set_freeze_mode {
+	struct virtio_gpu_ctrl_hdr hdr;
+	virtio_gpu_freeze_mode_t freeze_mode;
+};
+
 #endif
-- 
2.34.1

Re: [LINUX KERNEL PATCH v3 0/1] add S3 support for virtgpu

[Chen, Jiqian]

Hi all,
Sorry for the late update of the v4 patches on Qemu side: https://lore.kernel.org/qemu-devel/20230720120816.8751-1-Jiqian.Chen@amd.com/ .
And I have created an "issue" in the Qemu gitlab for tracing this problem: https://gitlab.com/qemu-project/qemu/-/issues/1860.
Do you have any other comments about my patches ? Or any guidance on what I should do next? I am looking forward to getting reply from you.

On 2023/7/20 19:58, Jiqian Chen wrote:
> v3:
> Hi all,
> Thanks for Gerd Hoffmann's advice. V3 makes below changes:
> * Use enum for freeze mode, so this can be extended with more
>   modes in the future.
> * Rename functions and paratemers with "_S3" postfix.
> And no functional changes.
> 
> Best regards,
> Jiqian Chen.
> 
> 
> v2:
> 
> Hi all,
> Thanks to Marc-André Lureau, Robert Beckett and Gerd Hoffmann for
> their advice and guidance. V2 makes below changes:
> * Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000)
> * Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and
>   host can negotiate whenever freezing is supported or not.
> 
> V2 of Qemu patch:
> https://lore.kernel.org/qemu-devel/20230630070016.841459-1-Jiqian.Chen@amd.com/T/#t
> 
> 
> v1:
> 
> link,
> https://lore.kernel.org/lkml/20230608063857.1677973-1-Jiqian.Chen@amd.com/
> 
> Hi all,
> I am working to implement virtgpu S3 function on Xen.
> 
> Currently on Xen, if we start a guest who enables virtgpu, and then
> run "echo mem > /sys/power/state" to suspend guest. And run
> "sudo xl trigger <guest id> s3resume" to resume guest. We can find that
> the guest kernel comes back, but the display doesn't. It just shows a
> black screen.
> 
> In response to the above phenomenon, I have found two problems.
> 
> First, if we move mouse on the black screen, guest kernel still sends a
> cursor request to Qemu, but Qemu doesn't response. Because when guest
> is suspending, it calls device_suspend, and then call into Qemu to call
> virtio_reset->__virtio_queue_reset. In __virtio_queue_reset, it clears
> all virtqueue information on Qemu end. So, after guest resumes, Qemu
> can't get message from virtqueue.
> 
> Second, the reason why display can't come back is that when guest is
> suspending, it calls into Qemu to call virtio_reset->virtio_gpu_gl_reset.
> In virtio_gpu_gl_reset, it destroys all resources and resets renderer,
> which are used for display. So after guest resumes, the display can't
> come back to the status when guest is suspended.
> 
> This patch initializes virtqueue when guest is resuming to solve first
> problem. And it notifies Qemu that guest is suspending to prevent Qemu
> destroying resources, this is to solve second problem. And then, I can
> bring the display back, and everything continues their actions after
> guest resumes.
> 
> Modifications on Qemu end:
> https://lore.kernel.org/qemu-devel/20230608025655.1674357-2-Jiqian.Chen@amd.com/
> 
> Jiqian Chen (1):
>   virtgpu: init vq during resume and notify qemu guest status
> 
>  drivers/gpu/drm/virtio/virtgpu_debugfs.c |  1 +
>  drivers/gpu/drm/virtio/virtgpu_drv.c     | 39 ++++++++++++++++++++++++
>  drivers/gpu/drm/virtio/virtgpu_drv.h     |  5 +++
>  drivers/gpu/drm/virtio/virtgpu_kms.c     | 36 ++++++++++++++++------
>  drivers/gpu/drm/virtio/virtgpu_vq.c      | 16 ++++++++++
>  include/uapi/linux/virtio_gpu.h          | 19 ++++++++++++
>  6 files changed, 107 insertions(+), 9 deletions(-)
> 

-- 
Best regards,
Jiqian Chen.