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.bumil...@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
