Chun Yan Liu
2010-Oct-20 06:38 UTC
[Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case
In hw/xenfb.c, xenfb_guest_copy() function doesn''t handle xenfb->depth=32 case. When xenfb->depth=32, it will report error and won''t copy data from the guest framebuffer region into QEMU''s displaysurface, thus when enter CTRL+ALT+2 to qemu monitor console then CTRL+ALT+1 back to guest X window, the screen cannot be restored. This patch adds two things: 1. when xenfb->depth equals destination bpp, do not need data translation, simply copy data from guest framebuffer into QEMU''s displaysurface. 2. when xenfb->depth and destination bpp differs, add processing for xenfb->depth=32 case. Signed-off by Chunyan Liu <cyliu@novell.com> diff -r e4f337bb97f7 tools/ioemu-qemu-xen/hw/xenfb.c --- a/hw/xenfb.c Wed Oct 20 19:39:28 2010 +0800 +++ b/hw/xenfb.c Wed Oct 20 21:42:37 2010 +0800 @@ -612,6 +612,12 @@ uint8_t *data = ds_get_data(xenfb->c.ds); if (!is_buffer_shared(xenfb->c.ds->surface)) { + if (xenfb->depth == bpp) { + for (line = y; line < (y+h); line++) { + memcpy (data + (line * linesize) + (x * bpp / 8), xenfb->pixels + xenfb->offset + (line * xenfb->row_stride) + (x * xenfb->depth / 8), w * xenfb->depth / 8); + } + } + else{ switch (xenfb->depth) { case 8: if (bpp == 16) { @@ -631,9 +637,17 @@ oops = 1; } break; + case 32: + if (bpp == 16) { + BLT(uint32_t, uint16_t, 8, 8, 8, 5, 6, 5); + } else { + oops = 1; + } + break; default: oops = 1; } + } } if (oops) /* should not happen */ xen_be_printf(&xenfb->c.xendev, 0, "%s: oops: convert %d -> %d bpp?\n", _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Oct-21 14:32 UTC
Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case
On Wed, 20 Oct 2010, Chun Yan Liu wrote:> In hw/xenfb.c, xenfb_guest_copy() function doesn''t handle xenfb->depth=32 case. When xenfb->depth=32, it will report error > and won''t copy data from the guest framebuffer region into QEMU''s displaysurface, thus when enter CTRL+ALT+2 to qemu > monitor console then CTRL+ALT+1 back to guest X window, the screen cannot be restored. > > This patch adds two things: > 1. when xenfb->depth equals destination bpp, do not need data translation, simply copy data from guest framebuffer into > QEMU''s displaysurface. > 2. when xenfb->depth and destination bpp differs, add processing for xenfb->depth=32 case. >The source of the confusion is that in the xenfb protocol depth actually means bpp (when bpp == 32, depth should be 24). It would be good to rename xenfb->depth to xenfb->bpp or at least add a comment in xenfb_configure_fb to explain that depth is actually bpp in that case.> Signed-off by Chunyan Liu <cyliu@novell.com> > ?? > diff -r e4f337bb97f7 tools/ioemu-qemu-xen/hw/xenfb.c > --- a/hw/xenfb.c??Wed Oct 20 19:39:28 2010 +0800 > +++ b/hw/xenfb.c??Wed Oct 20 21:42:37 2010 +0800 > @@ -612,6 +612,12 @@ > ???????? uint8_t *data = ds_get_data(xenfb->c.ds); > ?? > ???????? if (!is_buffer_shared(xenfb->c.ds->surface)) { > +?????????????? if (xenfb->depth == bpp) { > +?????????????????????????????? for (line = y; line < (y+h); line++) { > +?????????????????????????????????????????????? memcpy (data + (line * linesize) + (x * bpp / 8), xenfb->pixels + xenfb->offset + (line * > xenfb->row_stride) + (x * xenfb->depth / 8), w * xenfb->depth / 8); > +?????????????????????????????? } > +?????????????? } > +?????????????? else{This is not needed because if (xenfb->depth == bpp) the buffer should be shared. See xenfb_update and the call to qemu_create_displaysurface_from. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Chun Yan Liu
2010-Oct-25 06:26 UTC
答复: Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case
Dear Stefano, Thanks for you comments. Something to clarify: 1. (when bpp == 32, depth should be 24), I think not all places follow that rule. Change xenfb_configure_fb( ) only still has problem. In my testing, the initialization process: fb_connect -> xenfb_configure_fb(), it sets xenfb->depth to fb_page->depth, it''s 32. The graphic display after VM is started is fine. But if we change xenfb->depth to 24, the graphic display after VM is started is corrupted. 2. About the second comment, if (xenfb->depth == bpp), buffer should be shared. Actually, in our testing, do CTRL+ALT+2, then CTRL+ALT+1, in xenfb_guest_copy( ), xenfb->depth=32, bpp=32, buffer is NOT shared. If we do not add the if(xenfb->depth == bpp ) part, the screen can not be restored. Thanks, Chunyan>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 2010/10/21 22:32 >>>On Wed, 20 Oct 2010, Chun Yan Liu wrote:> In hw/xenfb.c, xenfb_guest_copy() function doesn''t handle xenfb->depth=32 case. When xenfb->depth=32, it will report error > and won''t copy data from the guest framebuffer region into QEMU''s displaysurface, thus when enter CTRL+ALT+2 to qemu > monitor console then CTRL+ALT+1 back to guest X window, the screen cannot be restored. > > This patch adds two things: > 1. when xenfb->depth equals destination bpp, do not need data translation, simply copy data from guest framebuffer into > QEMU''s displaysurface. > 2. when xenfb->depth and destination bpp differs, add processing for xenfb->depth=32 case. >The source of the confusion is that in the xenfb protocol depth actually means bpp (when bpp == 32, depth should be 24). It would be good to rename xenfb->depth to xenfb->bpp or at least add a comment in xenfb_configure_fb to explain that depth is actually bpp in that case.> Signed-off by Chunyan Liu <cyliu@novell.com> > ?? > diff -r e4f337bb97f7 tools/ioemu-qemu-xen/hw/xenfb.c > --- a/hw/xenfb.c??Wed Oct 20 19:39:28 2010 +0800 > +++ b/hw/xenfb.c??Wed Oct 20 21:42:37 2010 +0800 > @@ -612,6 +612,12 @@ > ???????? uint8_t *data = ds_get_data(xenfb->c.ds); > ?? > ???????? if (!is_buffer_shared(xenfb->c.ds->surface)) { > +?????????????? if (xenfb->depth == bpp) { > +?????????????????????????????? for (line = y; line < (y+h); line++) { > +?????????????????????????????????????????????? memcpy (data + (line * linesize) + (x * bpp / 8), xenfb->pixels + xenfb->offset + (line * > xenfb->row_stride) + (x * xenfb->depth / 8), w * xenfb->depth / 8); > +?????????????????????????????? } > +?????????????? } > +?????????????? else{This is not needed because if (xenfb->depth == bpp) the buffer should be shared. See xenfb_update and the call to qemu_create_displaysurface_from. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Oct-25 11:47 UTC
Re: 答复: Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case
On Mon, 25 Oct 2010, Chun Yan Liu wrote:> Dear Stefano, > ?? > Thanks for you comments. Something to clarify: > 1. (when bpp == 32, depth should be 24), I think not all places follow that rule.The fact that not all places follow the rule bpp == 32 -> depth == 24 is the source of the problem. In particular the name "depth" is used in place of "bpp" in many variables and struct fields.> Change xenfb_configure_fb( ) only still > has problem.I meant change the fifth argument from "depth" to "bpp" (because it is really bpp). Then it would make sense to change xenfb->depth to xenfb->bpp too for the same reason.> In my testing, the initialization process: fb_connect -> xenfb_configure_fb(), it sets xenfb->depth to fb_page->depth,?? > it''s 32. The graphic display after VM is started is fine. > But if we change xenfb->depth to 24, the graphic display after VM is started is corrupted.That would be a mistake, because xenfb->depth is actually storing bpp not depth. Yet another example of how broken the current situation is! The fix would not be changing xenfb->depth to 24, but renaming xenfb->depth to xenfb->bpp.> 2.?? About the second comment,??if (xenfb->depth == bpp), buffer should be shared. Actually, in our testing, do CTRL+ALT+2, > then??CTRL+ALT+1, in xenfb_guest_copy( ), xenfb->depth=32, bpp=32, buffer is??NOT shared. If we do not add the > if(xenfb->depth == bpp ) part, the screen can not be restored. > ??I think the reason for this particular bug would be that when you press CTR+ALT+2 and CTRL+ALT+1, console_select is called, that calls qemu_resize_displaysurface. qemu_resize_displaysurface calls defaultallocator_resize_displaysurface that creates another displaysurface without sharing the buffer. The code path that allows us to have a shared buffer with xenfb is the "if (xenfb->do_resize)" in xenfb_update but doesn''t get involved in this case. A possible solution would be to set do_resize in xenfb_update if the buffer is not shared and bpp == xenfb->depth. Another solution would be to create a displayallocator for xenfb. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel