qxl: properly handle upright and non-shared surfaces

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(-)

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
> 
>

>> -    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

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
> 
>