From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tokarev Subject: Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus Date: Sat, 29 May 2010 00:51:28 +0400 Message-ID: <4C002CD0.4060704@msgid.tls.msk.ru> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Jamie Lokier , Avi Kivity , Brian Kress , qemu-devel , KVM list To: Stefano Stabellini Return-path: Received: from isrv.corpit.ru ([81.13.33.159]:35307 "EHLO isrv.corpit.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753151Ab0E1Uvb (ORCPT ); Fri, 28 May 2010 16:51:31 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 12.05.2010 22:11, 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 So.. what's the status of this, after all? :) As far as I can tell, it has been agreed that the patch is good, and verified that it fixes the problem. Should we just throw it away and start from scratch, or what? :) Thanks! > 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); > 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(); > + } 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; > > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > @@ -57,6 +57,12 @@ glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s, > int x,y; > dstpitch += bltwidth; > srcpitch += bltwidth; > + > + if (dstpitch> 0) > + return; > + if (srcpitch> 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > ROP_OP(*dst, *src); > @@ -78,6 +84,12 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s, > uint8_t p; > dstpitch -= bltwidth; > srcpitch -= bltwidth; > + > + if (dstpitch< 0) > + return; > + if (srcpitch< 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > p = *dst; > @@ -101,6 +113,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s, > uint8_t p; > dstpitch += bltwidth; > srcpitch += bltwidth; > + > + if (dstpitch> 0) > + return; > + if (srcpitch> 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > p = *dst; > @@ -124,6 +142,12 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s, > uint8_t p1, p2; > dstpitch -= bltwidth; > srcpitch -= bltwidth; > + > + if (dstpitch< 0) > + return; > + if (srcpitch< 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x+=2) { > p1 = *dst; > @@ -152,6 +176,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s, > uint8_t p1, p2; > dstpitch += bltwidth; > srcpitch += bltwidth; > + > + if (dstpitch> 0) > + return; > + if (srcpitch> 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x+=2) { > p1 = *(dst-1); > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53708 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OI6Wl-0005RR-K2 for qemu-devel@nongnu.org; Fri, 28 May 2010 16:51:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OI6Wk-0001gV-67 for qemu-devel@nongnu.org; Fri, 28 May 2010 16:51:35 -0400 Received: from isrv.corpit.ru ([81.13.33.159]:36506) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OI6Wj-0001fS-Qc for qemu-devel@nongnu.org; Fri, 28 May 2010 16:51:34 -0400 Message-ID: <4C002CD0.4060704@msgid.tls.msk.ru> Date: Sat, 29 May 2010 00:51:28 +0400 From: Michael Tokarev 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=ISO-8859-1; 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 , Avi Kivity , KVM list , qemu-devel 12.05.2010 22:11, 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 So.. what's the status of this, after all? :) As far as I can tell, it has been agreed that the patch is good, and verified that it fixes the problem. Should we just throw it away and start from scratch, or what? :) Thanks! > 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); > 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(); > + } 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; > > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > @@ -57,6 +57,12 @@ glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s, > int x,y; > dstpitch += bltwidth; > srcpitch += bltwidth; > + > + if (dstpitch> 0) > + return; > + if (srcpitch> 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > ROP_OP(*dst, *src); > @@ -78,6 +84,12 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s, > uint8_t p; > dstpitch -= bltwidth; > srcpitch -= bltwidth; > + > + if (dstpitch< 0) > + return; > + if (srcpitch< 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > p = *dst; > @@ -101,6 +113,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s, > uint8_t p; > dstpitch += bltwidth; > srcpitch += bltwidth; > + > + if (dstpitch> 0) > + return; > + if (srcpitch> 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x++) { > p = *dst; > @@ -124,6 +142,12 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s, > uint8_t p1, p2; > dstpitch -= bltwidth; > srcpitch -= bltwidth; > + > + if (dstpitch< 0) > + return; > + if (srcpitch< 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x+=2) { > p1 = *dst; > @@ -152,6 +176,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s, > uint8_t p1, p2; > dstpitch += bltwidth; > srcpitch += bltwidth; > + > + if (dstpitch> 0) > + return; > + if (srcpitch> 0) > + srcpitch = 0; > + > for (y = 0; y< bltheight; y++) { > for (x = 0; x< bltwidth; x+=2) { > p1 = *(dst-1); > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html