| Message ID | 1383002356-2479-1-git-send-email-anssi.hannula@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/29 Anssi Hannula <anssi.hannula@iki.fi>: > Because of this, only the 2-channel SAD may be used if it appears before > the 8-channel SAD. Unless other SADs require otherwise, this may cause > the ALSA HDA driver to allow stereo playback only. I can confirm that the problem exists. My SADs (Onkyo TX-SR605): Format: 1 (PCM) Channels:1 Freq:0x7F (32-192) B2:0x07 (16-24b) Format: 1 (PCM) Channels:7 Freq:0x7F (32-192) B2:0x07 (16-24b) Format: 2 (AC3) Channels:7 Freq:0x07 (32-48) B2:0x50 (640?) Format: 7 (DTS) Channels:7 Freq:0x06 (44-48) B2:0xC0 (1536?) Format: 10 (EAC3) Channels:7 Freq:0x06 (44-48) B2:0x00 Format: 11 (DTS_HD) Channels:7 Freq:0x7E (44-192) B2:0x01 Format: 12 (MLP) Channels:7 Freq:0x1E (44-96) B2:0x00 Unfortunately I get only 1 emulated SAD entry for PCM (/proc/asound/card1/eld#0.0): sad0_coding_type [0x1] LPCM sad0_channels 2 sad0_rates [0x1ee0] 32000 44100 48000 88200 96000 176400 192000 sad0_bits [0xe0000] 16 20 24 So I can not play 8 channel LPCM "legally" (without forcing player to do so). > Fix the code to pick the PCM SAD with the highest number of channels, > while merging the rate masks of PCM SADs with lower amount of channels > into the additional stereo rate mask byte. Does it mean that now instead of 2 real SADs: 1) 192kHz support for 2 channels 2) 96kHz support for 8 channels you will only get: 1) 192kHz support for 8 channels ? Does it mean alsa may try to play 8channels audio at 192kHz and fail at this? That doesn't sound good, but on the other hand I can't propose anything better :| Out of curiosity, what does fglrx set in the verbs? > Technically there are even more cases to handle (multiple non-PCM SADs > of the same type, more than two PCM SADs with varying channel counts, > etc), but those have not actually been encountered in the field and > handling them would be non-trivial. > > Example affected EDID from Onkyo TX-SR674 specifying 192kHz stereo > support and 96kHz 8-channel support (and other 8-channel compressed > formats):
2013/11/1 Rafa? Mi?ecki <zajec5@gmail.com>: > 2013/10/29 Anssi Hannula <anssi.hannula@iki.fi>: >> Fix the code to pick the PCM SAD with the highest number of channels, >> while merging the rate masks of PCM SADs with lower amount of channels >> into the additional stereo rate mask byte. > > Does it mean that now instead of 2 real SADs: > 1) 192kHz support for 2 channels > 2) 96kHz support for 8 channels > you will only get: > 1) 192kHz support for 8 channels > ? > Does it mean alsa may try to play 8channels audio at 192kHz and fail at this? > That doesn't sound good, but on the other hand I can't propose > anything better :| Ahh, wait. I just noticed you're using SUPPORTED_FREQUENCIES and (different one) SUPPORTED_FREQUENCIES_STEREO there. Sorry I missed that earlier. Yeah, just code makes much more sense, I just think the logic in setting that frequency may be a bit bad... Let me think about this clearly tomorrow and I'll let you know :)
01.11.2013 01:38, Rafa? Mi?ecki kirjoitti: > 2013/10/29 Anssi Hannula <anssi.hannula@iki.fi>: >> Because of this, only the 2-channel SAD may be used if it appears before >> the 8-channel SAD. Unless other SADs require otherwise, this may cause >> the ALSA HDA driver to allow stereo playback only. > > I can confirm that the problem exists. My SADs (Onkyo TX-SR605): > Format: 1 (PCM) Channels:1 Freq:0x7F (32-192) B2:0x07 (16-24b) > Format: 1 (PCM) Channels:7 Freq:0x7F (32-192) B2:0x07 (16-24b) > Format: 2 (AC3) Channels:7 Freq:0x07 (32-48) B2:0x50 (640?) > Format: 7 (DTS) Channels:7 Freq:0x06 (44-48) B2:0xC0 (1536?) > Format: 10 (EAC3) Channels:7 Freq:0x06 (44-48) B2:0x00 > Format: 11 (DTS_HD) Channels:7 Freq:0x7E (44-192) B2:0x01 > Format: 12 (MLP) Channels:7 Freq:0x1E (44-96) B2:0x00 > > Unfortunately I get only 1 emulated SAD entry for PCM > (/proc/asound/card1/eld#0.0): > sad0_coding_type [0x1] LPCM > sad0_channels 2 > sad0_rates [0x1ee0] 32000 44100 48000 88200 96000 176400 192000 > sad0_bits [0xe0000] 16 20 24 > > So I can not play 8 channel LPCM "legally" (without forcing player to do so). > > >> Fix the code to pick the PCM SAD with the highest number of channels, >> while merging the rate masks of PCM SADs with lower amount of channels >> into the additional stereo rate mask byte. > > Does it mean that now instead of 2 real SADs: > 1) 192kHz support for 2 channels > 2) 96kHz support for 8 channels > you will only get: > 1) 192kHz support for 8 channels > ? No, the SADs will be separated again by the ATI/AMD ELD generator in hda_eld.c (as the 4th byte in the register contains the PCM stereo rate mask). I.e., assuming a receiver with 96kHz PCM multichannel limitation, with the patch the video-side sets the three standard SAD bytes according to the SAD with the highest number of channels (8ch+96kHz+24b), and the extra byte contains the or'ed rate mask of all PCM SADs (i.e. 192kHz). The audio-side then notices that 96kHz (in SAD) != 192kHz (in extra byte) and generates two SADs, one with 8ch 96kHz and one with 2ch 192kHz. > Does it mean alsa may try to play 8channels audio at 192kHz and fail at this? > That doesn't sound good, but on the other hand I can't propose > anything better :| Well, ALSA does not restrict that currently in any case (the ALSA HDMI code currently only takes the maximum channels/rate/etc into account, it doesn't check if the specific combination is allowed by SADs), but that is mostly just because nobody has added such code yet. > Out of curiosity, what does fglrx set in the verbs? I don't know, though I'd expect it to put the same values that get put there with this patch. >> Technically there are even more cases to handle (multiple non-PCM SADs >> of the same type, more than two PCM SADs with varying channel counts, >> etc), but those have not actually been encountered in the field and >> handling them would be non-trivial. >> >> Example affected EDID from Onkyo TX-SR674 specifying 192kHz stereo >> support and 96kHz 8-channel support (and other 8-channel compressed >> formats): >
2013/10/29 Anssi Hannula <anssi.hannula@iki.fi>: > Fix the code to pick the PCM SAD with the highest number of channels, > while merging the rate masks of PCM SADs with lower amount of channels > into the additional stereo rate mask byte. Don't you think that we should use SUPPORTED_FREQUENCIES_STEREO for stereo frequencies only? > if (sad->format == HDMI_AUDIO_CODING_TYPE_PCM) > + stereo_freqs |= sad->freq; I mean making that (... && sad->channels == 1)
2013/10/29 Anssi Hannula <anssi.hannula@iki.fi>: > + if (sad->channels > max_channels) { > + value = MAX_CHANNELS(sad->channels) | > + DESCRIPTOR_BYTE_2(sad->byte2) | > + SUPPORTED_FREQUENCIES(sad->freq); > + max_channels = sad->channels; > + } Just for a record. I was wondering if fglrx sets both: SUPPORTED_FREQUENCIES and SUPPORTED_FREQUENCIES_STEREO for receiver with LPCM stereo only. It does: 00005f84 07070701 (117901057) 00005f88 00000000 (0) 00005f8c 00000000 (0) 00005f90 00000000 (0) 00005f94 00000000 (0) 00005f98 00000000 (0) 00005f9c 00000000 (0) 00005fa0 00000000 (0) 00005fa4 00000000 (0) 00005fa8 00000000 (0) 00005fac 00000000 (0) 00005fb0 00000000 (0) 00005fb4 00000000 (0) 00005fb8 00000000 (0) So with the above code we'll be compatible with fglrx about that. Nice. Just wanted to note that :)
02.11.2013 03:01, Rafa? Mi?ecki kirjoitti: > 2013/10/29 Anssi Hannula <anssi.hannula@iki.fi>: >> Fix the code to pick the PCM SAD with the highest number of channels, >> while merging the rate masks of PCM SADs with lower amount of channels >> into the additional stereo rate mask byte. > > Don't you think that we should use SUPPORTED_FREQUENCIES_STEREO for > stereo frequencies only? > > >> if (sad->format == HDMI_AUDIO_CODING_TYPE_PCM) >> + stereo_freqs |= sad->freq; > > I mean making that (... && sad->channels == 1) > SAD with channels=6,freqs=32..96kHz,bits=16..24 implies that those freqs and bps are supported for all channel counts up to 6 (since it is "Max Number of channels"). Therefore the specified rates are supported in stereo mode as well and I believe should be included in the stereo bitmask. As per AMD HDA Verbs documentation the 4th byte is "Rates supported for stereo". And since those rates _are_ supported by stereo, IMO they should be there.
2013/11/2 Anssi Hannula <anssi.hannula@iki.fi>: > SAD with channels=6,freqs=32..96kHz,bits=16..24 implies that those freqs > and bps are supported for all channel counts up to 6 (since it is "Max > Number of channels"). Therefore the specified rates are supported in > stereo mode as well and I believe should be included in the stereo bitmask. > > As per AMD HDA Verbs documentation the 4th byte is "Rates supported for > stereo". And since those rates _are_ supported by stereo, IMO they > should be there. OK, that sounds sane. Thanks for explaining. Acked-by: Rafa? Mi?ecki <zajec5@gmail.com>
diff --git a/drivers/gpu/drm/radeon/dce6_afmt.c b/drivers/gpu/drm/radeon/dce6_afmt.c index 85a69d2..0a0fcee 100644 --- a/drivers/gpu/drm/radeon/dce6_afmt.c +++ b/drivers/gpu/drm/radeon/dce6_afmt.c @@ -198,20 +198,30 @@ void dce6_afmt_write_sad_regs(struct drm_encoder *encoder) for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) { u32 value = 0; + u8 stereo_freqs = 0; + int max_channels = -1; int j; for (j = 0; j < sad_count; j++) { struct cea_sad *sad = &sads[j]; if (sad->format == eld_reg_to_type[i][1]) { - value = MAX_CHANNELS(sad->channels) | - DESCRIPTOR_BYTE_2(sad->byte2) | - SUPPORTED_FREQUENCIES(sad->freq); + if (sad->channels > max_channels) { + value = MAX_CHANNELS(sad->channels) | + DESCRIPTOR_BYTE_2(sad->byte2) | + SUPPORTED_FREQUENCIES(sad->freq); + max_channels = sad->channels; + } + if (sad->format == HDMI_AUDIO_CODING_TYPE_PCM) - value |= SUPPORTED_FREQUENCIES_STEREO(sad->freq); - break; + stereo_freqs |= sad->freq; + else + break; } } + + value |= SUPPORTED_FREQUENCIES_STEREO(stereo_freqs); + WREG32_ENDPOINT(offset, eld_reg_to_type[i][0], value); } diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c b/drivers/gpu/drm/radeon/evergreen_hdmi.c index f71ce39..2a4837d 100644 --- a/drivers/gpu/drm/radeon/evergreen_hdmi.c +++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c @@ -139,20 +139,30 @@ static void evergreen_hdmi_write_sad_regs(struct drm_encoder *encoder) for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) { u32 value = 0; + u8 stereo_freqs = 0; + int max_channels = -1; int j; for (j = 0; j < sad_count; j++) { struct cea_sad *sad = &sads[j]; if (sad->format == eld_reg_to_type[i][1]) { - value = MAX_CHANNELS(sad->channels) | - DESCRIPTOR_BYTE_2(sad->byte2) | - SUPPORTED_FREQUENCIES(sad->freq); + if (sad->channels > max_channels) { + value = MAX_CHANNELS(sad->channels) | + DESCRIPTOR_BYTE_2(sad->byte2) | + SUPPORTED_FREQUENCIES(sad->freq); + max_channels = sad->channels; + } + if (sad->format == HDMI_AUDIO_CODING_TYPE_PCM) - value |= SUPPORTED_FREQUENCIES_STEREO(sad->freq); - break; + stereo_freqs |= sad->freq; + else + break; } } + + value |= SUPPORTED_FREQUENCIES_STEREO(stereo_freqs); + WREG32(eld_reg_to_type[i][0], value); } diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeon/r600_hdmi.c index f443010..da5cfa4 100644 --- a/drivers/gpu/drm/radeon/r600_hdmi.c +++ b/drivers/gpu/drm/radeon/r600_hdmi.c @@ -364,20 +364,30 @@ static void dce3_2_afmt_write_sad_regs(struct drm_encoder *encoder) for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) { u32 value = 0; + u8 stereo_freqs = 0; + int max_channels = -1; int j; for (j = 0; j < sad_count; j++) { struct cea_sad *sad = &sads[j]; if (sad->format == eld_reg_to_type[i][1]) { - value = MAX_CHANNELS(sad->channels) | - DESCRIPTOR_BYTE_2(sad->byte2) | - SUPPORTED_FREQUENCIES(sad->freq); + if (sad->channels > max_channels) { + value = MAX_CHANNELS(sad->channels) | + DESCRIPTOR_BYTE_2(sad->byte2) | + SUPPORTED_FREQUENCIES(sad->freq); + max_channels = sad->channels; + } + if (sad->format == HDMI_AUDIO_CODING_TYPE_PCM) - value |= SUPPORTED_FREQUENCIES_STEREO(sad->freq); - break; + stereo_freqs |= sad->freq; + else + break; } } + + value |= SUPPORTED_FREQUENCIES_STEREO(stereo_freqs); + WREG32(eld_reg_to_type[i][0], value); }