Patchwork qxl: properly handle upright and non-shared surfaces

login
register
mail settings
Submitter Gerd Hoffmann
Date Feb. 27, 2012, 10:10 a.m.
Message ID <1330337410-21905-1-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/143171/
State New
Headers show

Comments

Gerd Hoffmann - Feb. 27, 2012, 10:10 a.m.
Although qxl creates a shared displaysurface when the qxl surface is
upright and doesn't need to be flipped there is no guarantee that the
surface doesn't become unshared for some reason.  Rename qxl_flip to
qxl_blit and fix it to handle both flip and non-flip cases.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl-render.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)
Alon Levy - Feb. 27, 2012, 7:13 p.m.
On Mon, Feb 27, 2012 at 11:10:10AM +0100, Gerd Hoffmann wrote:
> Although qxl creates a shared displaysurface when the qxl surface is
> upright and doesn't need to be flipped there is no guarantee that the
> surface doesn't become unshared for some reason.  Rename qxl_flip to
> qxl_blit and fix it to handle both flip and non-flip cases.

Reviewed-by: Alon Levy <alevy@redhat.com>

just a few nitpicks.

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qxl-render.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/qxl-render.c b/hw/qxl-render.c
> index f323a4d..25857f6 100644
> --- a/hw/qxl-render.c
> +++ b/hw/qxl-render.c
> @@ -21,25 +21,31 @@
>  
>  #include "qxl.h"
>  
> -static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
> +static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
>  {
>      uint8_t *src;
>      uint8_t *dst = qxl->vga.ds->surface->data;
>      int len, i;
>  
> -    if (qxl->guest_primary.qxl_stride > 0) {
> +    if (is_buffer_shared(qxl->vga.ds->surface)) {

ok, /me regrets not liking this predicate and using qxl_stride > 0
instead.

>          return;
>      }
>      if (!qxl->guest_primary.data) {
>          dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__);
>          qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
>      }
> -    dprint(qxl, 1, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
> +    dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,

You know 2 is used right now for high frequency stuff, like
interface_get_command? I think this should be lower. Anyway, not a big
deal.

>              qxl->guest_primary.qxl_stride,
>              rect->left, rect->right, rect->top, rect->bottom);
>      src = qxl->guest_primary.data;
> -    src += (qxl->guest_primary.surface.height - rect->top - 1) *
> -        qxl->guest_primary.abs_stride;
> +    if (qxl->guest_primary.qxl_stride < 0) {
> +        /* qxl surface is upside down, walk src scanlines
> +         * in reverse order to flip it */

This passed checkpatch.pl? I demand equal mistreatment! :)

> +        src += (qxl->guest_primary.surface.height - rect->top - 1) *
> +            qxl->guest_primary.abs_stride;
> +    } else {
> +        src += rect->top * qxl->guest_primary.abs_stride;
> +    }
>      dst += rect->top  * qxl->guest_primary.abs_stride;
>      src += rect->left * qxl->guest_primary.bytes_pp;
>      dst += rect->left * qxl->guest_primary.bytes_pp;
> @@ -48,7 +54,7 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
>      for (i = rect->top; i < rect->bottom; i++) {
>          memcpy(dst, src, len);
>          dst += qxl->guest_primary.abs_stride;
> -        src -= qxl->guest_primary.abs_stride;
> +        src += qxl->guest_primary.qxl_stride;
>      }
>  }
>  
> @@ -132,7 +138,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
>          if (qemu_spice_rect_is_empty(qxl->dirty+i)) {
>              break;
>          }
> -        qxl_flip(qxl, qxl->dirty+i);
> +        qxl_blit(qxl, qxl->dirty+i);
>          dpy_update(vga->ds,
>                     qxl->dirty[i].left, qxl->dirty[i].top,
>                     qxl->dirty[i].right - qxl->dirty[i].left,
> -- 
> 1.7.1
> 
>
Gerd Hoffmann - Feb. 28, 2012, 8:29 a.m.
>> -    dprint(qxl, 1, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
>> +    dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
> 
> You know 2 is used right now for high frequency stuff, like
> interface_get_command? I think this should be lower. Anyway, not a big
> deal.

/me used '1' for important but infrequent stuff, basically all init ops,
mode switching etc.  This can happen quite alot in case you are running
with SDL.  Maybe we need not just 1+2 but 1+2+3 levels, with 3 for the
really frequent stuff which will flood the logfiles.

Or even better just turn them all into tracepoints ...

cheers,
  Gerd
Alon Levy - Feb. 28, 2012, 8:31 a.m.
On Tue, Feb 28, 2012 at 09:29:38AM +0100, Gerd Hoffmann wrote:
> >> -    dprint(qxl, 1, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
> >> +    dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
> > 
> > You know 2 is used right now for high frequency stuff, like
> > interface_get_command? I think this should be lower. Anyway, not a big
> > deal.
> 
> /me used '1' for important but infrequent stuff, basically all init ops,
> mode switching etc.  This can happen quite alot in case you are running
> with SDL.  Maybe we need not just 1+2 but 1+2+3 levels, with 3 for the
> really frequent stuff which will flood the logfiles.
> 
> Or even better just turn them all into tracepoints ...
> 

Yeah, I started working on this but didn't finish. I'll try to do it.

> cheers,
>   Gerd
> 
>

Patch

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index f323a4d..25857f6 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -21,25 +21,31 @@ 
 
 #include "qxl.h"
 
-static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
+static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
 {
     uint8_t *src;
     uint8_t *dst = qxl->vga.ds->surface->data;
     int len, i;
 
-    if (qxl->guest_primary.qxl_stride > 0) {
+    if (is_buffer_shared(qxl->vga.ds->surface)) {
         return;
     }
     if (!qxl->guest_primary.data) {
         dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__);
         qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram);
     }
-    dprint(qxl, 1, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
+    dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__,
             qxl->guest_primary.qxl_stride,
             rect->left, rect->right, rect->top, rect->bottom);
     src = qxl->guest_primary.data;
-    src += (qxl->guest_primary.surface.height - rect->top - 1) *
-        qxl->guest_primary.abs_stride;
+    if (qxl->guest_primary.qxl_stride < 0) {
+        /* qxl surface is upside down, walk src scanlines
+         * in reverse order to flip it */
+        src += (qxl->guest_primary.surface.height - rect->top - 1) *
+            qxl->guest_primary.abs_stride;
+    } else {
+        src += rect->top * qxl->guest_primary.abs_stride;
+    }
     dst += rect->top  * qxl->guest_primary.abs_stride;
     src += rect->left * qxl->guest_primary.bytes_pp;
     dst += rect->left * qxl->guest_primary.bytes_pp;
@@ -48,7 +54,7 @@  static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
     for (i = rect->top; i < rect->bottom; i++) {
         memcpy(dst, src, len);
         dst += qxl->guest_primary.abs_stride;
-        src -= qxl->guest_primary.abs_stride;
+        src += qxl->guest_primary.qxl_stride;
     }
 }
 
@@ -132,7 +138,7 @@  static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
         if (qemu_spice_rect_is_empty(qxl->dirty+i)) {
             break;
         }
-        qxl_flip(qxl, qxl->dirty+i);
+        qxl_blit(qxl, qxl->dirty+i);
         dpy_update(vga->ds,
                    qxl->dirty[i].left, qxl->dirty[i].top,
                    qxl->dirty[i].right - qxl->dirty[i].left,