| Message ID | 1485346353-23814-1-git-send-email-w.bumiller@proxmox.com |
|---|---|
| State | New |
| Headers | show |
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
> On January 25, 2017 at 1:35 PM Laszlo Ersek <lersek@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.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.) 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.
On 01/25/17 14:22, Wolfgang Bumiller wrote: > >> On January 25, 2017 at 1:35 PM Laszlo Ersek <lersek@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.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.) > > 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? ENOCLUE :/ Perhaps Gerd can answer? >> Regarding assert(off_cur_end >= off_cur): not a bad idea, IMO. > > I'll include it in v2. > >
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);
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(+)