[Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

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.