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 17:27:08 +0300 Message-ID: <4BEABABC.6080305@redhat.com> References: <4BE32178.2090103@msgid.tls.msk.ru> <4BE7B8C1.9060807@redhat.com> <4BE7C0A5.3090909@redhat.com> <4BEAA0CC.4090906@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]:54142 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751805Ab0ELO13 (ORCPT ); Wed, 12 May 2010 10:27:29 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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.