On Thu, 13 May 2010, Avi Kivity wrote:
> > /* 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?
>
Nope, I'll use line_offset there too to prevent possible divisions by 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.
>
Agreed.
>
> > + } 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?
I wouldn't want an operation that is supposed to be a forward copy
to become a backward copy instead.
With the way the code is currently written in cirrus_vga it shouldn't be
possible, but it might be a good idea to have the checks anyway.
Actually the limit I wrote is too strict, I'll fix it.
---
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 9f61a01..81c443b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -676,17 +676,19 @@ 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));
- dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
- dy = (dst / ABS(s->cirrus_blt_dstpitch));
+ sx = (src % line_offset) / depth;
+ sy = (src / line_offset);
+ dx = (dst % line_offset) / depth;
+ dy = (dst / line_offset);
/* normalize width */
w /= depth;
@@ -725,18 +727,22 @@ 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) {
+ /* this is not going to happen very often */
+ vga_hw_invalidate();
+ } else {
+ if (ABS(s->cirrus_blt_srcpitch) == line_offset && 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..3d41d41 100644
--- a/hw/cirrus_vga_rop.h
+++ b/hw/cirrus_vga_rop.h
@@ -29,13 +29,11 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
int bltwidth,int bltheight)
{
int x,y;
- dstpitch -= bltwidth;
- srcpitch -= bltwidth;
- if (dstpitch < 0 || srcpitch < 0) {
- /* is 0 valid? srcpitch == 0 could be useful */
+ if (dstpitch < 0 || srcpitch < 0)
return;
- }
+ dstpitch -= bltwidth;
+ srcpitch -= bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
@@ -55,6 +53,9 @@ glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
int bltwidth,int bltheight)
{
int x,y;
+
+ if (dstpitch > 0 || srcpitch > 0)
+ return;
dstpitch += bltwidth;
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
@@ -76,6 +77,9 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
{
int x,y;
uint8_t p;
+
+ if (dstpitch < 0 || srcpitch < 0)
+ return;
dstpitch -= bltwidth;
srcpitch -= bltwidth;
for (y = 0; y < bltheight; y++) {
@@ -99,6 +103,9 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
{
int x,y;
uint8_t p;
+
+ if (dstpitch > 0 || srcpitch > 0)
+ return;
dstpitch += bltwidth;
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
@@ -122,6 +129,9 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
{
int x,y;
uint8_t p1, p2;
+
+ if (dstpitch < 0 || srcpitch < 0)
+ return;
dstpitch -= bltwidth;
srcpitch -= bltwidth;
for (y = 0; y < bltheight; y++) {
@@ -150,6 +160,9 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
{
int x,y;
uint8_t p1, p2;
+
+ if (dstpitch > 0 || srcpitch > 0)
+ return;
dstpitch += bltwidth;
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
