HDMI driver format channel mismatch bug

[sujith kumar reddy]

Hi Takashi,

By HBR definition macro its non-pcm right?

/* HBR should be Non-PCM, 8 channels

*/#define is_hbr_format(format) \ ((format & AC_FMT_TYPE_NON_PCM) &&
(format & AC_FMT_CHAN_MASK) == 7)

if we restrict channel_max for LPCM does it  break it for other formats in
passthrough mode?

Thanks
Sujith

On Wed, Nov 17, 2021 at 1:19 PM Takashi Iwai <tiwai at suse.de> wrote:

> On Fri, 12 Nov 2021 15:32:41 +0100,
> sujith kumar reddy wrote:
> >
> > Hi Alsa Team,
> >
> > Sound is not coming in sony tv . which has below supported formats and
> > channels.
> >
> > please find the attached  edid information and hw_params dump.
> >
> > with intel platform, same monitor populating 2 channels as max in
> hw_params
> > in dump.
> > but with AMD card, its populating as 6 channels.
> >
> >
> > When we digged the code, we found that snd_hdmi_eld_update_pcm_info   API
> > setting hinfo->channel_max as 8 and as channels max retrieved from sad
> info
> > as 6.
> >
> >
> > for LPCM , why the channel max assignment logic is not safeguared with
> > audio format check as LPCM ?
> >
> >
> > snippet code:
> >
> > /* update PCM info based on ELD */void
> > snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e,
> >                             struct hda_pcm_stream *hinfo){
> >       u32 rates;
> >       u64 formats;
> >       unsigned int maxbps;
> >       unsigned int channels_max;
> >       int i;
> >
> >       /* assume basic audio support (the basic audio flag is not in
> ELD;       *
> > however, all audio capable sinks are required to support basic         *
> > audio) */
> >       rates = SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |
> >               SNDRV_PCM_RATE_48000;
> >       formats = SNDRV_PCM_FMTBIT_S16_LE;
> >       maxbps = 16;
> >       channels_max = 2;
> >       for (i = 0; i < e->sad_count; i++) {
> >               struct cea_sad *a = &e->sad[i];
> >               rates |= a->rates;
> >               if (a->channels > channels_max)
> >                       channels_max = a->channels;
> >               if (a->format == AUDIO_CODING_TYPE_LPCM) {
> >                       if (a->sample_bits & AC_SUPPCM_BITS_20) {
> >                               formats |= SNDRV_PCM_FMTBIT_S32_LE;
> >                               if (maxbps < 20)
> >                                       maxbps = 20;
> >                       }
> >                       if (a->sample_bits & AC_SUPPCM_BITS_24) {
> >                               formats |= SNDRV_PCM_FMTBIT_S32_LE;
> >                               if (maxbps < 24)
> >                                       maxbps = 24;
> >                       }
> >               }
> >       }
> >
> >       /* restrict the parameters by the values the codec provides */
> >       hinfo->rates &= rates;
> >       hinfo->formats &= formats;
> >       hinfo->maxbps = min(hinfo->maxbps, maxbps);
> >       hinfo->channels_max = min(hinfo->channels_max, channels_max);
> >
> > >>>>>>>>>>the above statement channels_max is the maximum of channels
> supported of different formats.
> >
> > For example: the below attached edid is of sony tv. which supports two
> > formats(LPCM and AC3)
> >
> >             LPCM --->supports 2 channels
> >
> >             AC3 ----->Supports 6 channels you can see in the attached
> edid info.
> >
> >        As AMD supports LPCM ...>when we keep logs here we got
> > channels_max =6 .in which sound is not observed on tv .when we
> > hardcode to 2 channels
> >
> >        the sound is heard from monitor. As you see the above code
> > ..for loop is not distinguishing channels_max for different formats.
> >
> > }
>
> Hm, the number of channels advertised from SAD makes little sense for
> the hw_params that is rather for the PCM stream.  So the patch like
> below would work, at least, for your examples.
>
> The remaining question is whether it's only LPCM that we'd have to
> take care of the channels.  For HBR, we'd have to set 8 channels.
>
> Is HBR always tied with LPCM?  Or may it be with
> AUDIO_CODING_TYPE_DTS_HD that advertises the channels?
>
>
> thanks,
>
> Takashi
>
> --- a/sound/pci/hda/hda_eld.c
> +++ b/sound/pci/hda/hda_eld.c
> @@ -572,9 +572,9 @@ void snd_hdmi_eld_update_pcm_info(struct
> parsed_hdmi_eld *e,
>         for (i = 0; i < e->sad_count; i++) {
>                 struct cea_sad *a = &e->sad[i];
>                 rates |= a->rates;
> -               if (a->channels > channels_max)
> -                       channels_max = a->channels;
>                 if (a->format == AUDIO_CODING_TYPE_LPCM) {
> +                       if (a->channels > channels_max)
> +                               channels_max = a->channels;
>                         if (a->sample_bits & AC_SUPPCM_BITS_20) {
>                                 formats |= SNDRV_PCM_FMTBIT_S32_LE;
>                                 if (maxbps < 20)
>