[PATCH 0/2] virtio-gpu: fix blob scanout post-load

[PATCH 0/2] virtio-gpu: fix blob scanout post-load

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

The current post-loading code for scanout has a FIXME: it doesn't take the
resource region/rect into account. But there is more, when adding blob migration
support in commit f66767f75c9, I didn't realize that blob resources could be
used for scanouts. This situationn leads to a crash during post-load, as they
don't have an associated res->image.

virtio_gpu_do_set_scanout() handle all cases, but requires the associated
virtio_gpu_framebuffer, which is currently not saved during migration.

Add a v2 of "virtio-gpu-one-scanout" with the framebuffer fields, so we can
restore blob scanouts, as well as fixing the existing FIXME.

Marc-André Lureau (2):
  virtio-gpu: remove needless condition
  virtio-gpu: fix scanout migration post-load

 include/hw/virtio/virtio-gpu.h |  1 +
 hw/display/virtio-gpu.c        | 61 ++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH 1/2] virtio-gpu: remove needless condition

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

qemu_create_displaysurface_pixman() never returns NULL.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index f8a675eb30..a62ffb1627 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -679,10 +679,6 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
 
         /* realloc the surface ptr */
         scanout->ds = qemu_create_displaysurface_pixman(rect);
-        if (!scanout->ds) {
-            *error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-            return;
-        }
 #ifdef WIN32
         qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, fb->offset);
 #endif
-- 
2.43.0

[PATCH 2/2] virtio-gpu: fix scanout migration post-load

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The current post-loading code for scanout has a FIXME: it doesn't take
the resource region/rect into account. But there is more, when adding
blob migration support in commit f66767f75c9, I didn't realize that blob
resources could be used for scanouts. This situationn leads to a crash
during post-load, as they don't have an associated res->image.

virtio_gpu_do_set_scanout() handle all cases, but requires the
associated virtio_gpu_framebuffer, which is currently not saved during
migration.

Add a v2 of "virtio-gpu-one-scanout" with the framebuffer fields, so we
can restore blob scanouts, as well as fixing the existing FIXME.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/virtio/virtio-gpu.h |  1 +
 hw/display/virtio-gpu.c        | 57 ++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 584ba2ed73..9a13afb34e 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -81,6 +81,7 @@ struct virtio_gpu_scanout {
     uint32_t resource_id;
     struct virtio_gpu_update_cursor cursor;
     QEMUCursor *current_cursor;
+    struct virtio_gpu_framebuffer fb;
 };
 
 struct virtio_gpu_requested_state {
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index a62ffb1627..05c30f30b6 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -595,6 +595,7 @@ static void virtio_unref_resource(pixman_image_t *image, void *data)
 static void virtio_gpu_update_scanout(VirtIOGPU *g,
                                       uint32_t scanout_id,
                                       struct virtio_gpu_simple_resource *res,
+                                      struct virtio_gpu_framebuffer *fb,
                                       struct virtio_gpu_rect *r)
 {
     struct virtio_gpu_simple_resource *ores;
@@ -612,9 +613,10 @@ static void virtio_gpu_update_scanout(VirtIOGPU *g,
     scanout->y = r->y;
     scanout->width = r->width;
     scanout->height = r->height;
+    scanout->fb = *fb;
 }
 
-static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
+static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
                                       uint32_t scanout_id,
                                       struct virtio_gpu_framebuffer *fb,
                                       struct virtio_gpu_simple_resource *res,
@@ -640,7 +642,7 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
                       r->x, r->y, r->width, r->height,
                       fb->width, fb->height);
         *error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        return;
+        return false;
     }
 
     g->parent_obj.enable = 1;
@@ -648,11 +650,12 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
     if (res->blob) {
         if (console_has_gl(scanout->con)) {
             if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
-                virtio_gpu_update_scanout(g, scanout_id, res, r);
+                virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
             } else {
                 *error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
+                return false;
             }
-            return;
+            return true;
         }
 
         data = res->blob;
@@ -688,7 +691,8 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
                                 scanout->ds);
     }
 
-    virtio_gpu_update_scanout(g, scanout_id, res, r);
+    virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
+    return true;
 }
 
 static void virtio_gpu_set_scanout(VirtIOGPU *g,
@@ -1159,7 +1163,8 @@ static void virtio_gpu_cursor_bh(void *opaque)
 
 static const VMStateDescription vmstate_virtio_gpu_scanout = {
     .name = "virtio-gpu-one-scanout",
-    .version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 1,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(resource_id, struct virtio_gpu_scanout),
         VMSTATE_UINT32(width, struct virtio_gpu_scanout),
@@ -1171,6 +1176,12 @@ static const VMStateDescription vmstate_virtio_gpu_scanout = {
         VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout),
         VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout),
         VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout),
