qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus


From: Avi Kivity
Subject: Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus
Date: Thu, 13 May 2010 09:49:17 +0300
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc12 Thunderbird/3.0.4

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<address@hidden>

---



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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]