bcm2835-i2s: Changes for allowing asymmetric sample formats. (#1783) #1803
Conversation
This is achieved by making changes only to the requested stream direction format, keeping the other stream direction configuration intact. Signed-off-by: Giedrius Trainavicius <[email protected]>
|
Disabling buth TXON and RXON doesn't sound correct. If a stream is
already running you shouldn't touch it's configuration and the
common I2S configurations like master/slave settings.
I think we first have to look why opening another stream causes
a glitch on the other stream and fix that. I haven't noticed that
but'll try to do some tests later this week.
|
|
The alternative would be to return errors and not changing the stream configurations at all, which would prevent user completely from doing what he intended... https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2835/BCM2835-ARM-Peripherals.pdf says about most of registers "This register cannot be changed whilst the PCM is running." and the condition of running seems to be "EN && (TXON || RXON)", so in order to change the stream configuration, it seems TXON and RXON must briefly be disabled and restored. Maybe you have access to Broadcom support and can ask them what is the suggested way to reconfigure the streams during playback? This isn't discussed anywhere in the documentation. About the glitches - I've tried preventing all register access when configuring the new stream by inserting early returns for that stream, so even without the code touching any of the registers, the glitch still occurs, so I think this indicates it's at some other level of ALSA. To reproduce - play some relatively long audio file using aplay, and start recording with same format to another file using arecord. Everytime you restart arecord, you can hear the glitch occur, it's a short click / pop. |
|
I re-read the docs, it's quite explicit about the setup procedure
in section 8.3 - neither TXON nor RXON must be set during configuration.
So the bug seems to be that symmetric_samplebits is not set in
snd_soc_dai_driver - symmetric_rates is set, symmetric_channels
is missing but since channels_min=channels_max there's not much
choice anyways.
Could you run a test with symmetric_samplebits = 1 in
bcm2835_i2s_dai? In this case ALSA should prevent setting up asymmetric
configurations which are not supported by the driver.
As for the glitch, I did a quick test (recording a SPDIF transmission
and comparing it with the original) and could reproduce it:
The same 16-bit stereo sample was repeated about 80 times. Not sure
where this bug comes from, I'll try to investigate it (could take
some time, currently quite busy with work).
|
|
In my testing, switching TXON and RXON to off during reconfiguration seemed to work fine. As I said in the previous pull request, I tried symmetric_samplebits set as 1, but Audacity fails to work this way, it still tries to request 16 bits for recording and 32 bits for output. Does the glitch always occur for the same amount of samples? Does my patch increase the sample affected count? |
|
The length of the glitch varies, from about 64 to 82 stereo-samples (on a RPi3 with kernel 4.9.4). I did some testing but so far nothing stuck out. ATM I'm not even sure if the BCM I2S dai or one of the codecs (wm5102/wm8804) is responsible for that. I tried overdub in audacity and it worked just fine. The wm5102 codec has symmetric_samplebits set and using aplay/arecord I can verify that the symmetric feature works. First play a file and let aplay output the allowed hw params. Note the all 3 sample formats and both 16 and 32 bit are allowed: Then, why aplay is running run arecord, again dumping the allowed hw params. Only S16_LE format is allowed now: Trying to record with S32_LE also gives an error. I haven't tried your patch yet. IMO you are barking up the wrong tree. We can't safely support asymmetric sample modes, changing the setup while a stream is running is not possible. And I don't consider stopping the other stream an acceptable solution. |
|
What settings did Audacity have in your tests, and which modes were requested in hw_params? In my case, I tried setting recording quality to 16 bits, so it was requesting recording using 16 bits, but for playback, it was still requesting 32 bits. With symmetric sample bits enabled, it was failing. I think Audacity chooses the hw_params first without starting playback or recording. At that point all formats are available. Once it starts one of the streams, the other one can no longer start, and Audacity fails. Running arecord and aplay is different from what Audacity is doing, because they start at different times, based on the latest available constraints. |
|
With sample format set to the default (32 bit float) everything's fine. Also with 24 bit. But when setting the sample format to 16 bit I can reproduce your issue: New project, record something, stop, record again (overdub) and I get an error message "please check soundcard settings" and messages on the console: If I then try record (overdub) a second time it works fine. I've tried changing the playback/recording settings (default, sysdefault - which should use plughw and RPi-Cirrus:hw) but that didn't make a difference. I suspect that could be a bug in Audacity (I'm using the raspbian version 2.0.6 which is already quite old - latest version is 2.1.2 from january last year). |
|
I don't think symmetric_samplebits is the right way to go, because the processor itself supports asymmetric rates. Could you give the patched code a go and check what's the impact on the samples during reconfiguration of stream and the glitch itself? |
|
I don't think symmetric_samplebits is the right way to go, because the processor itself supports asymmetric rates.
The I2S chip could probably support that, but not in the context of ALSA
where parameters are set up independently per stream. At least I know
of no solution to achieve this without (possibly) breaking something.
Could you give the patched code a go and check what's the impact on the samples during reconfiguration of stream and the glitch itself?
I tested with your patch: first there's a frame (stereo-sample) repeating
about 70 times, then the next frame of the original stream is
repeated 2 times, then 27 frames match, then a frame with all-zero bits
follows, after that the rest of the stream seems to be shifted by
1 frame.
I haven't seen this in my previous 4 tests last week, but even if
this additional all-zero-bit frame is not related to your patch
disabling transmission is just plain wrong.
|
Hey, this is the update code for asymmetric sample formats, addressing the issue found by @HiassofT #1799
The new changes are mainly introducing previous_txon_rxon to remember which streams were enabled previously, then both TXON and RXON get temporarily disabled (according to peripherals documentation, most of register bits can't be changed while either one of them is enabled), clearing only the FIFO of requested direction.
I noticed that starting the stream in the other direction while there was an already active stream produces a short glitch in the other stream, but the same thing happens in unmodified code, I've tried disabling all register accesses, even not starting one of the streams if requested, and the glitch was still happening, so it must be someplace else than this driver...