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

Well, about depth and bpp,  I got it. There IS confusion in varible usage. In
some places, for example, qemu_default_pixelformat(), it handles pf.depth = bpp
== 32 ? 24 : bpp; but other places, like xenfb_configure_fb, xenfb->depth =
depth ( which is actually bpp). But expect for a naming confusion, it
didn''t affect the result. As you mentioned, we can add a comment, or
change the code to make it clear.

And about the mentioned particular bug, first, why xenfb_guest_copy only handles
xenfb->depth=8 and 24 cases, because it assumes in xenfb->depth=16 or 32
cases, buffer is shared, like in xenfb_update ==>
qemu_create_displaysurface_from() does. But that''s not always the case,
as in the mentioned bug, it didn''t enter the xenfb_update do_resize
hunk, buffer is not shared.
To solve that problem, we can modify the mentioned case, to make it enter the
xenfb_update do_resize hunk, then xenfb_guest_copy need not change. Patch is:

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 27 00:33:40 2010 +0800
@@ -784,6 +784,7 @@
 static void xenfb_invalidate(void *opaque)
 {
     struct XenFB *xenfb = opaque;
+    xenfb->do_resize=1;
     xenfb->up_fullscreen = 1;
 }
 
or, change xenfb_guest_copy( ) as the original patch does, let it hanlde all
cases: xenfb->depth == bpp case, and xenfb->depth != bpp case.

In fact, even with the above patch, I think it''s better to change
xenfb_guest_copy and let it handle all cases. Perhaps in other special cases,
there is similar problem.

Thanks,
Chunyan
>>> Stefano Stabellini  10/25/10 7:48 AM >>>
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 mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, 26 Oct 2010, Chun Yan Liu wrote:
> Well, about depth and bpp, I got it. There IS confusion in varible usage.
In some places, for example,
> qemu_default_pixelformat(), it handles pf.depth = bpp == 32 ? 24 : bpp; but
other places, like xenfb_configure_fb,
> xenfb->depth = depth ( which is actually bpp). But expect for a naming
confusion, it didn''t affect the result. As you
> mentioned, we can add a comment, or change the code to make it clear.
> 
> And about the mentioned particular bug, first, why xenfb_guest_copy only
handles xenfb->depth=8 and 24 cases, because it
> assumes in xenfb->depth=16 or 32 cases, buffer is shared, like in
xenfb_update ==> qemu_create_displaysurface_from() does.
> But that''s not always the case, as in the mentioned bug, it
didn''t enter the xenfb_update do_resize hunk, buffer is not
> shared.
> To solve that problem, we can modify the mentioned case, to make it enter
the xenfb_update do_resize hunk, then
> xenfb_guest_copy need not change. Patch is:
> 
> 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 27 00:33:40 2010 +0800
> @@ -784,6 +784,7 @@
> ??static void xenfb_invalidate(void *opaque)
> ??{
> ???????? struct XenFB *xenfb = opaque;
> +?????? xenfb->do_resize=1;
> ???????? xenfb->up_fullscreen = 1;
> ??}
> ??
this patch is OK
> or, change xenfb_guest_copy( ) as the original patch does, let it hanlde
all cases: xenfb->depth == bpp case, and
> xenfb->depth != bpp case.
> 
> In fact, even with the above patch, I think it''s better to change
xenfb_guest_copy and let it handle all cases. Perhaps in
> other special cases, there is similar problem.
> 
all right it might a good idea, but if you want to do that please use
the switch statement already there as opposed to adding an another if
statement outside.

Could you please resubmit a patch with both changes and a signed-off-by
line?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel