Skip to content

Commit

Permalink
IRQ dispatch bugfix: PIC emulation used a global variable to determine
Browse files Browse the repository at this point in the history
whether to check for IRQs and dispatch! While it generally worked,
running certain demos or games would eventually hit a corner case where
one device raises an IRQ, and one device lowers an IRQ that was raised,
that would cause IRQs to get stuck. Also responsible for stuck IRQs was
the way that PIC_runIRQs() was written. It is written to dispatch one
IRQ (in IRQ priority order) then break from the loop (or dispatch two
IRQs if IRQ 9 -> IRQ 2 cascade). The bug in that routine is that despite
handling only one IRQ it reset the flag as if all IRQs had been handled,
which yet again can cause stuck IRQs.

This fix changes the behavior of IRQ dispatch. When a device raises an
IRQ, the global flag is set as before. The new code avoids stuck IRQs
by NOT clearing the flag when the IRQ is lowered. PIC_runIRQs() has been
changed to not clear the flag unless there are absolutely no IRQs
pending to handle.

Preliminary testing shows that this fix resolves problems with Crystal
Dream II and GUS music getting stuck at a random point throughout the
demo.
  • Loading branch information
joncampbell123 committed Feb 7, 2015
1 parent 9a04357 commit 7fcea59
Showing 1 changed file with 21 additions and 4 deletions.
25 changes: 21 additions & 4 deletions src/hardware/pic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,19 @@ void PIC_Controller::activate() {
void PIC_Controller::deactivate() {
//removes irq check value if master, signals master if slave
if(this == &master) {
PIC_IRQCheck = 0;
/* NTS: DOSBox code used to set PIC_IRQCheck = 0 here.
*
* That's actually not the way to handle it. Here's why:
*
* What would happen if one device raised an IRQ (setting PIC_IRQCheck=1)
* then before dispatching IRQs, another device lowered an IRQ (setting PIC_IRQCheck=0).
*
* Lowering the IRQ would reset the flag, and PIC_runIRQs() would never process
* any pending IRQs even though some are waiting.
*
* It's better if we set the flag when raising an IRQ, and leave the flag set until
* PIC_runIRQs() determines there are no more IRQs to dispatch. Then and only then
* will PIC_runIRQs() clear the flag. */
} else {
master.lower_irq(2);
}
Expand Down Expand Up @@ -380,7 +392,9 @@ void PIC_runIRQs(void) {

const Bit8u p = (master.irr & master.imrr)&master.isrr;
const Bit8u max = master.special?8:master.active_irq;
for(Bit8u i = 0,s = 1;i < max;i++, s<<=1){
Bit8u i,s;

for (i = 0,s = 1;i < max;i++, s<<=1){
if (p&s){
if (i==2 && enable_slave_pic) { //second pic
slave_startIRQ();
Expand All @@ -390,8 +404,11 @@ void PIC_runIRQs(void) {
break;
}
}
//Disable check variable.
PIC_IRQCheck = 0;

/* if we cleared all IRQs, then stop checking.
* otherwise, keep the flag set for the next IRQ to process. */
if (i == max && (master.irr&master.imrr&slave.irr&slave.imrr) == 0)
PIC_IRQCheck = 0;
}

void PIC_SetIRQMask(Bitu irq, bool masked) {
Expand Down

0 comments on commit 7fcea59

Please sign in to comment.