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

[Wolfgang Bumiller]

> On January 25, 2017 at 1:35 PM Laszlo Ersek <address@hidden> 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 <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.)

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.