thr3ads.net - Xen devel - [Xen-devel][PATCH]qemu-xen: let xenfb_guest

If this information is useful, please help other people find it:
Share via:

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

head link

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

head link

答复： 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

head link

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

Xen devel - Oct 2010 - [PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case

[Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case

Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case

答复： Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case

Re: 答复： Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case