[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] cirrus: handle negative pitch in cirrus_invalid
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] cirrus: handle negative pitch in cirrus_invalidate_region() |
Date: |
Wed, 25 Jan 2017 13:35:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 |
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 <address@hidden>
> ---
> 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