| Message ID | 20230624165216.5719-1-hias@horus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 04b49b90caeed0b5544ff616d654900d27d403b6 |
| Headers | show |
| Series | ALSA: pcm: fix ELD constraints for (E)AC3, DTS(-HD) and MLP formats | expand |
On Sat, 24 Jun 2023 18:52:16 +0200, Matthias Reichl wrote: > > The SADs of compressed formats contain the channel and sample rate > info of the audio data inside the compressed stream, but when > building constraints we must use the rates and channels used to > transport the compressed streams. > > eg 48kHz 6ch EAC3 needs to be transmitted as a 2ch 192kHz stream. > > This patch fixes the constraints for the common AC3 and DTS formats, > the constraints for the less common MPEG, DSD etc formats are copied > directly from the info in the SADs as before as I don't have the specs > and equipment to test those. > > Signed-off-by: Matthias Reichl <hias@horus.com> Thanks, applied now. Takashi
…
> This patch fixes the constraints for the common AC3 and DTS formats,
…
Please add an imperative change suggestion.
See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4-rc7#n94
How do you think about to add the tag “Fixes”?
Regards,
Markus
On Sun, Jun 25, 2023 at 02:10:21PM +0200, Markus Elfring wrote: > … > > This patch fixes the constraints for the common AC3 and DTS formats, > … > > Please add an imperative change suggestion. I assumed the motivation was pretty clear from the paragraph above which you snipped off: > The SADs of compressed formats contain the channel and sample rate > info of the audio data inside the compressed stream, but when > building constraints we must use the rates and channels used to > transport the compressed streams. > > eg 48kHz 6ch EAC3 needs to be transmitted as a 2ch 192kHz stream. The previous implementation added constraints that could be both too broad and incomplete at the same time, leading to the audio device accepting channel/rate combinations that are not supported by the sink while rejecting combinations that are required to transmit the compressed bitstream. Typical impact on users is eg "Dolby TrueHD passthrough does not work". Consider this EDID audio block of a 2020 Sony TV which rejected Dolby TrueHD passthrough: Linear PCM: Max channels: 6 Supported sample rates (kHz): 192 176.4 96 88.2 48 44.1 32 Supported sample sizes (bits): 24 20 16 AC-3: Max channels: 6 Supported sample rates (kHz): 48 44.1 32 Maximum bit rate: 640 kb/s DTS: Max channels: 6 Supported sample rates (kHz): 48 44.1 32 Maximum bit rate: 1504 kb/s Enhanced AC-3 (DD+): Max channels: 8 Supported sample rates (kHz): 48 44.1 Supports Joint Object Coding MAT (MLP): Max channels: 8 Supported sample rates (kHz): 48 Supports Dolby TrueHD, object audio PCM and channel-based PCM Hash calculation not required for object audio PCM or channel-based PCM The old implementation didn't add the 192kHz / 8ch combination that's required to transport the MLP TrueHD bitstream, so opening the device in 8ch 192kHz mode failed. > How do you think about to add the tag “Fixes”? I've thought about that but decided against it as adding exact constraints has the chance of breaking existing applications that accidentally relied on the (incorrect) previous behaviour of adding rather broad constraints. Consider the following EDID of a 2009 Sony TV: Linear PCM: Max channels: 2 Supported sample rates (kHz): 48 44.1 32 Supported sample sizes (bits): 24 20 16 AC-3: Max channels: 6 Supported sample rates (kHz): 48 44.1 32 Maximum bit rate: 640 kb/s Enhanced AC-3 (DD+): Max channels: 6 Supported sample rates (kHz): 48 44.1 32 The old implementation would have constraints that allowed up to 6ch output at 32/44.1/48kHz while the correct setup would be to only allow max 2ch output (both AC3 and EAC3 bitstreams are transmitted in 2ch mode). So you could successfully output eg 6ch audio (which the sink likely wouldn't accept and/or only output the first 2 channels) before but now this will get rejected. so long, Hias
>> Please add an imperative change suggestion. > > I assumed the motivation was pretty clear from the paragraph above which you > snipped off: How good does this view fit to a desire for another imperative change description? Regards, Markus
diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c index 4b5faae5d16e5..07075071972dd 100644 --- a/sound/core/pcm_drm_eld.c +++ b/sound/core/pcm_drm_eld.c @@ -2,11 +2,25 @@ /* * PCM DRM helpers */ +#include <linux/bitfield.h> #include <linux/export.h> +#include <linux/hdmi.h> #include <drm/drm_edid.h> #include <sound/pcm.h> #include <sound/pcm_drm_eld.h> +#define SAD0_CHANNELS_MASK GENMASK(2, 0) /* max number of channels - 1 */ +#define SAD0_FORMAT_MASK GENMASK(6, 3) /* audio format */ + +#define SAD1_RATE_MASK GENMASK(6, 0) /* bitfield of supported rates */ +#define SAD1_RATE_32000_MASK BIT(0) +#define SAD1_RATE_44100_MASK BIT(1) +#define SAD1_RATE_48000_MASK BIT(2) +#define SAD1_RATE_88200_MASK BIT(3) +#define SAD1_RATE_96000_MASK BIT(4) +#define SAD1_RATE_176400_MASK BIT(5) +#define SAD1_RATE_192000_MASK BIT(6) + static const unsigned int eld_rates[] = { 32000, 44100, @@ -17,9 +31,62 @@ static const unsigned int eld_rates[] = { 192000, }; +static unsigned int map_rate_families(const u8 *sad, + unsigned int mask_32000, + unsigned int mask_44100, + unsigned int mask_48000) +{ + unsigned int rate_mask = 0; + + if (sad[1] & SAD1_RATE_32000_MASK) + rate_mask |= mask_32000; + if (sad[1] & (SAD1_RATE_44100_MASK | SAD1_RATE_88200_MASK | SAD1_RATE_176400_MASK)) + rate_mask |= mask_44100; + if (sad[1] & (SAD1_RATE_48000_MASK | SAD1_RATE_96000_MASK | SAD1_RATE_192000_MASK)) + rate_mask |= mask_48000; + return rate_mask; +} + +static unsigned int sad_rate_mask(const u8 *sad) +{ + switch (FIELD_GET(SAD0_FORMAT_MASK, sad[0])) { + case HDMI_AUDIO_CODING_TYPE_PCM: + return sad[1] & SAD1_RATE_MASK; + case HDMI_AUDIO_CODING_TYPE_AC3: + case HDMI_AUDIO_CODING_TYPE_DTS: + return map_rate_families(sad, + SAD1_RATE_32000_MASK, + SAD1_RATE_44100_MASK, + SAD1_RATE_48000_MASK); + case HDMI_AUDIO_CODING_TYPE_EAC3: + case HDMI_AUDIO_CODING_TYPE_DTS_HD: + case HDMI_AUDIO_CODING_TYPE_MLP: + return map_rate_families(sad, + 0, + SAD1_RATE_176400_MASK, + SAD1_RATE_192000_MASK); + default: + /* TODO adjust for other compressed formats as well */ + return sad[1] & SAD1_RATE_MASK; + } +} + static unsigned int sad_max_channels(const u8 *sad) { - return 1 + (sad[0] & 7); + switch (FIELD_GET(SAD0_FORMAT_MASK, sad[0])) { + case HDMI_AUDIO_CODING_TYPE_PCM: + return 1 + FIELD_GET(SAD0_CHANNELS_MASK, sad[0]); + case HDMI_AUDIO_CODING_TYPE_AC3: + case HDMI_AUDIO_CODING_TYPE_DTS: + case HDMI_AUDIO_CODING_TYPE_EAC3: + return 2; + case HDMI_AUDIO_CODING_TYPE_DTS_HD: + case HDMI_AUDIO_CODING_TYPE_MLP: + return 8; + default: + /* TODO adjust for other compressed formats as well */ + return 1 + FIELD_GET(SAD0_CHANNELS_MASK, sad[0]); + } } static int eld_limit_rates(struct snd_pcm_hw_params *params, @@ -42,7 +109,7 @@ static int eld_limit_rates(struct snd_pcm_hw_params *params, * requested number of channels. */ if (c->min <= max_channels) - rate_mask |= sad[1]; + rate_mask |= sad_rate_mask(sad); } } @@ -70,7 +137,7 @@ static int eld_limit_channels(struct snd_pcm_hw_params *params, rate_mask |= BIT(i); for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) - if (rate_mask & sad[1]) + if (rate_mask & sad_rate_mask(sad)) t.max = max(t.max, sad_max_channels(sad)); }
The SADs of compressed formats contain the channel and sample rate info of the audio data inside the compressed stream, but when building constraints we must use the rates and channels used to transport the compressed streams. eg 48kHz 6ch EAC3 needs to be transmitted as a 2ch 192kHz stream. This patch fixes the constraints for the common AC3 and DTS formats, the constraints for the less common MPEG, DSD etc formats are copied directly from the info in the SADs as before as I don't have the specs and equipment to test those. Signed-off-by: Matthias Reichl <hias@horus.com> --- sound/core/pcm_drm_eld.c | 73 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 3 deletions(-)