| Message ID | 20230909114633.3193-1-hias@horus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: hdmi-codec: Fix broken channel map reporting | expand |
On Sat, Sep 09, 2023 at 01:46:33PM +0200, Matthias Reichl wrote: > Commit 4e0871333661 ("ASoC: hdmi-codec: fix channel info for > compressed formats") accidentally changed hcp->chmap_idx from > ca_id, the CEA channel allocation ID, to idx, the index to > the table of channel mappings ordered by preference. > > This resulted in wrong channel maps being reported to userspace, > eg for 5.1 "FL,FR,LFE,FC" was reported instead of the expected > "FL,FR,LFE,FC,RL,RR": Presumably this will cause a regression for people using compressed formats - isn't the fix here to make this behaviour conditional on if the format is compressed?
On Sat, Sep 09, 2023 at 01:45:38PM +0100, Mark Brown wrote: > On Sat, Sep 09, 2023 at 01:46:33PM +0200, Matthias Reichl wrote: > > Commit 4e0871333661 ("ASoC: hdmi-codec: fix channel info for > > compressed formats") accidentally changed hcp->chmap_idx from > > ca_id, the CEA channel allocation ID, to idx, the index to > > the table of channel mappings ordered by preference. > > > > This resulted in wrong channel maps being reported to userspace, > > eg for 5.1 "FL,FR,LFE,FC" was reported instead of the expected > > "FL,FR,LFE,FC,RL,RR": > > Presumably this will cause a regression for people using compressed > formats - isn't the fix here to make this behaviour conditional on if > the format is compressed? This change won't affect passthrough, the values of the HDMI audio infoframe are stored separately, in hdmi_codec_params.cea. chmap_idx in hdmi_codec_priv is only used by the get PCM channel map control - which userspace shouldn't use in the non-PCM case. Before the "fix channel info for compressed formats" change the control would always return a matching channel map, i.e. "FL FR" in the 2-channel case, regardless if PCM or non-PCM was used. When using high bitrate compressed streams, which are passed through as 8ch (containing 4 consecutive frames), this leads to a problem though as the sink might not announce support for 8 speakers (eg on TVs with 2 speakers and "virtual surround") and thus finding a channel map would fail. The "fix channel info" patch addressed this issue by only searching for a channel map in the PCM case. In the non-PCM case ca_id is set to 0 which means "FL, FR" - CTA doesn't have a separate value for "n/a" but specifies that the channel allocation only applies to PCM streams with more than 2 channels. Although the spec says the value doesn't apply to non-PCM some TVs seem to look at it and refuse to output HBR (TrueHD etc) if it's set to something other than 0. My plan was to return the exact same info via the channel map control as we set in the info frame but unfortunately I messed up and accidentally used the wrong value which this patch tries to rectify. I'm not really sure though if that's the best (or even proper) way for the channel map control to behave in the non-PCM case - it'll either return "FL, FR" or "FL, FR, UNKNOWN, UNKNOWN, UNKNOWN, UNKNOWN, UNKNOWN, UNKNOWN" with this patch. An alternative would be to set chmap_idx to HDMI_CODEC_CHMAP_IDX_UNKNOWN in the non-PCM case so the channel map control will return UNKNOWN for all channels. i.e. use this code instead: if (pcm_audio) hcp->chmap_idx = ca_id; else hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN; Any input on that topic is highly appreciated. so long & thanks, Hias
On Sun, Sep 10, 2023 at 02:14:34PM +0200, Matthias Reichl wrote: > An alternative would be to set chmap_idx to HDMI_CODEC_CHMAP_IDX_UNKNOWN > in the non-PCM case so the channel map control will return UNKNOWN for > all channels. i.e. use this code instead: > if (pcm_audio) > hcp->chmap_idx = ca_id; > else > hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN; > Any input on that topic is highly appreciated. This is going to depend a bit on what userspace is expecting which I don't have a good feel for. The above does look reasonable TBH, I'm tempted to go for that.
On Sun, Sep 10, 2023 at 02:14:34PM +0200, Matthias Reichl wrote: > An alternative would be to set chmap_idx to HDMI_CODEC_CHMAP_IDX_UNKNOWN > in the non-PCM case so the channel map control will return UNKNOWN for > all channels. i.e. use this code instead: > if (pcm_audio) > hcp->chmap_idx = ca_id; > else > hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN; > Any input on that topic is highly appreciated. Can you send this version as a real patch please?
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index 13689e718d36f..c8e48225598f8 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -531,7 +531,7 @@ static int hdmi_codec_fill_codec_params(struct snd_soc_dai *dai, hp->sample_rate = sample_rate; hp->channels = channels; - hcp->chmap_idx = idx; + hcp->chmap_idx = ca_id; return 0; }
Commit 4e0871333661 ("ASoC: hdmi-codec: fix channel info for compressed formats") accidentally changed hcp->chmap_idx from ca_id, the CEA channel allocation ID, to idx, the index to the table of channel mappings ordered by preference. This resulted in wrong channel maps being reported to userspace, eg for 5.1 "FL,FR,LFE,FC" was reported instead of the expected "FL,FR,LFE,FC,RL,RR": ~ # speaker-test -c 6 -t sine ... 0 - Front Left 3 - Front Center 1 - Front Right 2 - LFE 4 - Unknown 5 - Unknown ~ # amixer cget iface=PCM,name='Playback Channel Map' | grep ': values' : values=3,4,8,7,0,0,0,0 Revert this incorrect change so that channel maps are properly reported again. Fixes: 4e0871333661 ("ASoC: hdmi-codec: fix channel info for compressed formats") Cc: stable@vger.kernel.org Signed-off-by: Matthias Reichl <hias@horus.com> --- sound/soc/codecs/hdmi-codec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)