Re: [PATCH] ASoC: hdmi-codec: Fix broken channel map reporting

[Matthias Reichl <hias@horus.com>]

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