Determine MMP3 audio buffer parameters

[dsd]

Open etoys, go to "tutorials and demos" and start the self-repeating demo. Sounds are played at intervals, but they sound bad (just like a quick loud scratch).

[mike_lee]

Replying to dsd:

Open etoys, go to "tutorials and demos" and start the self-repeating demo. Sounds are played at intervals, but they sound bad (just like a quick loud scratch).

Confirmed this by loading one of my Etoys lessons which contains many AIFF sound files. Same short burst of static noise regardless of length of original sound. http://squeakland.org/showcase/project.jsp?id=10289

Camera shutter sound in Record is also the same burst of static. Default "meow" sound in Sound tab of the "sprite1" cat is the same burst of static. Other sounds loaded from the Scratch Sounds collection also play as bursts of static.

[dsd]

Saadia observes that from the kernel's view, squeak appears to open the sound device, then close it and open it again before playing, and suspects this is the cause of the problem.

I suspect it is a kernel bug but I'm interested on seeing what's happening on the userspace side as well. I see that sound_Start() is called once, but the hwparams set there get rejected with -EINVAL, then sound_Start() gets called again. This causes the device to be closed and opened again. The second time, hwparams is accepted and then the sound gets played (badly). Need to figure out why hwparams is rejected the first time, and how this sequence is confusing the kernel.

[dsd]

Squeak patch to avoid the open-close-open sequence

[dsd]

test case

[dsd]

There is a minor squeak bug causing the open-close-open sequence here, it is passing uninitialized memory to ALSA which by coincidence seems to always have a good value when the device open is retried. A squeak patch to fix this is attached, and RPMs that have it included: http://dev.laptop.org/~dsd/20130103/

That does not solve this issue though, squeak sounds are still bad. Now what happens is that the device open and hw_params succeeds, but then it fails trying to start the playback, and aborts after making a short bad noise. Then it retries again and the same thing happens.

I've moved this code into a standalone test case, attached as atest.c. On XO-1.75 this works fine (i.e. it produces no errors) but on XO-4 it fails with:

snd_pcm_prepare: Device or resource busy
snd_pcm_start(2): File descriptor in bad state

I believe that if we can make this test case stop producing errors, squeak/etoys/scratch will then start working.

It is strange how it calls start, prepare, start, but if it worked before we can hopefully keep it working anyway...

[dsd]

The test case errors go away when kernel commit 858059d215b8d386eaf320bd17355cb185c3eab7 is reverted. But then Scratch fails to play sound with lots of "snd_pcm_writei returned -11" in the logs. I think this is the right direction though, now we have to look into that error.

[dsd]

updated test that also calls snd_pcm_writei

[dsd]

Last update for today. Squeak calls snd_pcm_writei with 512 frames at a time. It succeeds twice (writing 512 frames each time to the device) before it starts returning -11. The updated atest (attached) demonstrates this.

When combined with Saadia's debug messages, something interesting becomes apparent:

sound_Start OK
writei ret 512
writei ret 512
[  406.857050] .. CALL 3 snd_pcm_release_substream
writei ret -11
snd_pcm_writei returned -11

That snd_pcm_release_substream() doesn't seem like it should be there.

[dsd]

The above debug messages came out in jumbled order, so that wasn't anything interesting.

Reverted commit 858059d215b8d386eaf320bd17355cb185c3eab7 in arm-3.5 to take us closer here.

What is happening here is that Squeak requests that the hardware buffers the first 7/8 of the sound before automatically moving from PREPARE to START state (and hence playing the sound). In the case of the meow, this is 2313 frames.

However, on XO-4, there are only 1024 frames available in the hardware buffer. Thats why the third writei doesn't manage to copy any more data in, and the playback never starts, so it just sits there looping around with a full buffer.

atest3.c demonstrates this. XO-4 has 1024 frames available here, but XO-1.75 had 3968.

This is definitely bad behaviour from squeak here, it is asking for a lot of buffering and introduces perhaps more latency than necessary before it starts playing the sound. At the same time I imagine a good XO-4 audio experience also needs a good hardware buffer size to be available so it would be nice to "fix" this on the kernel side anyway.

The low number on XO-4 originates from the default values hardcoded in mmp_pcm_probe(). We don't get these from DT, looks like that info is not there in current firmware releases. If I increase this to match arm-3.0-wip/XO-1.75, i.e.:

	pbbuf = 15872;
	pbper = 4096;

then I now get 3072 available. Not as many as XO-1.75, but reasonable, and enough to satisfy squeak.

Saadia and I don't have a complete understanding of these values and their constraints. So, questions for Mitch would be:

1. Why are the values in mmp_pcm_probe() so low? Should these be coming from DT?
2. What are good values for XO-4? Is the SRAM size the limit here? What's the size of the MMP3 audio SRAM?

I will also work on improving Squeak's buffering behaviour, but preferably for inclusion in a future release rather than this one.

[dsd]

updated test case that shows the requested playback threshold and the size of the available hardware buffer

[dsd]

The default values in mmp_pcm_probe come from Marvell code - maybe there is a platform where memory is so low that these values make sense. If not, they could perhaps be increased.

In the MMP3 case, SRAM is shared between audio and other components, so the max size used by audio depends on various factors.

As an initial strategy I tried to follow qseven alpha4's mmp3-squ-zsp driver, which uses 64kb (presumably 64kb for playback and 64kb for capture), but this broke speaker-test -c 2 -t wav (no sound output, process seems to hang).

