Fifo overflow fix

[skidau]

No description provided.

[MayImilae]

No description? :(

[comex]

For the record, fixing the issue I identified would involve having the threadsafe event re-check the conditions on the CPU thread (i.e. calling SetCPStatusFromCPU or whatever rather than going straight to UpdateInterrupts) in case they changed since the GPU sent it. I am somewhat skeptical this is the actual culprit, but it is definitely possible for it to cause incorrect behavior.

[comex]

My comments on the previous commits appear to be gone, so:

• "Separated out the CPU and GPU thread path to avoid clobbering." Does separating them like this do anything other than slightly affect timing? I don't see any different logic.
• "Flush the regcache before each FIFO write check. Fixes the FIFO overflow that occurs in NBA2K11." Why would this do anything other than, again, slightly affect timing, by forcing the JIT to reload registers for no reason?
• "Update CPStatus before processing the FIFO events and force an exception check on interrupts." There are multiple changes here, but I don't understand why changing the order of the function calls is appropriate.
• "Added a wait state if the CPU thread is running ahead of the GPU thread" I guess this will avoid the symptom, but it looks like a hack to me - makes sense if some emulated code is really fundamentally depending on timing of the GPU to avoid overflow, but I wouldn't be surprised if Dolphin is doing something wrong instead.

[MayImilae]

Kazuma tested it and found that The Last Story doesn't encounter fifo overflow errors in OpenGL (during the fifo overflow nasty lazulis attack). I confirmed his findings and tested in D3D as well. No fifo overflows.

[FioraAeterna]

since my comments keep getting removed, I agree with comex on the flushing bit, that doesn't make sense to me at all

[skidau]

I tried adding a SetCPStatusFromCPU call in UpdateInterrupts_Wrapper and as pointed out (in irc), that was not the best place for it and it did not have any effect that I noticed.

The idea behind separating the CPU and GPU thread path is to prevent two threads executing the same function (SetCPStatusFromCPU) at the same time. The CPU thread gets to that function via GetherPipeBursted and the GPU thread gets there via SetCPStatusFromGPU. I wrote the original (factored) version as I like to keep code duplication to a minimum. This worked most of the time but not all of the time. It was a good move to separate it to a GPU version and a CPU version, but then the GPU version called the CPU version at the tail end. Removing the call (as in this PR) prevents the FIFO overflow in Starfox Adventures.

Flushing the regcache is a strange one. I thought there was a bug in FLUSH_MAINTAIN_STATE, but I could not find it. The FLUSH_MAINTAIN_STATE code is quite simple and is correct. My next guess is that there is a regcache leakage somewhere. Force flushing the regcache prevents FIFO unknown opcodes in the intro of NBA2K11.

The "Updating the CPStatus before FIFO events" change is simply there to keep the DC code path and the SC code path consistent in the !GPLinked scenario. The SC code path does the same thing when !GPLinked via RunGPU. Putting the SetCPStatus call before the !GPLinked state changes the DC code path to do the same thing. The more we keep the DC and SC code paths consistent, the better.

Forcing the exception check on interrupts is the change for The Last Story. It makes the emulator process interrupts more responsively on CP interrupts.

I have removed the "Added a wait state" change as that code is a little too late if the user is hitting that case. It has been replaced with the HiWatermark check which is something that old (pre-2010) versions of Dolphin used to do. Games like Battalion Wars 2 do not expect the GPU to run so slowly and do not have the code to react to that situation, so this change makes the emulator extra careful in those situations.

[JMC47]

It seems slightly slower (4 - 5%) than Master in a lot of situations. That's probably worth the stability + accuracy, but we should make it known in the progress report/blog post that it should be expected to be slower.

[comex]

These changes probably should not be affecting performance, except for the regcache one, which is just a hack and should not be merged without figuring out what the real problem is. Might be a JIT bug that has little to do with the FIFO as such...

[skidau]

Have removed the flushing of the regcache change.