[Qemu-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Qemu-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

RHBZ #790083

Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
---
 hw/qxl.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index bc03c1d..a2a3380 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1478,14 +1478,21 @@ static void qxl_vm_change_state_handler(void *opaque, int running,
          * called
          */
          qxl_update_irq(qxl);
-    } else if (qxl->mode == QXL_MODE_NATIVE) {
-        /* dirty all vram (which holds surfaces) and devram (primary surface)
+    } else {
+        /* dirty all vram (which holds surfaces) and the primary surface
          * to make sure they are saved */
         /* FIXME #1: should go out during "live" stage */
         /* FIXME #2: we only need to save the areas which are actually used */
-        qxl_set_dirty(&qxl->vram_bar, 0, qxl->vram_size);
-        qxl_set_dirty(&qxl->vga.vram, qxl->shadow_rom.draw_area_offset,
-                      qxl->shadow_rom.surface0_area_size);
+        switch (qxl->mode) {
+        case QXL_MODE_NATIVE:
+            qxl_set_dirty(&qxl->vram_bar, 0, qxl->vram_size);
+        case QXL_MODE_COMPAT:
+            qxl_set_dirty(&qxl->vga.vram, qxl->shadow_rom.draw_area_offset,
+                          qxl->shadow_rom.surface0_area_size);
+            break;
+        default:
+            break;
+        }
     }
 }
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Gerd Hoffmann]

On 02/14/12 09:10, Yonit Halperin wrote:
> RHBZ #790083
> 
> Signed-off-by: Yonit Halperin <yhalperi@redhat.com>

You are doing two things in one patch:  (a) fix the compat mode bug,
which also matches the patch description, and (b) skip vram when it is
unused (in compat mode).

I'd love to see (b) done in a different way: simply walk all surfaces
and tag them dirty.  Will have the same effect for compat mode (no
surfaces used -> nothing tagged dirty) and additionally it will (in
native mode) only migrate over the vram areas which are actually filled
with surfaces.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

On 02/14/2012 10:35 AM, Gerd Hoffmann wrote:
> On 02/14/12 09:10, Yonit Halperin wrote:
>> RHBZ #790083
>>
>> Signed-off-by: Yonit Halperin<yhalperi@redhat.com>
>
> You are doing two things in one patch:  (a) fix the compat mode bug,
> which also matches the patch description, and (b) skip vram when it is
> unused (in compat mode).
>
> I'd love to see (b) done in a different way: simply walk all surfaces
> and tag them dirty.  Will have the same effect for compat mode (no
> surfaces used ->  nothing tagged dirty) and additionally it will (in
> native mode) only migrate over the vram areas which are actually filled
> with surfaces.
>
I can do it, by retrieving the surfaces addresses from the tracked guest 
commands. However, if we already do it, it would be even better if we 
just dirty only the areas that are actually modified by the update_area 
calls. The problem is that (1) spice-server updates surfaces also 
without request from driver. We can add a cb to the interface or use the 
async_complete cb with a special flag (2) async_complete cb is called 
from spice server context. We can add a pipe for update_area dirty 
events, and make sure that it is handled, before migration moves from 
the live stage.

Yonit.
> thanks,
>    Gerd

Re: [Qemu-devel] [Spice-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

On 02/14/2012 11:10 AM, Yonit Halperin wrote:
> On 02/14/2012 10:35 AM, Gerd Hoffmann wrote:
>> On 02/14/12 09:10, Yonit Halperin wrote:
>>> RHBZ #790083
>>>
>>> Signed-off-by: Yonit Halperin<yhalperi@redhat.com>
>>
>> You are doing two things in one patch: (a) fix the compat mode bug,
>> which also matches the patch description, and (b) skip vram when it is
>> unused (in compat mode).
>>
>> I'd love to see (b) done in a different way: simply walk all surfaces
>> and tag them dirty. Will have the same effect for compat mode (no
>> surfaces used -> nothing tagged dirty) and additionally it will (in
>> native mode) only migrate over the vram areas which are actually filled
>> with surfaces.
>>
> I can do it, by retrieving the surfaces addresses from the tracked guest
> commands. However, if we already do it, it would be even better if we
> just dirty only the areas that are actually modified by the update_area
> calls. The problem is that (1) spice-server updates surfaces also
> without request from driver. We can add a cb to the interface or use the
> async_complete cb with a special flag (2) async_complete cb is called
> from spice server context. We can add a pipe for update_area dirty
> events, and make sure that it is handled, before migration moves from
> the live stage.
Giving it another thought, I don't really like it because surfaces are 
destroyed with high frequency. So it will be a waste.
So doing it just before migration sounds better.
>
> Yonit.
>> thanks,
>> Gerd
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Gerd Hoffmann]

  Hi,

> I can do it, by retrieving the surfaces addresses from the tracked guest
> commands.

Exactly.

> However, if we already do it, it would be even better if we
> just dirty only the areas that are actually modified by the update_area
> calls. The problem is that (1) spice-server updates surfaces also
> without request from driver.

On worker->stop() for example, which renderes all outstanding commands
so all state is flushed to the surfaces (and thereby device memory).
This is done on vm_stop too, so I wouldn't be surprised if most surfaces
are dirtied anyway at this point.  Getting notifications about
spice-server touching surfaces doesn't buy us much then.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

On 02/14/2012 11:24 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> I can do it, by retrieving the surfaces addresses from the tracked guest
>> commands.
>
> Exactly.
>
>> However, if we already do it, it would be even better if we
>> just dirty only the areas that are actually modified by the update_area
>> calls. The problem is that (1) spice-server updates surfaces also
>> without request from driver.
>
> On worker->stop() for example, which renderes all outstanding commands
> so all state is flushed to the surfaces (and thereby device memory).
> This is done on vm_stop too, so I wouldn't be surprised if most surfaces
> are dirtied anyway at this point.  Getting notifications about
> spice-server touching surfaces doesn't buy us much then.
We also render drawables that are not hidden by other ones, when we lack 
free memory in the driver.
In addition inter-surfaces dependencies are handled in a brute-force 
manner by rendering old drawables (I plan to improve this). But anyway,
as I mentioned it the preceding email, since most of the surfaces are 
destroyed (at least with the current drivers), I prefer not to dirty 
them "live". So I'll make the change you originally suggested.

Regards,
Yonit.

>
> cheers,
>    Gerd
>