bsnes hdma fix

[nattthebear]

https://board.byuu.org/viewtopic.php?f=4&t=2197

[brunovalads]

The full byuu's statement natt posted:

In all previous higan and bsnes versions, there was a bug with DMA and HDMA timing that I just caught by chance.

The way DMA sync works (or HDMA sync when it triggers at a time that DMA is not already running), is:

1. let one more CPU cycle execute
2. record how many clocks said cycle took
3. synchronize the CPU to the DMA clock divider (CPU clock / 8): step(8 - clock % 7);
4. run the (H)DMA transfer
5. synchronize the DMA back to the last CPU cycle's divider (DMA clock / lastCycle): step(lastCycles - dmaClocks % lastCycles);

In this case, dmaClocks are the number of clock cycles executing during the DMA, the sum of cycles from steps 3 and 4. To facilitate this, the CPU::dmaStep() function was added, which was like CPU::step(), but it incremented dmaCounter.

But there was a problem: DRAM refresh only ever called CPU::step(), even during (H)DMA transfers.

All of my HDMA test ROMs ended up missing this and passing, because lastCycles ended up being 8 (slow), which is evenly divisible by 40. But 6 (fast) and 12 (serial) are not, and will result in subtle timing errors. Not an issue for general emulation, but a big deal for real hardware and for perfect emulation.

My fix was to eliminate dmaStep() and dmaClocks. Instead, when performing the CPU->DMA sync, I cache the current CPU clock cycle to a DMA CPU cycle value. When the DMA completes, the number of cycles executed is cycleCounterCPU-cycleCounterDMA. And to handle overflow, the full function is:

auto CPU::dmaClocks() const -> uint {
  if(counter.cpu >= counter.dma) {
    return counter.cpu - counter.dma;
  } else {
    //99% sure this will work; the counters are type uint32_t
    return 0 - counter.cpu + counter.dma;
  }
}

Also, counter.dma = counter.cpu; needs to happen right before both CPU->DMA sync calls, rather than during the status.dmaActive = true; statement where status.dmaClocks = 0; was at before, because otherwise the one cycle to run ahead gets added to the number of DMA clocks executed, which is wrong.

[YoshiRulz]

I assume this is something that was set to be backported to our BSNES fork @nattthebear? If so, the new BSNES core should include the relevant patches.