WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: 答复: Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle de

To: <Stefano.Stabellini@xxxxxxxxxxxxx>
Subject: Re: 答复: Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case
From: "Chun Yan Liu" <cyliu@xxxxxxxxxx>
Date: Tue, 26 Oct 2010 02:45:53 -0600
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Tue, 26 Oct 2010 01:46:52 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>