Fix many issues in OpenXDK XAudio #364
Conversation
JayFoxRox
force-pushed
the
fix-xaudio
branch
2 times, most recently
from
d8c77f8 to
4582eaf
Compare
|
I tested this in conjunction with https://github.com/XboxDev/nxdk-sdl/pull/27 running on a 1.4 Xbox with digital audio out (SPDIF) I compiled these homebrew games and the 'crackling' audio was no longer present and there was no other noticable audible issues to my ears. Interestingly, the audio in the latest xqemu release from xqemu.com was no longer working with this PR. Omnispeak would hang, other tests just had no audio, but worked fine on hardware. Did not investigate this further. I have not stress tested this and only played ~5 minutes. Will report back after some game time If I experience any issues or otherwise. |
| ; | ||
|
|
||
| // reset bus master registers for digital output | ||
| pb[0x17B] = (1 << 4) | (1 << 3) | (1 << 2) | (1 << 1); |
There was a problem hiding this comment.
nit: This could be done at the same time as the other reset, to reduce wait times
There was a problem hiding this comment.
I'm not planning to change this, because I don't want to force another round of testing.
I'll need to look at AC97 for XQEMU anyway, and I can clean this up then.
In a future cleanup pass, we can also define constants for these fields etc. (my code just mirrors the existing style - which was also ugly), if we don't end up making an all new API altogether.
However, for now, this works.
| pac97device->mmio[0x12C>>2] |= 2; | ||
|
|
||
| // wait until the chip is finished resetting... | ||
| while(!(pac97device->mmio[0x130>>2]&0x100)) | ||
| ; | ||
|
|
||
| // reset bus master registers for analog output | ||
| pb[0x11B] = (1 << 4) | (1 << 3) | (1 << 2) | (1 << 1); |
There was a problem hiding this comment.
Should check if the cold-reset already resets these, too
There was a problem hiding this comment.
At least in QEMU, the cold-reset is a no-op. So I assume it only resets the codec.
I'll create an issue about this after merge, so we can check.
For now, assume that this is required.
There was a problem hiding this comment.
Code looks overall ok.
There are a couple of things that I think could be changed, but they span the whole code and not just the lines changed here (such as using unsigned char instead of uint8_t when talking to hardware, formatting etc.).
I haven't tested the changes myself yet, so I'll wait for that with the merge.
lib/hal/audio.c
Outdated
| bool waitCompleted = false; | ||
|
|
||
| if (analogInterrupt) { | ||
|
|
There was a problem hiding this comment.
nit: This newline feels weird.
There was a problem hiding this comment.
Agreed; removed the empty lines.
lib/hal/audio.c
Outdated
|
|
||
| // If a buffer was consumed by analog and digital output, we ask for a DPC | ||
| if (waitCompleted) { | ||
| KeInsertQueueDpc(&DPCObject,NULL,NULL); //calls user callback soon |
There was a problem hiding this comment.
This came from the original code. I've re-arranged it, and added another commit to also improve the DPC function, which had similar issues.
It's still not ideal, but better. The goal of this PR is functionality, not style.
|
I've tested it now, and the results look good. Even when running SDLPoPX without optimizations, where sound previously crackled like crazy, it now sounds perfect. Seems to be perfectly stable as well. |
I agree. But formatting / cleanup wasn't really a goal here. If I had to design a new audio driver from scratch, it would be radically different. This is really just to have the old code with the old API (which is broken by design) working. Fixing the types would also require other changes like moving data from headers into the files. |
This code was tested very little; in particular, I did not test it with any existing OpenXDK XAudio applications. I also did not test the digital audio-out.
Please test and review before merge. For testing, I've been using JayFoxRox#78 which can create some metadata for audacity.
Synchronize analog and digital buffers
The interrupts for analog audio come about 16 samples before digital audio interrupts. This means we might mark a buffer as processed (by calling the user callback) before the AC97 has finished processing it.
We also cleared the interrupts for the digital audio in the interrupt handler of analog audio. This could lead to race conditions.
This commit keeps counters so we always know how many buffers there are per bus master.
Only once both bus masters have processed the buffer, we call the callback.
Only cause 1 callback per XAudioProvideSamples
When AC97 runs out of data (because no new buffer has been provided) it will actually keep repeating the last buffer. Each buffer completion causes an interrupt.
This means that submitting a single buffer usually caused infinitely many callbacks.
If the application submitted 2 buffers, it would not know if one of the buffers played twice, or if both buffers have played.
This code changes the callback behaviour:
If the last buffer is played it will have 4 (last valid buffer completion) in addition to the usual 8 (end of buffer) set as interrupt reason (MMIO register 0x116 / 0x176).
If this condition has been found, a single callback is generated. Further buffer completions are ignored until new buffers have been submitted and started processing.
Unfortunately, there might be a risk of losing buffers now: If the buffer is too small, I could imagine that AC97 fires 2 interrupts but the CPU might only see one of them (as it's not fast enough to clear the interrupt flag between the two). As such, the CPU might skip one callback. This is only a theoretical problem though. I'll create an issue about this after merge, so it can be investigated.
Respect timing requirement of AC97 cold-reset
AMD AC97 docs say that cold-reset must be pulled to 0 for at least 1 microsecond.
Our current code never pulled it to zero and also didn't respect any timing.
This commit respects this.
(This was supposed to prevent #362, but it did not work. It doesn't seem to have any practical effect)
Reset bus master registers at startup
Saw this in the documentation and figured I should add it.
It might be unnecessary (is cold-reset enough?).
(This was supposed to prevent #362, but it did not work. It doesn't seem to have any practical effect)
Avoid recalculation of descriptor parameters
General cleanup.
Rename nextDescriptorMod31 to nextDescriptor
General cleanup. Intention was correctness and readability.
Improve use of the volatile keyword
General cleanup. Intention was better support for optimizations and more stability.