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/
Home Products Support Community News


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

To: Chun Yan Liu <cyliu@xxxxxxxxxx>
Subject: Re: 答复: Re: [Xen-devel][PATCH]qemu-xen: let xenfb_guest_copy() handle depth=32 case
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Wed, 27 Oct 2010 12:23:38 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Wed, 27 Oct 2010 04:25:17 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4CC72FF10200006600019665@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
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>
References: <4CC72FF10200006600019665@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
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

Xen-devel mailing list