At Thu, 20 Oct 2011 21:32:38 +0400, Konstantin Ozerkov wrote:
Thank you for fast response and comments.
I will rewrite this patch according you suggestions:
- add VM environment detection logic (PD only, I don't have clear vision how to
that for VirtualBox/QEMU) and module parameter (for manual control)
- make separated VM-optimized variant of snd_intel8x0_pcm_pointer()
Sounds reasonable.
Initially this patch is result of research and profiling audio emulation quality in Parallels Desktop.
On 20.10.11 19:21, Takashi Iwai wrote:
At Thu, 20 Oct 2011 19:02:50 +0400, Konstantin Ozerkov wrote:
This patch intended to improve performance in virtualized environments like Parallels Desktop or VirtualBox/QEMU (virtual ICH/AC97 audio).
I/O access is very time-expensive operation in virtual world: VCPU can be rescheduled and in the worst case we got more than 10ms delay on each I/O access.
In the original code normal loop exit rule (old_civ == current_civ&& old_picb == current_picb) was never satisfied, because old_picb was never the same as current_picb due delay inspired by reading current_civ. As a result loop ended by timeout and we got 10x more I/O operations.
You mean in the virtual environment, right? On the real machines, this condition can be satisfied normally because the read is much faster than the sample rate. This check is needed to avoid the broken behavior around the boundary.
To prevent glitch on buffer swapping the rule (old_civ == current_civ) is enough.
Not for real (thus less logical) machines, unfortunately. Both checks were needed in the real tests.
Can you say which hardware glitches this way and and which conditions? My simplified rule works well in many Windows AC97 drivers which I debug/analyze.
Some hardware showed the unstable CIV and PICB values when it goes crossing the boundary. Both registers should be updated simultaneously, but in reality, you may hit at a bad timing where only one of them is updated. And the order of updates of CIV/PICB isn't defined. Actually, in the earlier version, we tested only CIV, but it turned out that it's didn't suffice. So, we had to check both values.
thanks,
Takashi