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

[Avi Kivity]

On 05/12/2010 06:57 PM, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Avi Kivity wrote:
>    
> > > 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 see: you want to support the case when abs(src_blt_pitch) != display_pitch,
> while I though it is some sort of error.
>    

When blting withinh the screen, it is an error from the point of view of 
writing a sane driver.  My guess is that it's a bug in the NT driver.

It's not an error from the point of view of the hardware, or when blting 
offscreen bitmaps (which can have different geomeries than the display).

> Considering that src_blt_pitch can even be 0 (as in the bug report), we
> would still need to calculate sx and sy using display_pitch instead, so
> the only place in which it would make a difference is when src_blt_pitch
> is passed as an argument to cirrus_rop.
>    

cirrus_rop() and its arguments don't need to change.  They're correctly 
using the blt pitch.

My point is: x[xy] and d[xy] are only valid if both source and 
destination overlap the display, and if both src_pitch and dst_pitch are 
absequal to the display pitch.  When they aren't valid, there's no point 
in calculating them or using them in anything.  The raster operation is 
still valid though.


> I guess even a src blt pitch of 0 could be useful there, however in
> practice I think the only rop function that was written with this case in
> mind has:
>
> dstpitch -= bltwidth;
> srcpitch -= bltwidth;
>
> if (dstpitch<  0 || srcpitch<  0) {
>      /* is 0 valid? srcpitch == 0 could be useful */
>      return;
> }
>
> at the beginning and the others probably just don't deal with the case
> with possibly buggy consequences.
> Also the documentation I have states:
>
> 3CEh index 26h W(R/W):  BLT Source Pitch                              (5426 +)
> bit 0-11  (5426-28) Number of bytes in a scanline at the source.
>      0-12  (5429 +)  do
>
> if the source BLT is supposed to be the number of bytes in a scanline at
> the source, then 0 is not a correct value for it.
>    

It's useful if you have a one-line horizontal pattern you want to 
propagate all over.

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