vruppert
commented
10 months ago I'm not familiar with 3D stuff and I don't know how the "count_holes" stuff really works. The "cmdBump" stuff needs to be reviewed.
Open Vort opened 10 months ago
vruppert
commented
10 months ago I'm not familiar with 3D stuff and I don't know how the "count_holes" stuff really works. The "cmdBump" stuff needs to be reviewed.
Vort
commented
10 months ago I'm not familiar with 3D stuff and I don't know how the "count_holes" stuff really works.
If I understand correctly, f->depth++; should be executed no matter if counting is enabled or not.
Otherwise there is no way to start FIFO commands execution.
And f->depth++; was present in else branch before 5698c6974411ab9c7878773513788d5531b94ccf (lines 2453..2455).
Do not know if it is enough to just put it back.
Also this thing is not much related to 3D, just some specifics of command processing.
Vort
commented
10 months ago Here is another attempt, with more thinking this time.
f->holes, last 2 of them change f->holes.f->count_holes is disabled, then f->holes should always be zero, so last two branches should be disabled.BX_ERROR should trigger.My logic may be wrong, but:
f->count_holes = true case.f->count_holes = false is broken anyway, so patch should not make situation worse.diff --git a/bochs/iodev/display/banshee.cc b/bochs/iodev/display/banshee.cc
index a6c4f2d6c..cd84cac23 100644
--- a/bochs/iodev/display/banshee.cc
+++ b/bochs/iodev/display/banshee.cc
@@ -1088,7 +1088,9 @@ void bx_banshee_c::agp_reg_write(Bit8u reg, Bit32u value)
case cmdBump0:
case cmdBump1:
if (value > 0) {
- BX_ERROR(("cmdBump%d not implemented (value = 0x%04x)", fifo_idx, (Bit16u)value));
+ BX_LOCK(cmdfifo_mutex);
+ v->fbi.cmdfifo[fifo_idx].amin += value * 4;
+ BX_UNLOCK(cmdfifo_mutex);
}
break;
case cmdRdPtrL0:
diff --git a/bochs/iodev/display/voodoo_func.h b/bochs/iodev/display/voodoo_func.h
index 768765f9a..e92183eb4 100644
--- a/bochs/iodev/display/voodoo_func.h
+++ b/bochs/iodev/display/voodoo_func.h
@@ -2886,21 +2886,20 @@ void cmdfifo_w(cmdfifo_info *f, Bit32u fbi_offset, Bit32u data)
{
BX_LOCK(cmdfifo_mutex);
*(Bit32u*)(&v->fbi.ram[fbi_offset]) = data;
- /* count holes? */
- if (f->count_holes) {
- if ((f->holes == 0) && (fbi_offset == (f->amin + 4))) {
- /* in-order, no holes */
- f->amin = f->amax = fbi_offset;
- f->depth++;
- } else if (fbi_offset < f->amin) {
- /* out-of-order, below the minimum */
- if (f->holes != 0) {
- BX_ERROR(("Unexpected CMDFIFO: AMin=0x%08x AMax=0x%08x Holes=%d WroteTo:0x%08x RdPtr:0x%08x",
- f->amin, f->amax, f->holes, fbi_offset, f->rdptr));
- }
- f->amin = f->amax = fbi_offset;
- f->depth++;
- } else if (fbi_offset < f->amax) {
+ if ((f->holes == 0) && (fbi_offset == (f->amin + 4))) {
+ /* in-order, no holes */
+ f->amin = f->amax = fbi_offset;
+ f->depth++;
+ } else if (fbi_offset < f->amin) {
+ /* out-of-order, below the minimum */
+ if (f->holes != 0) {
+ BX_ERROR(("Unexpected CMDFIFO: AMin=0x%08x AMax=0x%08x Holes=%d WroteTo:0x%08x RdPtr:0x%08x",
+ f->amin, f->amax, f->holes, fbi_offset, f->rdptr));
+ }
+ f->amin = f->amax = fbi_offset;
+ f->depth++;
+ } else if (f->count_holes) {
+ if (fbi_offset < f->amax) {
/* out-of-order, but within the min-max range */
f->holes--;
if (f->holes == 0) {
@@ -2913,6 +2912,10 @@ void cmdfifo_w(cmdfifo_info *f, Bit32u fbi_offset, Bit32u data)
f->amax = fbi_offset;
}
}
+ else {
+ BX_ERROR(("Unexpected CMDFIFO: AMin=0x%08x AMax=0x%08x WroteTo:0x%08x RdPtr:0x%08x",
+ f->amin, f->amax, fbi_offset, f->rdptr));
+ }
if (f->depth_needed == BX_MAX_BIT32U) {
f->depth_needed = cmdfifo_calc_depth_needed(f);
} There is one more problem with these drivers: Fast resolution change often leads to crash of either Windows XP or Bochs. Most likely, my patch above should be refined further.
Here is test program (modeset.zip) which make 5 resolution changes between 800x600@16 and 1024x768@32:
#define WIN32_LEAN_AND_MEAN
#include <windows.h>
int ChangeResolution(int width, int height, int bpp)
{
DEVMODE dm;
memset(&dm, 0, sizeof(dm));
dm.dmSize = sizeof(dm);
dm.dmFields = DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT;
dm.dmPelsWidth = width;
dm.dmPelsHeight = height;
dm.dmBitsPerPel = bpp;
return ChangeDisplaySettings(&dm, 0);
}
int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int)
{
for (int i = 0; i < 5; i++)
{
ChangeResolution(800, 600, 16);
Sleep(100);
ChangeResolution(1024, 768, 32);
Sleep(100);
}
return 0;
}Yeah, my previous code wasn't right.
cmdBumps make no sense with automatic updates of amin.
So I think disabled count_holes means that amin should be updated by driver, not by hardware.
There is a hint in documentation about it: "cmdBump0 defines the number of words to increment the amin pointer by, when managed by software".
Here is my new attempt at fixing problems with this driver:
diff --git a/bochs/iodev/display/banshee.cc b/bochs/iodev/display/banshee.cc
index a91ed4c00..061250260 100644
--- a/bochs/iodev/display/banshee.cc
+++ b/bochs/iodev/display/banshee.cc
@@ -1108,7 +1108,9 @@ void bx_banshee_c::agp_reg_write(Bit8u reg, Bit32u value)
case cmdBump0:
case cmdBump1:
if (value > 0) {
- BX_ERROR(("cmdBump%d not implemented (value = 0x%04x)", fifo_idx, (Bit16u)value));
+ BX_LOCK(cmdfifo_mutex);
+ v->fbi.cmdfifo[fifo_idx].amin += value * 4;
+ BX_UNLOCK(cmdfifo_mutex);
}
break;
case cmdRdPtrL0:
diff --git a/bochs/iodev/display/voodoo_func.h b/bochs/iodev/display/voodoo_func.h
index 141618903..b0fbb514d 100644
--- a/bochs/iodev/display/voodoo_func.h
+++ b/bochs/iodev/display/voodoo_func.h
@@ -2915,6 +2915,10 @@ void cmdfifo_w(cmdfifo_info *f, Bit32u fbi_offset, Bit32u data)
f->amax = fbi_offset;
}
}
+ else {
+ f->amax = fbi_offset;
+ f->depth++;
+ }
if (f->depth_needed == BX_MAX_BIT32U) {
f->depth_needed = cmdfifo_calc_depth_needed(f);
}
@@ -2943,7 +2947,7 @@ Bit32u cmdfifo_r(cmdfifo_info *f)
void cmdfifo_process(cmdfifo_info *f)
{
- Bit32u command, data, mask, nwords, regaddr;
+ Bit32u command, data, mask, nwords, regaddr, prev_rdptr;
Bit8u type, code, nvertex, smode, disbytes;
bool inc, pcolor;
voodoo_reg reg;
@@ -2959,7 +2963,9 @@ void cmdfifo_process(cmdfifo_info *f)
case 0: // NOP
break;
case 3: // JMP
+ prev_rdptr = f->rdptr;
f->rdptr = (command >> 4) & 0xfffffc;
+ f->amin -= prev_rdptr - f->rdptr;
if (f->count_holes) {
BX_DEBUG(("cmdfifo_process(): JMP 0x%08x", f->rdptr));
} Some problems remain however (crash with blt_rectangle_fill for example), so more testing and thinking is needed.
upd. if (!f->count_holes) should be added before f->amin -= prev_rdptr - f->rdptr;.
But it is possible to debug problem with crash even without this change.
upd2. Here is commit with addition mentioned above: https://github.com/Vort/Bochs/commit/c3198780f95f26436740535cf4a2e6b36f678051.
Vort
commented
9 months ago Looks like blt_rectangle_fill crash is triggered by race condition in driver itself.
I've added BX_ERROR prints to see exactly what is happening: https://github.com/Vort/Bochs/commit/344a3528e7039fe1b33bebb2cf7ce25dad9dc133.
And here is how logs looks like when problem appears:
Most likely, driver expects Memory write blt_dstBaseAddr: 0x00010400 to happen first.
Then Rectangle fill: 1024 x 4079 ROP0 CC and only then Memory write blt_dstBaseAddr: 0x00d00000.
But what happens is:
17920505693e[VOODOO] Memory write blt_dstBaseAddr: 0x00010400
...
17920684597e[VOODOO] Memory write blt_dstBaseAddr: 0x00d00000
...
17920900310e[VOODOO] CMDFIFO read: AMin=0x00000038 AMax=0x00000038 RdPtr:0x00000014 value:0x0fef0400
...
17920901406e[VOODOO] Rectangle fill: 1024 x 4079 ROP0 CCThere may be two reasons why driver authors did not noticed this problem:
Whether this problem can be avoided somehow by Bochs or not, I think it worth adding checks to bx_banshee_c::blt_rectangle_fill preventing out of bounds access. @vruppert, what do you think about such idea?
When I install Sfft1.9.zip drivers for Voodoo 3 on Windows XP and reboot machine, hang happens - only mouse cursor on black background is visible.
Looks like hang happens because of incorrect handling of disabled
count_holesfeature.I don't know how to fix it properly, but such hacky change allowed me to boot OS:
Also I don't know if my
cmdBumpchange is correct - I implemented it semi-randomly.Version: 54831068df334989405f16f85117f7f46fef944d