> On January 25, 2017 at 1:35 PM Laszlo Ersek <ler...@redhat.com> wrote:
>
>
> 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.)
Right, this again.
> Therefore the decrement should be (bytesperline - bytesperpixel), IMO.
At this point we don't operate in a pixel byte group basis though, and
as far as I can see the backward blit functions are always byte-based
operations, so I think `bytesperpixel` can just be 1 here?
> Regarding assert(off_cur_end >= off_cur): not a bad idea, IMO.
I'll include it in v2.
