[Qemu-devel] [PATCH] Missing cirrus_invalidate_region() in cirrus_do_copy()

[Qemu-devel] [PATCH] Missing cirrus_invalidate_region() in cirrus_do_copy()

[Brian Kress]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

After doing a video to video copy, cirrus_do_copy() in cirrus_vga.c does 
a conditional
call to cirrus_invalidate_region() with an "if (!notify)" test.  However 
at this point the blt
has already been done so it seems like this call should be made 
unconditionally.  The patch
below fixes some display update problems when dragging windows under 
both X (fedora
10 guest) and a Windows XP guest.




[-- Attachment #2: patch.cirrus_vga_invalidate --]
[-- Type: text/plain, Size: 708 bytes --]

Signed-off-by: Brian Kress <kressb@moose.net>

Index: hw/cirrus_vga.c
===================================================================
--- hw/cirrus_vga.c	(revision 6626)
+++ hw/cirrus_vga.c	(working copy)
@@ -781,10 +781,9 @@
     /* we don't have to notify the display that this portion has
        changed since qemu_console_copy implies this */
 
-    if (!notify)
-	cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
-				 s->cirrus_blt_dstpitch, s->cirrus_blt_width,
-				 s->cirrus_blt_height);
+    cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
+				s->cirrus_blt_dstpitch, s->cirrus_blt_width,
+				s->cirrus_blt_height);
 }
 
 static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)

Re: [Qemu-devel] [PATCH] Missing cirrus_invalidate_region() in cirrus_do_copy()

[Alexander Graf]

On 16.02.2009, at 23:09, Brian Kress <kressb@moose.net> wrote:

> After doing a video to video copy, cirrus_do_copy() in cirrus_vga.c  
> does a conditional
> call to cirrus_invalidate_region() with an "if (!notify)" test.   
> However at this point the blt
> has already been done so it seems like this call should be made  
> unconditionally.  The patch
> below fixes some display update problems when dragging windows under  
> both X (fedora
> 10 guest) and a Windows XP guest.

Hm - I've run into this too, exposed the most using a gnome guest on  
vnc using copyrect.

Basically from what I understood, the conditional is

If display supports copyrect
   tell it to copy
Else
   draw yourself

And with this patch you're basically reverting to 'draw yourself'  
always, no?

I boiled things far enough to see that the blt addresses were  
completely off. The next idea on my list was to check if this is a  
Linux driver issue by running the same image on a real cirrus logic.

Alex


>
> Signed-off-by: Brian Kress <kressb@moose.net>
>
> Index: hw/cirrus_vga.c
> ===================================================================
> --- hw/cirrus_vga.c    (revision 6626)
> +++ hw/cirrus_vga.c    (working copy)
> @@ -781,10 +781,9 @@
>     /* we don't have to notify the display that this portion has
>        changed since qemu_console_copy implies this */
>
> -    if (!notify)
> -    cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
> -                 s->cirrus_blt_dstpitch, s->cirrus_blt_width,
> -                 s->cirrus_blt_height);
> +    cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
> +                s->cirrus_blt_dstpitch, s->cirrus_blt_width,
> +                s->cirrus_blt_height);
> }
>
> static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)

Re: [Qemu-devel] [PATCH] Missing cirrus_invalidate_region() in cirrus_do_copy()

[Brian Kress]

Alexander Graf wrote:
>
> On 16.02.2009, at 23:09, Brian Kress <kressb@moose.net> wrote:
>
>> After doing a video to video copy, cirrus_do_copy() in cirrus_vga.c 
>> does a conditional
>> call to cirrus_invalidate_region() with an "if (!notify)" test.  
>> However at this point the blt
>> has already been done so it seems like this call should be made 
>> unconditionally.  The patch
>> below fixes some display update problems when dragging windows under 
>> both X (fedora
>> 10 guest) and a Windows XP guest.
>
> Hm - I've run into this too, exposed the most using a gnome guest on 
> vnc using copyrect.
>
> Basically from what I understood, the conditional is
>
> If display supports copyrect
>   tell it to copy
> Else
>   draw yourself
    That logic is done in vnc.c and is still being done.  The vnc
viewer is still being sent the copyrect, it's just getting overwritten
on the next update because the display surface is not updated.  The
data in s->ds needs to be updated, either by vnc_copy() or by
vga_hw_update() (which is what this patch does).  An alternate way to
fix this would be to do a memmove from the source rectangle to the
destination rectangle in vnc_copy(), which I think is what previous
versions of the copyrect code did, but it got taken out for some
reason, which is what triggered this bug.
>
> And with this patch you're basically reverting to 'draw yourself' 
> always, no?
>
> I boiled things far enough to see that the blt addresses were 
> completely off. The next idea on my list was to check if this is a 
> Linux driver issue by running the same image on a real cirrus logic.
>
> Alex
    The blt addresses being off seemed to be a separate issue caused by
assuming pitch == width * depth.  I already fixed that in another patch,
which has already been applied.  That one did show up only on Linux,
the one this patch is targeted at affects both.

Re: [Qemu-devel] [PATCH] Missing cirrus_invalidate_region() in cirrus_do_copy()

[Anthony Liguori]

Brian Kress wrote:
> After doing a video to video copy, cirrus_do_copy() in cirrus_vga.c 
> does a conditional
> call to cirrus_invalidate_region() with an "if (!notify)" test.  
> However at this point the blt
> has already been done so it seems like this call should be made 
> unconditionally.  The patch
> below fixes some display update problems when dragging windows under 
> both X (fedora
> 10 guest) and a Windows XP guest.

Applied.  Thanks.

Regards,

Anthony Liguori
>
>