Corrupted icons in Windows 95 with Voodoo 3

[Vort]

In cases, when I need to start Windows 95 with debug: action=ignore, voodoo=report options, icons corruption shows up sometimes in system tray. Images appear damaged or even swapped:

This is how the same icons should look when everything works fine:

After some searching, I figured out that this problem is related to BX_DEBUG(("launchArea write: value = 0x%08x", value)); line.
However, later it became clear that debug lines by themselves are not needed for bug reproduction. It is enough to add some delay to bx_banshee_c::blt_launch_area_write function, like so: volatile int z = 0; for (; z < 20000; z++) {}.

During Windows operation, it checks status register, whose contents are formed by register_r function.
This function performs some checks and sets various busy flags according to the state of GPU.
However, the problem is, during blt processing, there is a time, when FIFO is already empty, but blt.busy flag is not set yet, which produces incorrect value in status register.

I was able to hackfix this problem by adding one more check to register_r, but I don't how real hardware manages these flags and I can't say if this change is correct.

diff --git a/bochs/iodev/display/voodoo_func.h b/bochs/iodev/display/voodoo_func.h
index e8be0682e..5b6830375 100644
--- a/bochs/iodev/display/voodoo_func.h
+++ b/bochs/iodev/display/voodoo_func.h
@@ -3653,7 +3653,7 @@ Bit32u register_r(Bit32u offset)
       else
       {
         /* bit 10 is 2D busy */
-        if (v->banshee.blt.busy)
+        if (v->banshee.blt.busy || v->banshee.blt.lamem != NULL)
           result |= 3 << 9;
 
         /* bit 11 is cmd FIFO 0 busy */ 

Version: e7fb54f

[vruppert]

Does it help when you modify the code like this?

index 9da498f97..37a13970d 100644
--- a/bochs/iodev/display/banshee.cc
+++ b/bochs/iodev/display/banshee.cc
@@ -2152,8 +2152,6 @@ void bx_banshee_c::blt_execute()
             blt_host_to_screen_stretch();
           }
         }
-        delete [] BLT.lamem;
-        BLT.lamem = NULL;
       } else {
         BX_ERROR(("Host to screen blt: immediate execution not supported"));
       }
@@ -2302,6 +2300,10 @@ void bx_banshee_c::blt_complete()
     BLT.reg[blt_dstXY] &= 0xffff;
     BLT.reg[blt_dstXY] |= (BLT.dst_y << 16);
   }
+  if (BLT.lamem != NULL) {
+    delete [] BLT.lamem;
+    BLT.lamem = NULL;
+  }
   BLT.busy = 0;
 }
 

[Vort]

Does it help when you modify the code like this?

No.
If I understand correctly, corruption happens on guest level, because of incorrect status values returned by Bochs. Blts by themselves work fine.

[vruppert]

If your change from above helps, I guess there is a short time between setting busy to 0 and deleting BLT.lamem that causes this issue. With my change this gap doesn't exist. If the 2D busy bit is set/cleared incorrectly, we need to find out how it should be. Currently it is set to 0 after the 2D operation is done.

[Vort]

If the 2D busy bit is set/cleared incorrectly, we need to find out how it should be

It is hard to say which busy bit exactly is wrong, because all of them are combined at the end into SST_BUSY (BIT(9)) (and this is how it is made in Bochs as well):

I can rewrite my change like so and it will work fine as well:

diff --git a/bochs/iodev/display/voodoo_func.h b/bochs/iodev/display/voodoo_func.h
index 21b8a3444..c4577045e 100644
--- a/bochs/iodev/display/voodoo_func.h
+++ b/bochs/iodev/display/voodoo_func.h
@@ -3624,7 +3624,7 @@ Bit32u register_r(Bit32u offset)
         result |= 1 << 8;
 
       /* bit 9 is overall busy */
-      if (v->pci.op_pending)
+      if (v->pci.op_pending || (v->type >= VOODOO_BANSHEE && v->banshee.blt.lamem != NULL))
         result |= 1 << 9;
 
       if (v->type == VOODOO_2) { 

However, I think bit 10 (SST_GUI_BUSY / "2D busy") is what really (internally) causes problems here, because it is related to 2D operations. (upd: probably not, see messages below)

[Vort]

Currently it is set to 0 after the 2D operation is done.

I suspect problem appears not when it goes from 1 to 0, but when it goes from 0 to 1.
May it be that blt_launch_area_setup and blt_launch_area_write are treated as part of 2D operation in addition to blt_execute?
I don't know how BLTs are made in hardware, so I can't say how this mechanism should work.

[Vort]

Besides bit 10, difference may originate from bits 11 and 12 (SST_CMD0_BUSY and SST_CMD1_BUSY).
Documentation states that "Depth is the number of remaining unexecuted commands in off chip memory".
It can be read as (my interpretation) "depth 0 means all commands were executed".
H5\CSIM\CMDFIFO.C confirms what documentation says:

In Bochs, depth is updated right after data is read:

And right away CPU can sense it via status read:

Bochs/bochs/iodev/display/voodoo_func.h

Lines 3659 to 3661 in d192649

	/* bit 11 is cmd FIFO 0 busy */
	if (v->fbi.cmdfifo[0].enabled && v->fbi.cmdfifo[0].depth > 0)
	result |= 5 << 9;

[Vort]

This hack also allows to get rid of corruption:

diff --git a/bochs/iodev/display/voodoo_func.h b/bochs/iodev/display/voodoo_func.h
index 21b8a3444..eda96babf 100644
--- a/bochs/iodev/display/voodoo_func.h
+++ b/bochs/iodev/display/voodoo_func.h
@@ -3628,7 +3628,7 @@ Bit32u register_r(Bit32u offset)
         result |= 1 << 9;
 
       if (v->type == VOODOO_2) {
-        if (v->fbi.cmdfifo[0].enabled && v->fbi.cmdfifo[0].depth > 0)
+        if (v->fbi.cmdfifo[0].enabled && (v->fbi.cmdfifo[0].depth > 0 || v->fbi.cmdfifo[0].cmd_ready))
           result |= 7 << 7;
       }
       /* Banshee is different starting here */
@@ -3657,11 +3657,11 @@ Bit32u register_r(Bit32u offset)
           result |= 3 << 9;
 
         /* bit 11 is cmd FIFO 0 busy */
-        if (v->fbi.cmdfifo[0].enabled && v->fbi.cmdfifo[0].depth > 0)
+        if (v->fbi.cmdfifo[0].enabled && (v->fbi.cmdfifo[0].depth > 0 || v->fbi.cmdfifo[0].cmd_ready))
           result |= 5 << 9;
 
         /* bit 12 is cmd FIFO 1 busy */
-        if (v->fbi.cmdfifo[1].enabled && v->fbi.cmdfifo[1].depth > 0)
+        if (v->fbi.cmdfifo[1].enabled && (v->fbi.cmdfifo[1].depth > 0 || v->fbi.cmdfifo[1].cmd_ready))
           result |= 9 << 9;
       }
  

[vruppert]

I have applied your latest patch, since I think it's okay for now.

[Vort]

Ok, thanks.
In future, however, proper depth calculations should be done.