Re: [Qemu-devel] [PATCH] qxl: properly handle upright and non-shared surfaces

[Alon Levy]

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 <address@hidden>

just a few nitpicks.

> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  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
> 
>