From f7ff089689dbdf3fcde8a4b703a0282e77b98885 Mon Sep 17 00:00:00 2001 From: Sour Date: Thu, 21 Nov 2019 22:27:39 -0500 Subject: [PATCH] Core: Fix NMI code to better reflect the hardware Fixes a crash in Super Chinese 3 caused by the PPU rapidly switching the NMI signal on and then back off (which caused the CPU to incorrectly jump to the IRQ handler and crashed the game) --- Core/CPU.cpp | 32 +++++++++++++++++++++++--------- Core/CPU.h | 4 ++++ Core/PPU.cpp | 31 +++++++++++++------------------ 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/Core/CPU.cpp b/Core/CPU.cpp index 7a2a99c6..e59c69f8 100644 --- a/Core/CPU.cpp +++ b/Core/CPU.cpp @@ -145,7 +145,7 @@ void CPU::Reset(bool softReset, NesModel model) string cpuAlignment = " CPU: " + std::to_string(cpuOffset) + "/" + std::to_string(cpuDivider - 1); MessageManager::Log("CPU/PPU alignment -" + ppuAlignment + cpuAlignment); } else { - _ppuOffset = 2; + _ppuOffset = 1; cpuOffset = 0; } @@ -165,7 +165,7 @@ void CPU::Exec() _operand = FetchOperand(); (this->*_opTable[opCode])(); - if(_prevRunIrq) { + if(_prevRunIrq || _prevNeedNmi) { IRQ(); } } @@ -180,12 +180,12 @@ void CPU::IRQ() DummyRead(); //read next instruction byte (actually the same as above, since PC increment is suppressed. Also discarded.) Push((uint16_t)(PC())); - if(_state.NMIFlag) { + if(_needNmi) { + _needNmi = false; Push((uint8_t)(PS() | PSFlags::Reserved)); SetFlags(PSFlags::Interrupt); SetPC(MemoryReadWord(CPU::NMIVector)); - _state.NMIFlag = false; #ifndef DUMMYCPU _console->DebugAddTrace("NMI"); @@ -208,7 +208,8 @@ void CPU::BRK() { Push((uint16_t)(PC() + 1)); uint8_t flags = PS() | PSFlags::Break | PSFlags::Reserved; - if(_state.NMIFlag) { + if(_needNmi) { + _needNmi = false; Push((uint8_t)flags); SetFlags(PSFlags::Interrupt); @@ -228,8 +229,8 @@ void CPU::BRK() { #endif } - //Since we just set the flag to prevent interrupts, do not run one right away after this (fixes nmi_and_brk & nmi_and_irq tests) - _prevRunIrq = false; + //Ensure we don't start an NMI right after running a BRK instruction (first instruction in IRQ handler must run first - needed for nmi_and_brk test) + _prevNeedNmi = false; } void CPU::MemoryWrite(uint16_t addr, uint8_t value, MemoryOperationType operationType) @@ -320,10 +321,22 @@ void CPU::EndCpuCycle(bool forRead) _masterClock += forRead ? (_endClockCount + 1) : (_endClockCount - 1); _console->GetPpu()->Run(_masterClock - _ppuOffset); + //"The internal signal goes high during φ1 of the cycle that follows the one where the edge is detected, + //and stays high until the NMI has been handled. " + _prevNeedNmi = _needNmi; + + //"This edge detector polls the status of the NMI line during φ2 of each CPU cycle (i.e., during the + //second half of each cycle) and raises an internal signal if the input goes from being high during + //one cycle to being low during the next" + if(!_prevNmiFlag && _state.NMIFlag) { + _needNmi = true; + } + _prevNmiFlag = _state.NMIFlag; + //"it's really the status of the interrupt lines at the end of the second-to-last cycle that matters." //Keep the irq lines values from the previous cycle. The before-to-last cycle's values will be used _prevRunIrq = _runIrq; - _runIrq = _state.NMIFlag || ((_state.IRQFlag & _irqMask) > 0 && !CheckFlag(PSFlags::Interrupt)); + _runIrq = ((_state.IRQFlag & _irqMask) > 0 && !CheckFlag(PSFlags::Interrupt)); } void CPU::StartCpuCycle(bool forRead) @@ -464,7 +477,8 @@ void CPU::StreamState(bool saving) Stream(_state.PC, _state.SP, _state.PS, _state.A, _state.X, _state.Y, _cycleCount, _state.NMIFlag, _state.IRQFlag, _dmcDmaRunning, _spriteDmaTransfer, extraScanlinesBeforeNmi, extraScanlinesBeforeNmi, dipSwitches, - _needDummyRead, _needHalt, _startClockCount, _endClockCount, _ppuOffset, _masterClock); + _needDummyRead, _needHalt, _startClockCount, _endClockCount, _ppuOffset, _masterClock, + _prevNeedNmi, _prevNmiFlag, _needNmi); if(!saving) { settings->SetPpuNmiConfig(extraScanlinesBeforeNmi, extraScanlinesAfterNmi); diff --git a/Core/CPU.h b/Core/CPU.h index 2717e7de..219f4fef 100644 --- a/Core/CPU.h +++ b/Core/CPU.h @@ -58,6 +58,10 @@ private: bool _prevRunIrq = false; bool _runIrq = false; + + bool _prevNmiFlag = false; + bool _prevNeedNmi = false; + bool _needNmi = false; uint64_t _lastCrashWarning = 0; diff --git a/Core/PPU.cpp b/Core/PPU.cpp index 5b6546cf..967592ea 100644 --- a/Core/PPU.cpp +++ b/Core/PPU.cpp @@ -269,7 +269,7 @@ uint8_t PPU::PeekRAM(uint16_t addr) switch(GetRegisterID(addr)) { case PPURegisters::Status: returnValue = ((uint8_t)_statusFlags.SpriteOverflow << 5) | ((uint8_t)_statusFlags.Sprite0Hit << 6) | ((uint8_t)_statusFlags.VerticalBlank << 7); - if(_scanline == 241 && _cycle < 3) { + if(_scanline == _nmiScanline && _cycle < 3) { //Clear vertical blank flag returnValue &= 0x7F; } @@ -518,15 +518,13 @@ void PPU::SetControlRegister(uint8_t value) _flags.BackgroundPatternAddr = ((_state.Control & 0x10) == 0x10) ? 0x1000 : 0x0000; _flags.LargeSprites = (_state.Control & 0x20) == 0x20; - //"By toggling NMI_output ($2000 bit 7) during vertical blank without reading $2002, a program can cause /NMI to be pulled low multiple times, causing multiple NMIs to be generated." - bool originalVBlank = _flags.VBlank; _flags.VBlank = (_state.Control & 0x80) == 0x80; - - if(!originalVBlank && _flags.VBlank && _statusFlags.VerticalBlank && (_scanline != -1 || _cycle != 0)) { - _console->GetCpu()->SetNmiFlag(); - } - if(_scanline == 241 && _cycle < 3 && !_flags.VBlank) { + + //"By toggling NMI_output ($2000 bit 7) during vertical blank without reading $2002, a program can cause /NMI to be pulled low multiple times, causing multiple NMIs to be generated." + if(!_flags.VBlank) { _console->GetCpu()->ClearNmiFlag(); + } else if(_flags.VBlank && _statusFlags.VerticalBlank) { + _console->GetCpu()->SetNmiFlag(); } } @@ -578,15 +576,11 @@ void PPU::UpdateStatusFlag() ((uint8_t)_statusFlags.Sprite0Hit << 6) | ((uint8_t)_statusFlags.VerticalBlank << 7); _statusFlags.VerticalBlank = false; + _console->GetCpu()->ClearNmiFlag(); - if(_scanline == 241 && _cycle < 3) { - //"Reading on the same PPU clock or one later reads it as set, clears it, and suppresses the NMI for that frame." - _statusFlags.VerticalBlank = false; - _console->GetCpu()->ClearNmiFlag(); - - if(_cycle == 0) { - _preventVblFlag = true; - } + if(_scanline == _nmiScanline && _cycle == 0) { + //"Reading one PPU clock before reads it as clear and never sets the flag or generates NMI for that frame." + _preventVblFlag = true; } } @@ -898,10 +892,10 @@ void PPU::UpdateGrayscaleAndIntensifyBits() int pixelNumber; if(_scanline >= 240) { pixelNumber = 61439; - } else if(_cycle < 4) { + } else if(_cycle < 3) { pixelNumber = (_scanline << 8) - 1; } else if(_cycle <= 258) { - pixelNumber = (_scanline << 8) + _cycle - 4; + pixelNumber = (_scanline << 8) + _cycle - 3; } else { pixelNumber = (_scanline << 8) + 255; } @@ -945,6 +939,7 @@ void PPU::ProcessScanline() //Pre-render scanline logic if(_cycle == 1) { _statusFlags.VerticalBlank = false; + _console->GetCpu()->ClearNmiFlag(); } if(_state.SpriteRamAddr >= 0x08 && IsRenderingEnabled() && !_settings->CheckFlag(EmulationFlags::DisableOamAddrBug)) { //This should only be done if rendering is enabled (otherwise oam_stress test fails immediately)