So I dropped to 64kb (32kb+32kb) instead, this still works and sound in etoys/scratch is fixed. This workaround is pushed as 62abd444014393c039f76e258 (it should reverted and fixed in the DT at a later date).

Now we need to investigate why the 128kb setting didn't work, and once we've made a decision, get the chosen values exposed from the firmware.

[wmb@…]

You can't use all of the SRAM for buffers because you need some for ADMA descriptors.

Maybe qseven works with 64k because no SRAM is earmarked for CForth, thus it still has room for descriptors.

The test version of OFW that I gave saadia reports 31K for each buffer, leaving a total of 0x1000 bytes for descriptors. I used 31K instead of 62K because I wasn't sure whether Linux was assuming double-buffering and thus allocating 4 buffers (2 playback, 2 capture). I now suspect that 62K for each buffer might be better, under the assumption that the total buffer space is carved into more than two buffers per direction.

[dsd]

Now when I try 64kb for each stream, it fails with a memory allocation error message. Not sure why I found otherwise earlier.

This matches my reading of the code. The firmware offers 128kb ASRAM to Linux. The mmp-pcm driver claims 64kb for each channel, 128kb total. Then the tdma code tries to claim 256 bytes for descriptors, and fails because there is no memory left.

Mitch was right, and the allocation strategy must keep this in mind. Reserving 1kb for buffers sounds sensible.

So marvell,buffer-sizes should state:

1. 62kb (playback buffer)
2. 2048 (playback max period size)
3. 62kb (recording buffer)
4. 2048 (recording max period size)

I don't know what considerations should be applied for choosing max period size, so I'm simply suggesting the values that are already in use in the driver.

[dsd]

Submitted squeak patches upstream:

Avoid the pointless double-open: http://lists.squeakfoundation.org/pipermail/vm-dev/2013-March/012508.html

Be more sensible when requesting buffer sizes: http://lists.squeakfoundation.org/pipermail/vm-dev/2013-March/012509.html

[dsd]

(In #12343) With the patch from #12549, etoys is working. Tested sound recording and playback on XO-1.5, XO-1.75 and XO-4.

With the squeak-vm patches from #12400, Scratch-24 is working. Tested sound recording, playback and camera usage on XO-1.5, XO-1.75 and XO-4.

I will add Scratch-24 to the next build, then we will wait on the closure of those 2 tickets.

Unfortunately the new Scratch version is 10mb bigger than the last one due to more files in the Media directory.

[dsd]

James, please add the following equivalent code to OFW:

dev /pcm@0
d# 63488 encode-int
d# 2048 encode-int encode+
d# 63488 encode-int encode+
d# 2048 encode-int encode+
" marvell,buffer-sizes" property
dend

Rationale for the values:

The /asram node (via its 'reg' property) defines that 128kb of SRAM is assigned to the audio functionality. We now divide that here, roughly half to playback and half to recording.

Even though we choose big buffers, we do leave a little bit of free space for the DMA engine which also needs to use a bit of the SRAM area for DMA descriptors. Therefore we choose 62kb for each buffer, leaving 4kb available for DMA descriptors.

For the max period size, we don't have any justification other than 2048 is the value we've been using all along.

[Quozl]

Is this to be XO-4 specific? (the reg property of /asram is from the constant /audio-sram which is 131072 on MMP3, and 65536 on MMP2.)

What is the value 2048 in the property? Is it room for DMA descriptors or is it maximum period size?

http://dev.laptop.org/~quozl/q7b29je.rom is a test build.

http://dev.laptop.org/~quozl/z/1USPNn.txt is the working patch.

The generated output is:

ok dev /pcm
ok .properties 
marvell,buffer-sizes     00 00 f8 00 00 00 08 00 00 00 f8 00 00 00 08 00 
compatible               marvell,mmp-pcm-audio
reg                      00000000 00000000 
name                     pcm
adma-node                000d00f8 
ok f800 .d
63488 
ok 

[dsd]

Those values are XO-4 specific. The 2048 value refers to max period size.

[Quozl]

svn 3645

[Quozl]

(change tracking was temporarily lost due to component incorrect).

[Quozl]

Fixed in Q7B30.

[dsd]

Test in 13.2.0 build 5

[dsd]

Thanks James. Unfortunately this half-broke audio recording, sorry for not testing that before. When recording at 48khz there is a loud audible pop every 171ms.

Experimenting a bit, I found that audio recording is only clean when the first DMA transfer happens to a SRAM address on an 8kb boundary. And we now start the audio recording buffer at an offset of 62kb into the ASRAM, which clearly does not meet such constraints. And unfortunately, genpool (the layer that manages ASRAM allocations) does not allow us to request specific alignments.

To work around that limitation, we can just round the allocation down to be cleanly divisible by 8192. Then when we make these allocations, genpool will make them contiguously at the start of ASRAM, and the allocations will be on 8kb boundaries as a result. This workaround is implemented in arm-3.5 a2200ace4. The 62kb suggested buffer size is now trimmed down to 56kb.

The firmware is still correct in suggesting 62kb buffer sizes - hopefully we will find a clean way to use all that buffer space some day.

I also reverted an earlier hack (62abd4440) now that we have this fixed in the firmware, and also Squeak has been fixed to not request buffer sizes that are outside of what the hardware can offer.

[dsd]

Test in 13.2.0 build 6.

[dsd]

Tested 13.2.0 build 7 on XO-4, sound recording work in Scratch