Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bcm2835-pcm: Fix up multichannel pcm audio #3933

Merged
merged 1 commit into from Oct 28, 2020

Conversation

popcornmix
Copy link
Collaborator

Fixes: a9c1660
Signed-off-by: Dom Cobley popcornmix@gmail.com

@popcornmix
Copy link
Collaborator Author

The previous commit enabled multichannel for the spdif device (as we were experimenting with HBR passthrough which never worked through this interface). The more useful interface to enable multichannel is the pcm one.

Tested here: https://www.diyaudio.com/forums/pc-based/334443-2-8-dsp-platform-using-raspberry-pi-hats-10.html

@pavhofman
Copy link
Contributor

IMO the problem is the HDMI cards created by the xxx_simple_xxx config call the function snd_bcm2835_new_pcm with param spdif=false which causes that the 2ch hw params for the headphones route are chosen, instead of the 8ch spdif hw params.

Perhaps something like this could be a solution (not tested at all):

index 092ccbf..e57f5f8 100644
--- a/bcm2835.c
+++ b/bcm2835.c
@@ -91,7 +91,7 @@ static int bcm2835_audio_alsa_newpcm(struct bcm2835_chip *chip,
        return 0;
 }
 
-static int bcm2835_audio_simple_newpcm(struct bcm2835_chip *chip,
+static int bcm2835_audio_headphones_newpcm(struct bcm2835_chip *chip,
                                       const char *name,
                                       enum snd_bcm2835_route route,
                                       u32 numchannels)
@@ -99,6 +99,14 @@ static int bcm2835_audio_simple_newpcm(struct bcm2835_chip *chip,
        return snd_bcm2835_new_pcm(chip, name, 0, route, numchannels, false);
 }
 
+static int bcm2835_audio_hdmi_newpcm(struct bcm2835_chip *chip,
+                                      const char *name,
+                                      enum snd_bcm2835_route route,
+                                      u32 numchannels)
+{
+       return snd_bcm2835_new_pcm(chip, name, 0, route, numchannels, true);
+}
+
 static struct bcm2835_audio_driver bcm2835_audio_alsa = {
        .driver = {
                .name = "bcm2835_alsa",
@@ -119,7 +127,7 @@ static struct bcm2835_audio_driver bcm2835_audio_hdmi0 = {
        .shortname = "bcm2835 HDMI 1",
        .longname  = "bcm2835 HDMI 1",
        .minchannels = 1,
-       .newpcm = bcm2835_audio_simple_newpcm,
+       .newpcm = bcm2835_audio_hdmi_newpcm,
        .newctl = snd_bcm2835_new_hdmi_ctl,
        .route = AUDIO_DEST_HDMI0
 };
@@ -132,7 +140,7 @@ static struct bcm2835_audio_driver bcm2835_audio_hdmi1 = {
        .shortname = "bcm2835 HDMI 2",
        .longname  = "bcm2835 HDMI 2",
        .minchannels = 1,
-       .newpcm = bcm2835_audio_simple_newpcm,
+       .newpcm = bcm2835_audio_hdmi_newpcm,
        .newctl = snd_bcm2835_new_hdmi_ctl,
        .route = AUDIO_DEST_HDMI1
 };
@@ -145,7 +153,7 @@ static struct bcm2835_audio_driver bcm2835_audio_headphones = {
        .shortname = "bcm2835 Headphones",
        .longname  = "bcm2835 Headphones",
        .minchannels = 1,
-       .newpcm = bcm2835_audio_simple_newpcm,
+       .newpcm = bcm2835_audio_headphones_newpcm,
        .newctl = snd_bcm2835_new_headphones_ctl,
        .route = AUDIO_DEST_HEADPHONES
 };

@pavhofman
Copy link
Contributor

Well, multiple users report 8ch HDMI works OK on their RPis https://www.diyaudio.com/forums/pc-based/334443-2-8-dsp-platform-using-raspberry-pi-hats-10.html#post6345442

@popcornmix
Copy link
Collaborator Author

@HiassofT what is your view on these settings?
On Pi0-3 we can support:
8 channel 48kHz PCM
2 channel 192kHz PCM
2 channel 44.1/48kHz passthrough

@HiassofT
Copy link
Contributor

@popcornmix I don't think it's possible to express that via standard pcm constraints.

A solution would be to do it programatically. i.e. specify relaxed constraints (max 8ch, 192kHz) and reject invalid combinations with EINVAL in hw_params

@popcornmix
Copy link
Collaborator Author

What about the snd_bcm2835_playback_hw and snd_bcm2835_playback_spdif_hw devices?
Do we treat the spdif as passthrough only and only report what we believe to be useful or report the full range of channels/samplerates?

@HiassofT
Copy link
Contributor

TBH I'm not too familiar with that particular driver.

From a quick glance it looks like the spdif device might work like normal HDMI audio devices and PCM/non-audio can be selected via IEC958 ctl. So supportting multi-channel and higher rates on would be fine - difference to the non-spdif device seems to be that it's a single-user non-mixing device.

OTOH it depends on what the hardware+firmware can support and what you want to support - allowing non-working combinations to be used might not be ideal. So if you want the spdif device to be 2ch/48kHz only then that's fine, too.

@pavhofman
Copy link
Contributor

That's my experience too. The spdif (or HDMI) is a regular PCM device, without mixer controls provided by the codecs. IMO "passthrough" is just a name of a bit-perfect (i.e. no mixer/volume control) path. Digital (nonPCM and nonDSD) formats have a marker in the stream (and a corresponding flag in the SPDIF preamble, if applicable).

People use RPi to feed 8ch HDMI to a HDMI extractor to get 8 analog channels. Until recently they had to recompile kernel to change the channels_max hw param, with the latest patches the snd_bcm2835_playback_spdif_hw params already specify 8ch but the card-based device (enable_hdmi=1 instead of the device based enable_compat_alsa=1) uses snd_bcm2835_playback_hw params defined for the headphone audio (PWM-based) hardware, even though it's still the same HDMI hardware.

Fixes: a9c1660
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
@popcornmix
Copy link
Collaborator Author

Fixed the rates of snd_bcm2835_playback_spdif_hw so it now purely a revert of a9c1660.
I believe this device was always intended to be used for passthough so just report what works.
The pcm device is upgraded for multichannel and should be used when that is the use case.

@pelwell pelwell merged commit 551bfe1 into raspberrypi:rpi-5.9.y Oct 28, 2020
@pavhofman
Copy link
Contributor

Please do I understand correctly this is the current result?

module param enable_headphones=1 => using snd_bcm2835_playback_hw => 8ch/192kHz on the stereo analog jack
enable_hdmi=1 => using snd_bcm2835_playback_hw => 8ch/192kHz on HDMI0/1 (depending on which output is currently used)

enable_compat_alsa=1 => using snd_bcm2835_playback_hw for headphones and snd_bcm2835_playback_spdif_hw for HDMI0/1 ( https://github.com/popcornmix/linux/blob/75daef083656bd9d84a388089b5671e1eca8640c/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c#L78 ) => 8ch/192kHz on the stereo analog jack + 2ch/48kHz on HDMI0 + 2ch/48kHz on HDMI1

Please correct me if I am wrong.

@popcornmix popcornmix deleted the multichanpcm branch October 28, 2020 12:58
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Oct 31, 2020
kernel: drm/vc4: hdmi: Avoid sleeping in atomic context
See: raspberrypi/linux#3910

kernel: bcm2835-pcm: Fix up multichannel pcm audio
See: raspberrypi/linux#3933
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Oct 31, 2020
kernel: drm/vc4: hdmi: Avoid sleeping in atomic context
See: raspberrypi/linux#3910

kernel: bcm2835-pcm: Fix up multichannel pcm audio
See: raspberrypi/linux#3933
@gordoste
Copy link

gordoste commented Nov 1, 2020

Hi @popcornmix , I think it may be necessary to modify lines 83 and 87 of bcm2835.c - the HDMI devices get a maximum channel count of 2 because the spdif parameter to snd_bcm2835_new_pcm is true.
Of course, the workaround is to simply not use the enable_compat_alsa parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants