* [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases [not found] <526EE844.4030608@iki.fi> @ 2013-10-28 23:19 ` Anssi Hannula 2013-10-31 23:38 ` Rafał Miłecki ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Anssi Hannula @ 2013-10-28 23:19 UTC (permalink / raw) To: Alex Deucher Cc: Takashi Iwai, alsa-devel, Andre Heider, Rafał Miłecki, dri-devel The current code writing SADs to the audio registers seems to assume that there is at most a single SAD per audio format. However, that is not the case. Especially for PCM it is somewhat common for sinks to have two SADs, one for 8-channel and one for 2-channel audio, which may have different supported sample rates (i.e. the sink supports stereo audio at higher sample rates than multichannel audio). 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. 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. 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): 00ffffffffffff003dcb010000000001 ffff0103800000780a0dc9a057479827 12484c00000001010101010101010101 010101010101011d8018711c1620582c 2500c48e2100009e011d007251d01e20 6e285500c48e2100001e000000fc0054 582d53523637342020202020000000fd 00313d0f2e08000a202020202020019b 02032f724f8504030f0e07069413121e 1d1615012f097f070f1f071707503707 503f07c0834f000066030c00ffff808c 0ad08a20e02d10103e9600c48e210000 18011d80d0721c1620102c2580c48e21 00009e011d00bc52d01e20b8285540c4 8e2100001e8c0ad090204031200c4055 00c48e210000180000000000000000a8 Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi> Tested-by: Andre Heider <a.heider@gmail.com> Cc: Rafał Miłecki <zajec5@gmail.com> --- drivers/gpu/drm/radeon/dce6_afmt.c | 20 +++++++++++++++----- drivers/gpu/drm/radeon/evergreen_hdmi.c | 20 +++++++++++++++----- drivers/gpu/drm/radeon/r600_hdmi.c | 20 +++++++++++++++----- 3 files changed, 45 insertions(+), 15 deletions(-) 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); } -- 1.8.1.5 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-10-28 23:19 ` [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases Anssi Hannula @ 2013-10-31 23:38 ` Rafał Miłecki 2013-10-31 23:46 ` Rafał Miłecki 2013-10-31 23:52 ` Anssi Hannula 2013-11-02 1:01 ` Rafał Miłecki 2013-11-02 1:03 ` Rafał Miłecki 2 siblings, 2 replies; 8+ messages in thread From: Rafał Miłecki @ 2013-10-31 23:38 UTC (permalink / raw) To: Anssi Hannula; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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): -- Rafał _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-10-31 23:38 ` Rafał Miłecki @ 2013-10-31 23:46 ` Rafał Miłecki 2013-10-31 23:52 ` Anssi Hannula 1 sibling, 0 replies; 8+ messages in thread From: Rafał Miłecki @ 2013-10-31 23:46 UTC (permalink / raw) To: Anssi Hannula; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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 :) -- Rafał _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-10-31 23:38 ` Rafał Miłecki 2013-10-31 23:46 ` Rafał Miłecki @ 2013-10-31 23:52 ` Anssi Hannula 1 sibling, 0 replies; 8+ messages in thread From: Anssi Hannula @ 2013-10-31 23:52 UTC (permalink / raw) To: Rafał Miłecki; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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): > -- Anssi Hannula _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-10-28 23:19 ` [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases Anssi Hannula 2013-10-31 23:38 ` Rafał Miłecki @ 2013-11-02 1:01 ` Rafał Miłecki 2013-11-02 1:08 ` Anssi Hannula 2013-11-02 1:03 ` Rafał Miłecki 2 siblings, 1 reply; 8+ messages in thread From: Rafał Miłecki @ 2013-11-02 1:01 UTC (permalink / raw) To: Anssi Hannula; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-11-02 1:01 ` Rafał Miłecki @ 2013-11-02 1:08 ` Anssi Hannula 2013-11-02 1:15 ` Rafał Miłecki 0 siblings, 1 reply; 8+ messages in thread From: Anssi Hannula @ 2013-11-02 1:08 UTC (permalink / raw) To: Rafał Miłecki; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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. -- Anssi Hannula _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-11-02 1:08 ` Anssi Hannula @ 2013-11-02 1:15 ` Rafał Miłecki 0 siblings, 0 replies; 8+ messages in thread From: Rafał Miłecki @ 2013-11-02 1:15 UTC (permalink / raw) To: Anssi Hannula; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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> -- Rafał _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases 2013-10-28 23:19 ` [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases Anssi Hannula 2013-10-31 23:38 ` Rafał Miłecki 2013-11-02 1:01 ` Rafał Miłecki @ 2013-11-02 1:03 ` Rafał Miłecki 2 siblings, 0 replies; 8+ messages in thread From: Rafał Miłecki @ 2013-11-02 1:03 UTC (permalink / raw) To: Anssi Hannula; +Cc: Alex Deucher, Takashi Iwai, alsa-devel, dri-devel 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 :) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-02 1:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <526EE844.4030608@iki.fi>
2013-10-28 23:19 ` [PATCH] drm/radeon/audio: fix missing multichannel PCM SAD in some cases Anssi Hannula
2013-10-31 23:38 ` Rafał Miłecki
2013-10-31 23:46 ` Rafał Miłecki
2013-10-31 23:52 ` Anssi Hannula
2013-11-02 1:01 ` Rafał Miłecki
2013-11-02 1:08 ` Anssi Hannula
2013-11-02 1:15 ` Rafał Miłecki
2013-11-02 1:03 ` Rafał Miłecki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).