| Message ID | 1329686886-6853-8-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/19/12 22:28, Alon Levy wrote: > Add an implementation of the DisplayAllocator callbacks for qxl. Uses > the QEMU_ALLOCATED_FLAG to ensure vga/vga_draw_graphic does the 24 to 32 > bits per pixel line convertion. Since free/resize/create are defined in > qxl.c, it is easy to ensure consistent usage of the flag (it means > QXL_ALLOCATED basically). > > With this patch and the previous two screendump works for vga and qxl > modes when using qxl and spice together. This might break sdl/vnc with > spice, untested since it isn't of known use. Breaking vnc+spice being used in parallel breaking is exactly what I suspect might be possible. IIRC there where discussions about just enabling both vnc+spice, then let users pick what to use, so I'd prefer to not break this. Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too. cheers, Gerd
On Mon, Feb 20, 2012 at 12:41:09PM +0100, Gerd Hoffmann wrote: > On 02/19/12 22:28, Alon Levy wrote: > > Add an implementation of the DisplayAllocator callbacks for qxl. Uses > > the QEMU_ALLOCATED_FLAG to ensure vga/vga_draw_graphic does the 24 to 32 > > bits per pixel line convertion. Since free/resize/create are defined in > > qxl.c, it is easy to ensure consistent usage of the flag (it means > > QXL_ALLOCATED basically). > > > > With this patch and the previous two screendump works for vga and qxl > > modes when using qxl and spice together. This might break sdl/vnc with > > spice, untested since it isn't of known use. > > Breaking vnc+spice being used in parallel breaking is exactly what I > suspect might be possible. IIRC there where discussions about just > enabling both vnc+spice, then let users pick what to use, so I'd prefer > to not break this. I'll send a series that works with vnc+spice and sdl+spice (didn't test sdl+vnc+spice), and screendumps at the same time. > > Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too. So you think I should introduce a new flag? I could introduce a QEMU_DISPLAYSURFACE_MAX and then use QEMU_DISPLAY_SURFACE_MAX<<1 for my own flag. Actually I think the right thing is to move/copy the 24bit->32bit convertion from vga.c to pflib.c, what do you think? > > cheers, > Gerd >
Hi, > I'll send a series that works with vnc+spice and sdl+spice (didn't test > sdl+vnc+spice), and screendumps at the same time. > >> >> Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too. QEMU_ALLOCATED_FLAG is just a flag which is used by the defaultallocator_free_displaysurface function to figure whenever the displaysurface memory was allocated by qemu_alloc_display or not. I think vga.c shouldn't look at it in the first place, and faking it because vga.c looks at it is even worse. I'm also not sure it is the right approach to to have qxl register a display allocator in the first place. The only other place doing this is sdl, which is a user interface. None of the gfx card emulations in the tree do that ... > Actually I think the right thing is to move/copy the 24bit->32bit convertion > from vga.c to pflib.c, what do you think? Agree, although that easily gets a patch series of its own when you collect optimized format conversion functions to move them over to pflib ... BTW: qxl insisting on a shared displaysurface isn't very clean too, it better should be able to fallback to just copying/converting for the non-shared case. cheers, Gerd
On Mon, Feb 20, 2012 at 02:18:19PM +0100, Gerd Hoffmann wrote: > Hi, > > > I'll send a series that works with vnc+spice and sdl+spice (didn't test > > sdl+vnc+spice), and screendumps at the same time. > > > >> > >> Setting the QEMU_ALLOCATED_FLAG flag sounds hackish too. > > QEMU_ALLOCATED_FLAG is just a flag which is used by the > defaultallocator_free_displaysurface function to figure whenever the > displaysurface memory was allocated by qemu_alloc_display or not. > > I think vga.c shouldn't look at it in the first place, and faking it > because vga.c looks at it is even worse. > > I'm also not sure it is the right approach to to have qxl register a > display allocator in the first place. The only other place doing this > is sdl, which is a user interface. None of the gfx card emulations in > the tree do that ... TBH I'm having problems without doing it with my own Allocator. In particular calling ppm_save from spice_server thread seems to be a problem. I can remove the hackish use of QEMU_ALLOCATED_FLAG by not checking for it in vga.c in the first place, but I don't know what other code it affects (well, I do - every machine using vga, but I'm not testing all of these). Also reallocating the displaysurface seems to be the wrong thing, and that's what we do whenever we check in qxl_render_update is_shared_buffer. > > > Actually I think the right thing is to move/copy the 24bit->32bit convertion > > from vga.c to pflib.c, what do you think? > > Agree, although that easily gets a patch series of its own when you > collect optimized format conversion functions to move them over to pflib ... Yes, I think I'll defer that to later. > > BTW: qxl insisting on a shared displaysurface isn't very clean too, it > better should be able to fallback to just copying/converting for the > non-shared case. I'll try to get that working, although it seems to require having some timer/bh to do the ppm_save. > > cheers, > Gerd > >
Hi, > TBH I'm having problems without doing it with my own Allocator. In > particular calling ppm_save from spice_server thread seems to be a > problem. Grabbed the qemu mutex? >> BTW: qxl insisting on a shared displaysurface isn't very clean too, it >> better should be able to fallback to just copying/converting for the >> non-shared case. > > I'll try to get that working, although it seems to require having some > timer/bh to do the ppm_save. bh should do, that is one of the things they are supposed to handle. cheers, Gerd
On Tue, Feb 21, 2012 at 08:57:54AM +0100, Gerd Hoffmann wrote: > Hi, > > > TBH I'm having problems without doing it with my own Allocator. In > > particular calling ppm_save from spice_server thread seems to be a > > problem. > > Grabbed the qemu mutex? Didn't we remove qemu mutex grabbing from the spice server thread a long long time ago? I've switched to using a bh, it seems to work better but still not perfectly. I've also found my notes on why I tried the allocator approach in the first place (all the following is in QXL_NATIVE mode): Boot with qxl only, no vnc or sdl (so no qxl_render_update users except screendump). Issue screendump. Right now qxl_render_update checks if the displaysurface buffer is not shared, meaning it was allocated by qemu, and in this case it replaces it with the flipped buffer. But right after that surface->data gets reset, by vga_hw_screen_dump: vga_hw_screen_dump/console_select/qemu_resize_displaysurface/ds->allocator->resize_displaysurface/defaultallocator_resize_displaysurface/qemu_alloc_display Hence my line of thought that replacing the allocator with my own would prevent this. Since you have misgivings about using our own allocator that I don't know how to resolve, I'm instead doing a second reallocation in our dpy_resize callback qxl.c:display_resize, in affect it means that we have three allocations and three deallocations for every screendump. Do you still think it's less ugly then an allocator? note that I have sdl and vnc working with spice with my allocator scheme. (just didn't test all three together yet). > > >> BTW: qxl insisting on a shared displaysurface isn't very clean too, it > >> better should be able to fallback to just copying/converting for the > >> non-shared case. > > > > I'll try to get that working, although it seems to require having some > > timer/bh to do the ppm_save. > > bh should do, that is one of the things they are supposed to handle. > > cheers, > Gerd > >
diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 7b120ab..8280b91 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -82,7 +82,6 @@ void qxl_render_update(PCIQXLDevice *qxl, const char *filename) { VGACommonState *vga = &qxl->vga; QXLRect dirty[32]; - void *ptr; int redraw = 0; QXLRenderUpdateData *data; QXLCookie *cookie; @@ -96,38 +95,7 @@ void qxl_render_update(PCIQXLDevice *qxl, const char *filename) 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", - __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); + qemu_resize_displaysurface(qxl->vga.ds, 0, 0); } if (!qxl->guest_primary.commands) { diff --git a/hw/qxl.c b/hw/qxl.c index 6e25bd1..ec43c4e 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -138,6 +138,92 @@ void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...) } } +static DisplaySurface *qxl_create_displaysurface(int width, int height) +{ + PCIQXLDevice *qxl = qxl0; + DisplaySurface *ret; + void *ptr; + + dprint(qxl, 1, "%s: %d x %d (have %d, %d)\n", __func__, + width, height, + qxl->guest_primary.surface.width, + qxl->guest_primary.surface.height); + if (width != 0 && height != 0 && + (qxl->mode == QXL_MODE_VGA || qxl->mode == QXL_MODE_UNDEFINED)) { + /* initial surface creation in vga mode, use give width and height */ + qxl->guest_primary.surface.width = width; + qxl->guest_primary.surface.height = height; + qxl->guest_primary.qxl_stride = width * 4; + qxl->guest_primary.abs_stride = width * 4; + qxl->guest_primary.bytes_pp = 4; + qxl->guest_primary.bits_pp = 32; + qxl->guest_primary.flipped = 0; + qxl->guest_primary.data = qxl->ssd.buf; + } else { + if (qxl->guest_primary.flipped) { + g_free(qxl->guest_primary.flipped); + qxl->guest_primary.flipped = NULL; + } + 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", + __func__, + 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"); + ret = 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); + /* + * set this flag to ensure vga_draw_graphic treats this as a non shared + * buffer, and so does the correct convertion to 32 bits that we always use. + */ + ret->flags |= QEMU_ALLOCATED_FLAG; + return ret; +} + +static void qxl_free_displaysurface(DisplaySurface *surface) +{ + g_free(surface); +} + +static DisplaySurface *qxl_resize_displaysurface(DisplaySurface *surface, + int width, int height) +{ + if (surface != qxl0->vga.ds->surface) { + qxl_free_displaysurface(surface); + } + return qxl_create_displaysurface(width, height); +} + +static struct DisplayAllocator qxl_displaysurface_allocator = { + qxl_create_displaysurface, + qxl_resize_displaysurface, + qxl_free_displaysurface +}; + +void qxl_set_allocator(DisplayState *ds) +{ + if (register_displayallocator(ds, &qxl_displaysurface_allocator) == + &qxl_displaysurface_allocator) { + printf("qxl_set_allocator success\n"); + dpy_resize(ds); + } +} void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id, struct QXLRect *area, struct QXLRect *dirty_rects, @@ -1493,9 +1579,18 @@ static void qxl_hw_invalidate(void *opaque) static void qxl_hw_screen_dump(void *opaque, const char *filename) { PCIQXLDevice *qxl = opaque; - VGACommonState *vga = &qxl->vga; switch (qxl->mode) { + case QXL_MODE_VGA: + /* + * TODO: vga_hw_screen_dump needless does a number of ppm_save calls + * fix it instead of redoing it correctly here (needs testing which is + * why it isn't yet done) + */ + qxl->vga.invalidate(&qxl->vga); + vga_hw_update(); + ppm_save(filename, qxl->ssd.ds->surface); + break; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: /* @@ -1508,9 +1603,6 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename) */ qxl_render_update(qxl, filename); break; - case QXL_MODE_VGA: - vga->screen_dump(vga, filename); - break; default: break; } @@ -1682,9 +1774,8 @@ static int qxl_init_primary(PCIDevice *dev) vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate, qxl_hw_screen_dump, qxl_hw_text_update, qxl); - qemu_spice_display_init_common(&qxl->ssd, vga->ds); - qxl0 = qxl; + qemu_spice_display_init_common(&qxl->ssd, vga->ds); register_displaychangelistener(vga->ds, &display_listener); return qxl_init_common(qxl); diff --git a/ui/spice-display.c b/ui/spice-display.c index 680e6f4..7fd18f4 100644 --- a/ui/spice-display.c +++ b/ui/spice-display.c @@ -295,6 +295,7 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds) ssd->mouse_y = -1; ssd->bufsize = (16 * 1024 * 1024); ssd->buf = g_malloc(ssd->bufsize); + qxl_set_allocator(ds); } /* display listener callbacks */ diff --git a/ui/spice-display.h b/ui/spice-display.h index 8f286f8..9f24097 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -123,3 +123,5 @@ void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd, void qemu_spice_wakeup(SimpleSpiceDisplay *ssd); void qemu_spice_start(SimpleSpiceDisplay *ssd); void qemu_spice_stop(SimpleSpiceDisplay *ssd); + +void qxl_set_allocator(DisplayState *ds);
Add an implementation of the DisplayAllocator callbacks for qxl. Uses the QEMU_ALLOCATED_FLAG to ensure vga/vga_draw_graphic does the 24 to 32 bits per pixel line convertion. Since free/resize/create are defined in qxl.c, it is easy to ensure consistent usage of the flag (it means QXL_ALLOCATED basically). With this patch and the previous two screendump works for vga and qxl modes when using qxl and spice together. This might break sdl/vnc with spice, untested since it isn't of known use. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 34 +----------------- hw/qxl.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++--- ui/spice-display.c | 1 + ui/spice-display.h | 2 + 4 files changed, 101 insertions(+), 39 deletions(-)