[Qemu-devel] [PATCH] qxl: don't render stuff when the vm is stopped.

[Qemu-devel] [PATCH] qxl: don't render stuff when the vm is stopped.

[Gerd Hoffmann]

This patch fixes the local qxl renderer to not kick spice-server in case
the vm is stopped.  First it is pointless because we render evevything
when the vm is stopped.  Thus there is nothing to render anyway because
a stopped guest can hardly queue more commands.  Second we avoid
triggering an assert in spice-server.

With this patch applied it is possible to take screen shots (via
screendump monitor command) from a qxl gpu even in case the guest
is stopped.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl-render.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 133d093..7084143 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -121,19 +121,17 @@ void qxl_render_update(PCIQXLDevice *qxl)
         dpy_resize(vga->ds);
     }
 
-    if (!qxl->guest_primary.commands) {
-        return;
-    }
-    qxl->guest_primary.commands = 0;
-
     update.left   = 0;
     update.right  = qxl->guest_primary.surface.width;
     update.top    = 0;
     update.bottom = qxl->guest_primary.surface.height;
 
     memset(dirty, 0, sizeof(dirty));
-    qxl_spice_update_area(qxl, 0, &update,
-                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
+    if (runstate_is_running() && qxl->guest_primary.commands) {
+        qxl->guest_primary.commands = 0;
+        qxl_spice_update_area(qxl, 0, &update,
+                              dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
+    }
     if (redraw) {
         memset(dirty, 0, sizeof(dirty));
         dirty[0] = update;
-- 
1.7.1

Re: [Qemu-devel] [PATCH] qxl: don't render stuff when the vm is stopped.

[Yonit Halperin]

Hi,
On 02/15/2012 03:11 PM, Gerd Hoffmann wrote:
> This patch fixes the local qxl renderer to not kick spice-server in case
> the vm is stopped.  First it is pointless because we render evevything
> when the vm is stopped.  Thus there is nothing to render anyway because
> a stopped guest can hardly queue more commands.
hmm...When the vm is stopped we render only commands that we already 
read. We don't do more reading from the command ring. So there may be 
other pending commands that were not rendered.
That is why I suggested allowing rendering when the vm is stopped. But 
not allowing it during loading the vm during migration.

Regards,
Yonit.
  Second we avoid
> triggering an assert in spice-server.
>
> With this patch applied it is possible to take screen shots (via
> screendump monitor command) from a qxl gpu even in case the guest
> is stopped.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   hw/qxl-render.c |   12 +++++-------
>   1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/hw/qxl-render.c b/hw/qxl-render.c
> index 133d093..7084143 100644
> --- a/hw/qxl-render.c
> +++ b/hw/qxl-render.c
> @@ -121,19 +121,17 @@ void qxl_render_update(PCIQXLDevice *qxl)
>           dpy_resize(vga->ds);
>       }
>
> -    if (!qxl->guest_primary.commands) {
> -        return;
> -    }
> -    qxl->guest_primary.commands = 0;
> -
>       update.left   = 0;
>       update.right  = qxl->guest_primary.surface.width;
>       update.top    = 0;
>       update.bottom = qxl->guest_primary.surface.height;
>
>       memset(dirty, 0, sizeof(dirty));
> -    qxl_spice_update_area(qxl, 0,&update,
> -                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
> +    if (runstate_is_running()&&  qxl->guest_primary.commands) {
> +        qxl->guest_primary.commands = 0;
> +        qxl_spice_update_area(qxl, 0,&update,
> +                              dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
> +    }
>       if (redraw) {
>           memset(dirty, 0, sizeof(dirty));
>           dirty[0] = update;

Re: [Qemu-devel] [PATCH] qxl: don't render stuff when the vm is stopped.

[Gerd Hoffmann]

On 02/15/12 14:22, Yonit Halperin wrote:
> Hi,
> On 02/15/2012 03:11 PM, Gerd Hoffmann wrote:
>> This patch fixes the local qxl renderer to not kick spice-server in case
>> the vm is stopped.  First it is pointless because we render evevything
>> when the vm is stopped.  Thus there is nothing to render anyway because
>> a stopped guest can hardly queue more commands.
> hmm...When the vm is stopped we render only commands that we already
> read. We don't do more reading from the command ring.

Ah, ok.

> So there may be
> other pending commands that were not rendered. That is why I
> suggested allowing rendering when the vm is stopped. But
> not allowing it during loading the vm during migration.

I'd prefer to keep things simple: let the spice worker run when the
guest runs, no exceptions.

We may leave some unrendered commands in the ring then.  Ok.  Is that a
problem for some reason?  Note that the guest may have more commands
queued which spice-server simply doesn't see yet due to the ring being
full.  If we stop the guest the wrong moment we can end up with a
half-done screen update operation no matter what.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] qxl: don't render stuff when the vm is stopped.

[Yonit Halperin]

On 02/15/2012 03:36 PM, Gerd Hoffmann wrote:
> On 02/15/12 14:22, Yonit Halperin wrote:
>> Hi,
>> On 02/15/2012 03:11 PM, Gerd Hoffmann wrote:
>>> This patch fixes the local qxl renderer to not kick spice-server in case
>>> the vm is stopped.  First it is pointless because we render evevything
>>> when the vm is stopped.  Thus there is nothing to render anyway because
>>> a stopped guest can hardly queue more commands.
>> hmm...When the vm is stopped we render only commands that we already
>> read. We don't do more reading from the command ring.
>
> Ah, ok.
>
>> So there may be
>> other pending commands that were not rendered. That is why I
>> suggested allowing rendering when the vm is stopped. But
>> not allowing it during loading the vm during migration.
>
> I'd prefer to keep things simple: let the spice worker run when the
> guest runs, no exceptions.
>
> We may leave some unrendered commands in the ring then.  Ok.  Is that a
> problem for some reason?  Note that the guest may have more commands
> queued which spice-server simply doesn't see yet due to the ring being
> full.  If we stop the guest the wrong moment we can end up with a
> half-done screen update operation no matter what.

Ok. Then ack after correcting the patch comment.

Thanks,
Yonit.
>
> cheers,
>    Gerd
>

Re: [Qemu-devel] [PATCH] qxl: don't render stuff when the vm is stopped.

[Alon Levy]

On Wed, Feb 15, 2012 at 02:11:06PM +0100, Gerd Hoffmann wrote:
> This patch fixes the local qxl renderer to not kick spice-server in case
> the vm is stopped.  First it is pointless because we render evevything
*everything
> when the vm is stopped.  Thus there is nothing to render anyway because
> a stopped guest can hardly queue more commands.  Second we avoid
> triggering an assert in spice-server.
> 
> With this patch applied it is possible to take screen shots (via
> screendump monitor command) from a qxl gpu even in case the guest
> is stopped.
> 

Ack, just a minor request for the comment below.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qxl-render.c |   12 +++++-------
>  1 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/qxl-render.c b/hw/qxl-render.c
> index 133d093..7084143 100644
> --- a/hw/qxl-render.c
> +++ b/hw/qxl-render.c
> @@ -121,19 +121,17 @@ void qxl_render_update(PCIQXLDevice *qxl)
>          dpy_resize(vga->ds);
>      }
>  
> -    if (!qxl->guest_primary.commands) {
> -        return;
> -    }
> -    qxl->guest_primary.commands = 0;
> -
>      update.left   = 0;
>      update.right  = qxl->guest_primary.surface.width;
>      update.top    = 0;
>      update.bottom = qxl->guest_primary.surface.height;
>  
>      memset(dirty, 0, sizeof(dirty));
> -    qxl_spice_update_area(qxl, 0, &update,
> -                          dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
> +    if (runstate_is_running() && qxl->guest_primary.commands) {
> +        qxl->guest_primary.commands = 0;
> +        qxl_spice_update_area(qxl, 0, &update,
> +                              dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC);
> +    }

Can you update the comment to note that you are also changing it so even
when there is no qxl_spice_update_area call you still honor the redraw
request?

>      if (redraw) {
>          memset(dirty, 0, sizeof(dirty));
>          dirty[0] = update;
> -- 
> 1.7.1
> 
>