| Message ID | 1329860377-6284-7-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, It's not obvious to me how the non-flipped case (qxl_stride > 0) is handled now. Have you tested this with both windows+linux guests? cheers, Gerd
On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote: > Hi, > > It's not obvious to me how the non-flipped case (qxl_stride > 0) is > handled now. Have you tested this with both windows+linux guests? It isn't handled. The simplest way is just to if on the stride and do a single memcpy instead of individual line memcpy. This of course means we are doing a redundant copy, since using our own DisplayAllocator or just the existing deallocate + our own allocate of ds->surface->data removes one copy. Since the main use case is screendump, I don't think it's a big deal. How does that sound? > > cheers, > Gerd > >
On 02/22/12 13:28, Alon Levy wrote: > On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote: >> Hi, >> >> It's not obvious to me how the non-flipped case (qxl_stride > 0) is >> handled now. Have you tested this with both windows+linux guests? > > It isn't handled. The simplest way is just to if on the stride and do a > single memcpy instead of individual line memcpy. Single memcpy works only for full scanlines. qxl_flip can be extended to handle both cases (and should probably also renamed then). > This of course means we > are doing a redundant copy, No. You can wrap the qxl_flip call into ... if (is_shared_buffer()) { ... }. ... to skip the copy if it isn't needed. > since using our own DisplayAllocator or just > the existing deallocate + our own allocate of ds->surface->data removes > one copy. I would just do if (qxl_stride > 0) { qemu_free_displaysurface qemu_create_displaysurface_from } else { qemu_resize_displaysurface } cheers, Gerd
On Wed, Feb 22, 2012 at 03:09:09PM +0100, Gerd Hoffmann wrote: > On 02/22/12 13:28, Alon Levy wrote: > > On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote: > >> Hi, > >> > >> It's not obvious to me how the non-flipped case (qxl_stride > 0) is > >> handled now. Have you tested this with both windows+linux guests? > > > > It isn't handled. The simplest way is just to if on the stride and do a > > single memcpy instead of individual line memcpy. > > Single memcpy works only for full scanlines. qxl_flip can be extended > to handle both cases (and should probably also renamed then). > > > This of course means we > > are doing a redundant copy, > > No. You can wrap the qxl_flip call into ... > > if (is_shared_buffer()) { ... }. > > ... to skip the copy if it isn't needed. > > > since using our own DisplayAllocator or just > > the existing deallocate + our own allocate of ds->surface->data removes > > one copy. > > I would just do > > if (qxl_stride > 0) { > qemu_free_displaysurface > qemu_create_displaysurface_from > } else { > qemu_resize_displaysurface > } hmm. Yes, I know we can do that since it already does it right now. I guess with the console_select call gone it should be ok. > > cheers, > Gerd >
On Wed, Feb 22, 2012 at 03:09:09PM +0100, Gerd Hoffmann wrote: > On 02/22/12 13:28, Alon Levy wrote: > > On Wed, Feb 22, 2012 at 12:18:50PM +0100, Gerd Hoffmann wrote: > >> Hi, > >> > >> It's not obvious to me how the non-flipped case (qxl_stride > 0) is > >> handled now. Have you tested this with both windows+linux guests? > > > > It isn't handled. The simplest way is just to if on the stride and do a > > single memcpy instead of individual line memcpy. > > Single memcpy works only for full scanlines. qxl_flip can be extended > to handle both cases (and should probably also renamed then). > > > This of course means we > > are doing a redundant copy, > > No. You can wrap the qxl_flip call into ... > > if (is_shared_buffer()) { ... }. > > ... to skip the copy if it isn't needed. > > > since using our own DisplayAllocator or just > > the existing deallocate + our own allocate of ds->surface->data removes > > one copy. > > I would just do > > if (qxl_stride > 0) { > qemu_free_displaysurface > qemu_create_displaysurface_from > } else { > qemu_resize_displaysurface > } > Ok, this all works fine, thanks, just one note - linux qxl also uses negative stride, so actually there is no user for positive stride (and hence that code path is untested). > cheers, > Gerd >
diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 133d093..4518a56 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -23,10 +23,15 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect) { - uint8_t *src = qxl->guest_primary.data; - uint8_t *dst = qxl->guest_primary.flipped; + uint8_t *src; + uint8_t *dst = qxl->vga.ds->surface->data; int len, i; + 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); + } + src = qxl->guest_primary.data; src += (qxl->guest_primary.surface.height - rect->top - 1) * qxl->guest_primary.abs_stride; dst += rect->top * qxl->guest_primary.abs_stride; @@ -75,50 +80,20 @@ void qxl_render_update(PCIQXLDevice *qxl) { VGACommonState *vga = &qxl->vga; QXLRect dirty[32], update; - void *ptr; int i, redraw = 0; - - if (!is_buffer_shared(vga->ds->surface)) { - dprint(qxl, 1, "%s: restoring shared displaysurface\n", __func__); - qxl->guest_primary.resized++; - qxl->guest_primary.commands++; - redraw = 1; - } + DisplaySurface *surface = vga->ds->surface; if (qxl->guest_primary.resized) { qxl->guest_primary.resized = 0; - if (qxl->guest_primary.flipped) { - g_free(qxl->guest_primary.flipped); - qxl->guest_primary.flipped = NULL; - } - qemu_free_displaysurface(vga->ds); - qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram); - if (qxl->guest_primary.qxl_stride < 0) { - /* spice surface is upside down -> need extra buffer to flip */ - qxl->guest_primary.flipped = - g_malloc(qxl->guest_primary.surface.width * - qxl->guest_primary.abs_stride); - ptr = qxl->guest_primary.flipped; - } else { - ptr = qxl->guest_primary.data; - } - dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n", + dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d\n", __FUNCTION__, qxl->guest_primary.surface.width, qxl->guest_primary.surface.height, qxl->guest_primary.qxl_stride, qxl->guest_primary.bytes_pp, - qxl->guest_primary.bits_pp, - qxl->guest_primary.flipped ? "yes" : "no"); - vga->ds->surface = - qemu_create_displaysurface_from(qxl->guest_primary.surface.width, - qxl->guest_primary.surface.height, - qxl->guest_primary.bits_pp, - qxl->guest_primary.abs_stride, - ptr); - dpy_resize(vga->ds); + qxl->guest_primary.bits_pp); } if (!qxl->guest_primary.commands) { @@ -138,14 +113,19 @@ void qxl_render_update(PCIQXLDevice *qxl) memset(dirty, 0, sizeof(dirty)); dirty[0] = update; } - + if (surface->width != qxl->guest_primary.surface.width || + surface->height != qxl->guest_primary.surface.height) { + dprint(qxl, 1, "%s: resizing displaysurface to guest_primary\n", + __func__); + qemu_resize_displaysurface(vga->ds, + qxl->guest_primary.surface.width, + qxl->guest_primary.surface.height); + } for (i = 0; i < ARRAY_SIZE(dirty); i++) { if (qemu_spice_rect_is_empty(dirty+i)) { break; } - if (qxl->guest_primary.flipped) { - qxl_flip(qxl, dirty+i); - } + qxl_flip(qxl, dirty+i); dpy_update(vga->ds, dirty[i].left, dirty[i].top, dirty[i].right - dirty[i].left, diff --git a/hw/qxl.h b/hw/qxl.h index 739ec27..eadd016 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -52,7 +52,7 @@ typedef struct PCIQXLDevice { uint32_t abs_stride; uint32_t bits_pp; uint32_t bytes_pp; - uint8_t *data, *flipped; + uint8_t *data; } guest_primary; struct surfaces {
Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 58 ++++++++++++++++++------------------------------------- hw/qxl.h | 2 +- 2 files changed, 20 insertions(+), 40 deletions(-)