Re: [Qemu-devel] [PATCH] cirrus: handle negative pitch in cirrus_invalidate_region()

[Laszlo Ersek <lersek@redhat.com>]

On 01/25/17 13:12, Wolfgang Bumiller wrote:
> cirrus_invalidate_region() calls memory_region_set_dirty()
> on a per-line basis, always ranging from off_begin to
> off_begin+bytesperline. With a negative pitch off_begin
> marks the top most used address and thus we need to do an
> initial shift backwards by bytesperline for negative
> pitches of backward blits, otherwise the first iteration
> covers the line going from the start offset forwards instead
> of backwards.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> I bumped the patch context to 4 lines to get it to include the
> memory_region_set_dirty() call which takes an unsigned length (`hwaddr
> size`) because I'm also not too happy about the masking going on there
> since it means off_cur_end may be less than off_cur, but if the range
> checks are finnaly correct I don't think this case could happen
> anymore? (A check shouldn't hurt though, or maybe an assert()?)
> 
>  hw/display/cirrus_vga.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index b1a0773..af61981 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -669,8 +669,12 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
>      int y;
>      int off_cur;
>      int off_cur_end;
>  
> +    if (off_pitch < 0) {
> +        off_begin -= bytesperline;
> +    }
> +
>      for (y = 0; y < lines; y++) {
>  	off_cur = off_begin;
>  	off_cur_end = (off_cur + bytesperline) & s->cirrus_addr_mask;
>          memory_region_set_dirty(&s->vga.vram, off_cur, off_cur_end - off_cur);
> 

I think the adjustment is off by one. The loop body inherently considers
"off_cur" inclusive, and "off_cur_end" exclusive.

When "off_pitch" is negative, and the initial "off_begin" value is an
"inclusive end", then by subtracting a full "bytesperline", you just go
to the "inclusive end" of the previous line. Whereas, you should go to
the address right above it, to the "inclusive start" of the *same* line.
(From bottom right to bottom left corner.)

Therefore the decrement should be (bytesperline - bytesperpixel), IMO.

Regarding assert(off_cur_end >= off_cur): not a bad idea, IMO.

Thanks
Laszlo