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