Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity <avi@redhat.com>]

On 05/12/2010 04:45 PM, Stefano Stabellini wrote:
>
>    
>> Note it's just during mode changes.  During normal operation I'm sure
>> the pitches are equal.
>>
>>      
> The source blt pitch as set by the driver is always equal to the display
> pitch (apart from the case reported above).
> However cirrus_blt_srcpitch is not always equal to the display pitch
> because of CIRRUS_BLTMODE_BACKWARDS: cirrus_blt_srcpitch can also be
> negative and equal to -display_pitch.
>    

Yes.

> I suggest to start using the display pitch (with the proper sign)
> instead of cirrus_blt_srcpitch in cirrus_do_copy at least when
> cirrus_blt_srcpitch doesn't have a proper value.
>    

Why switch from one bug to the other?

It's perfectly possible to take into account both values.  Of course 
when abs(blt_pitch) != display pitch we can't use console_copy, but so what.

>> I think the code should be something like
>>
>>       if bitblt destination intersects display memory:
>>           if bitblt pitch == display pitch
>>              use console_copy
>>          else
>>              invalidate entire display
>>
>>      
> I think the following should be enough:
>
> if bitblt destination intersects display memory:
>      qemu_console_copy
> else
>      invalidate region
>
> why do we need if bitblt pitch == display pitch or to invalidate
> everything?
>    

Because the region is not a rectangle anymore.  We could compute exactly 
what needs to be invalidated, but since it will never happen, there's no 
point in optimizing it.

>>>> 31c05501c says this breaks bitblt, but I can't see why this is true.
>>>> The copy should update the display.  This is probably due to a
>>>> miscalculation of the affected region, and now we have two invalidates
>>>> instead of one, reducing performance.
>>>>
>>>>
>>>>          
>>> I agree with you: qemu_console_copy does imply that the copied portion
>>> of the screen changed, so there is no reason to invalidate it again if
>>> qemu_console_copy is called.
>>>
>>>        
>> Well, we can't just revert 31c05501c.  There's probably another bug
>> involved.
>>
>>      
> Sure, we have to fix the other one first :)
>    

And find it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.