+        VMSTATE_UINT32_V(fb.format, struct virtio_gpu_scanout, 2),
+        VMSTATE_UINT32_V(fb.bytes_pp, struct virtio_gpu_scanout, 2),
+        VMSTATE_UINT32_V(fb.width, struct virtio_gpu_scanout, 2),
+        VMSTATE_UINT32_V(fb.height, struct virtio_gpu_scanout, 2),
+        VMSTATE_UINT32_V(fb.stride, struct virtio_gpu_scanout, 2),
+        VMSTATE_UINT32_V(fb.offset, struct virtio_gpu_scanout, 2),
         VMSTATE_END_OF_LIST()
     },
 };
@@ -1342,6 +1353,7 @@ static int virtio_gpu_blob_save(QEMUFile *f, void *opaque, size_t size,
         if (!res->blob_size) {
             continue;
         }
+        assert(!res->image);
         qemu_put_be32(f, res->resource_id);
         qemu_put_be32(f, res->blob_size);
         qemu_put_be32(f, res->iov_cnt);
@@ -1404,24 +1416,43 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
     int i;
 
     for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
-        /* FIXME: should take scanout.r.{x,y} into account */
         scanout = &g->parent_obj.scanout[i];
         if (!scanout->resource_id) {
             continue;
         }
+
         res = virtio_gpu_find_resource(g, scanout->resource_id);
         if (!res) {
             return -EINVAL;
         }
-        scanout->ds = qemu_create_displaysurface_pixman(res->image);
-        if (!scanout->ds) {
-            return -EINVAL;
-        }
+
+        if (scanout->fb.format != 0) {
+            uint32_t error = 0;
+            struct virtio_gpu_rect r = {
+                .x = scanout->x,
+                .y = scanout->y,
+                .width = scanout->width,
+                .height = scanout->height
+            };
+
+            if (!virtio_gpu_do_set_scanout(g, i, &scanout->fb, res, &r, &error)) {
+                return -EINVAL;
+            }
+        } else {
+            /* legacy v1 migration support */
+            if (!res->image) {
+                return -EINVAL;
+            }
+            scanout->ds = qemu_create_displaysurface_pixman(res->image);
+            if (!scanout->ds) {
+                return -EINVAL;
+            }
 #ifdef WIN32
-        qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
+            qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
 #endif
+            dpy_gfx_replace_surface(scanout->con, scanout->ds);
+        }
 
-        dpy_gfx_replace_surface(scanout->con, scanout->ds);
         dpy_gfx_update_full(scanout->con);
         if (scanout->cursor.resource_id) {
             update_cursor(g, &scanout->cursor);
-- 
2.43.0

Re: [PATCH 2/2] virtio-gpu: fix scanout migration post-load

[Sebastian Ott]

On Mon, 15 Jan 2024, marcandre.lureau@redhat.com wrote:
> +            scanout->ds = qemu_create_displaysurface_pixman(res->image);
> +            if (!scanout->ds) {
> +                return -EINVAL;
> +            }

"qemu_create_displaysurface_pixman() never returns NULL." ;-)

Re: [PATCH 2/2] virtio-gpu: fix scanout migration post-load

[Marc-André Lureau]

Hi

On Tue, Jan 16, 2024 at 3:17 PM Sebastian Ott <sebott@redhat.com> wrote:
>
> On Mon, 15 Jan 2024, marcandre.lureau@redhat.com wrote:
> > +            scanout->ds = qemu_create_displaysurface_pixman(res->image);
> > +            if (!scanout->ds) {
> > +                return -EINVAL;
> > +            }
>
> "qemu_create_displaysurface_pixman() never returns NULL." ;-)

Right, I'll update the first patch.

Other comments about this patch?

thanks

-- 
Marc-André Lureau

Re: [PATCH 2/2] virtio-gpu: fix scanout migration post-load

[Sebastian Ott]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Wed, 17 Jan 2024, Marc-André Lureau wrote:
> On Tue, Jan 16, 2024 at 3:17 PM Sebastian Ott <sebott@redhat.com> wrote:
>> On Mon, 15 Jan 2024, marcandre.lureau@redhat.com wrote:
>>> +            scanout->ds = qemu_create_displaysurface_pixman(res->image);
>>> +            if (!scanout->ds) {
>>> +                return -EINVAL;
>>> +            }
>>
>> "qemu_create_displaysurface_pixman() never returns NULL." ;-)
>
> Right, I'll update the first patch.
>
> Other comments about this patch?

Nope, looks good to me.

Reviewed-by: Sebastian Ott <sebott@redhat.com>