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 <stefano.stabellini@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
