From 7fcea596b7ee479e4e715ad01085d029fb236582 Mon Sep 17 00:00:00 2001
From: Jonathan Campbell <jonathan@castus.tv>
Date: Sat, 7 Feb 2015 02:02:50 -0800
Subject: [PATCH] IRQ dispatch bugfix: PIC emulation used a global variable to
 determine 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.
---
 src/hardware/pic.cpp | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/hardware/pic.cpp b/src/hardware/pic.cpp
index ec7d004d332..b7a233ef8ad 100644
--- a/src/hardware/pic.cpp
+++ b/src/hardware/pic.cpp
@@ -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);
 	}
@@ -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();
@@ -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) {