From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34982 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OCWcR-0006zh-Dq for qemu-devel@nongnu.org; Thu, 13 May 2010 07:30:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OCSEf-0003VV-Pi for qemu-devel@nongnu.org; Thu, 13 May 2010 02:51:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6275) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OCSEf-0003V1-0O for qemu-devel@nongnu.org; Thu, 13 May 2010 02:49:33 -0400 Message-ID: <4BEBA0ED.9010009@redhat.com> Date: Thu, 13 May 2010 09:49:17 +0300 From: Avi Kivity MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus References: <4BE32178.2090103@msgid.tls.msk.ru> <4BE7B8C1.9060807@redhat.com> <4BE7C0A5.3090909@redhat.com> <4BEAA0CC.4090906@redhat.com> <4BEABABC.6080305@redhat.com> <4BEAD232.2040700@redhat.com> <20100512170702.GE19314@shareable.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Brian Kress , Michael Tokarev , qemu-devel , KVM list On 05/12/2010 09:11 PM, Stefano Stabellini wrote: > On Wed, 12 May 2010, Jamie Lokier wrote: > >> Stefano Stabellini wrote: >> >>> On Wed, 12 May 2010, Avi Kivity wrote: >>> >>>> It's useful if you have a one-line horizontal pattern you want to >>>> propagate all over. >>>> >>> >>> It might be useful all right, but it is not entirely clear what the >>> hardware should do in this situation from the documentation we have, and >>> certainly the current state of the cirrus emulation code doesn't help. >>> >> It's quite a reasonable thing for hardware to do, even if not documented. >> It would be surprising if the hardware didn't copy the one-line pattern. >> > > All right then, you convinced me :) > > This is my proposed solution, however it is untested with Windows NT. > > > Signed-off-by: Stefano Stabellini > > --- > > > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c > index 9f61a01..a7f0d3c 100644 > --- a/hw/cirrus_vga.c > +++ b/hw/cirrus_vga.c > @@ -676,15 +676,17 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) > int sx, sy; > int dx, dy; > int width, height; > + uint32_t start_addr, line_offset, line_compare; > int depth; > int notify = 0; > > depth = s->vga.get_bpp(&s->vga) / 8; > s->vga.get_resolution(&s->vga,&width,&height); > + s->vga.get_offsets(&s->vga,&line_offset,&start_addr,&line_compare); > > /* extra x, y */ > - sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth; > - sy = (src / ABS(s->cirrus_blt_srcpitch)); > + sx = (src % line_offset) / depth; > + sy = (src / line_offset); > Does anything prevent the guest from programming the CRTC display pitch to 0? > dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth; > dy = (dst / ABS(s->cirrus_blt_dstpitch)); > > @@ -725,18 +727,23 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) > s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch, > s->cirrus_blt_width, s->cirrus_blt_height); > > - if (notify) > - qemu_console_copy(s->vga.ds, > - sx, sy, dx, dy, > - s->cirrus_blt_width / depth, > - s->cirrus_blt_height); > - > - /* we don't have to notify the display that this portion has > - changed since qemu_console_copy implies this */ > - > - cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, > - s->cirrus_blt_dstpitch, s->cirrus_blt_width, > - s->cirrus_blt_height); > + if (ABS(s->cirrus_blt_dstpitch) != line_offset || > + ABS(s->cirrus_blt_srcpitch) != line_offset) { > + /* this is not going to happen very often */ > + vga_hw_invalidate(); > I think we need to consider only dstpitch for a full invalidate. We might be copying an offscreen bitmap into the screen, and srcpitch is likely to be the bitmap width instead of the screen pitch. > + } else { > + if (notify) > + /* we don't have to notify the display that this portion has > + changed since qemu_console_copy implies this */ > + qemu_console_copy(s->vga.ds, > + sx, sy, dx, dy, > + s->cirrus_blt_width / depth, > + s->cirrus_blt_height); > + else > + cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, > + s->cirrus_blt_dstpitch, s->cirrus_blt_width, > + s->cirrus_blt_height); > + } > } > > static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s) > diff --git a/hw/cirrus_vga_rop.h b/hw/cirrus_vga_rop.h > index 39a7b72..80f135b 100644 > --- a/hw/cirrus_vga_rop.h > +++ b/hw/cirrus_vga_rop.h > @@ -32,10 +32,10 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s, > dstpitch -= bltwidth; > srcpitch -= bltwidth; > > - if (dstpitch< 0 || srcpitch< 0) { > - /* is 0 valid? srcpitch == 0 could be useful */ > + if (dstpitch< 0) > return; > - } > + if (srcpitch< 0) > + srcpitch = 0; > Why? -- Do not meddle in the internals of kernels, for they are subtle and quick to panic.