From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: Another SIGFPE in display code, now in cirrus Date: Wed, 12 May 2010 19:07:14 +0300 Message-ID: <4BEAD232.2040700@redhat.com> References: <4BE32178.2090103@msgid.tls.msk.ru> <4BE7B8C1.9060807@redhat.com> <4BE7C0A5.3090909@redhat.com> <4BEAA0CC.4090906@redhat.com> <4BEABABC.6080305@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Michael Tokarev , KVM list , qemu-devel , Brian Kress To: Stefano Stabellini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39405 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755551Ab0ELQH0 (ORCPT ); Wed, 12 May 2010 12:07:26 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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.