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

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

[Wolfgang Bumiller]

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);
-- 
2.1.4

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

[Laszlo Ersek]

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

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

[Wolfgang Bumiller]

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

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

[Laszlo Ersek]

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