| Message ID | alpine.DEB.2.00.1005121855230.11380@kaball-desktop |
|---|---|
| State | New |
| Headers | show |
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. Well. At least it does not crash anymore. With this patch applied, when hitting "Test" (of a new video mode) button on WindowsNT, the guest window gets resized to correct size, but is painted with yellow and nothing happens. The CPU enters 100%, and on the kvm console the following messages are displayed: BUG: kvm_dirty_pages_log_enable_slot: invalid parameters BUG: kvm_dirty_pages_log_enable_slot: invalid parameters That's not new, it sometimes displays that shit on attempt to migrate too, as I mentioned in another thread. Thanks! /mjt > 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; [...]
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<stefano.stabellini@eu.citrix.com> > > --- > > > > 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?
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<stefano.stabellini@eu.citrix.com> 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
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<stefano.stabellini@eu.citrix.com> > > --- > > > > 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)); > dx/dy also need to be calculated according to line_offset. I think the rules are: if src_byte_range in screen and dst_byte_range in screen and srcpitch == dstpitch == line_offset: can use qemu_console_copy elif dst_byte_range overlaps screen: if dstpitch == line_offset: invalidate rectangle else: invalidate full screen
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);