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

[Laszlo Ersek <lersek@redhat.com>]

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