| Message ID | 20131126104140.16e3eb87@armhf |
|---|---|
| State | New |
| Headers | show |
At Tue, 26 Nov 2013 10:41:40 +0100, Jean-Francois Moine wrote: > > This patch removes the 32 bits format which is not supported by S/PDIF > output. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> Not against the patch itself, but just found looking through it: > @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = { > .rates = SNDRV_PCM_RATE_8000_192000 | > SNDRV_PCM_RATE_CONTINUOUS | > SNDRV_PCM_RATE_KNOT, Setting both CONTINUOUS and KNOT doesn't make sense. Takashi
On Tue, 26 Nov 2013 11:20:48 +0100 Takashi Iwai <tiwai@suse.de> wrote: > Not against the patch itself, but just found looking through it: > > > @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = { > > .rates = SNDRV_PCM_RATE_8000_192000 | > > SNDRV_PCM_RATE_CONTINUOUS | > > SNDRV_PCM_RATE_KNOT, > > Setting both CONTINUOUS and KNOT doesn't make sense. Hi Takashi, I understand 'continuous', but I could not find any clear definition of 'knot'. May you explain what is its purpose? BTW, if you may help me, while looking for SNDRV_PCM_RATE_KNOT, I found this sequence in soc-pcm.c: if (codec_stream->rates & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) hw->rates |= cpu_stream->rates; if (cpu_stream->rates & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) hw->rates |= codec_stream->rates; and it seems related to the problem I have: - the cpu dai (kirkwood i2s) may generate any continuous rate 8000..192000 as shown in the patch. - the HDMI transmitter (tda998x) accepts either i2s or s/pdif audio input at any continuous rate, but when getting audio from s/pdif, the lowest rate is 22.06kHz. In the associated codec, if I define: .rates = SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | ... SNDRV_PCM_RATE_192000 | SNDRV_PCM_RATE_CONTINUOUS, audio works fine with streams at 33.075kHz (the kirkwood clock is exactly 33.075kHz). But when I want a stream at 7850Hz, the kirkwood i2s driver gets a clock at 8000Hz and the tda998x cannot do audio output. Otherwise, removing SNDRV_PCM_RATE_CONTINUOUS in the codec, I can hear the 7850Hz stream which is converted to 22.05 kHz by vlc, but when I want a stream at 33.075kHz, vlc converts it to 32kHz. So, with the commit d9ad6296ec3b4a55b "ASoC: PCM_RATE: Check for KNOT and CONTINUOUS flags", I cannot get the exact clock I want. Do you know the reason of this patch, and, if it must stay, is there any possible bypass? Thanks.
On Tue, Nov 26, 2013 at 08:09:16PM +0100, Jean-Francois Moine wrote: > Otherwise, removing SNDRV_PCM_RATE_CONTINUOUS in the codec, I can hear > the 7850Hz stream which is converted to 22.05 kHz by vlc, but when I > want a stream at 33.075kHz, vlc converts it to 32kHz. What is your fascination with doing stuff outside the specifications? The HDMI spec is quite clear: "An HDMI Source is permitted to transmit L-PCM audio data at sample rates of 32kHz, 44.1kHz, 48kHz, 88.2kHz, 96kHz, 176.4kHz or 192kHz." It goes on to say: "An HDMI Sink may accept L-PCM audio at sample rates of 32kHz, 44.1kHz, 48kHz, 88.2kHz, 96kHz, 176.4kHz or 192kHz, and should indicate these capabilities in the E-EDID data structure." That doesn't say that 8kHz, 22.05kHz etc are valid sample rates for HDMI. It may work for some devices but it's non-standard.
On 11/26/2013 08:09 PM, Jean-Francois Moine wrote: > On Tue, 26 Nov 2013 11:20:48 +0100 > Takashi Iwai <tiwai@suse.de> wrote: > >> Not against the patch itself, but just found looking through it: >> >>> @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = { >>> .rates = SNDRV_PCM_RATE_8000_192000 | >>> SNDRV_PCM_RATE_CONTINUOUS | >>> SNDRV_PCM_RATE_KNOT, >> >> Setting both CONTINUOUS and KNOT doesn't make sense. > > Hi Takashi, > > I understand 'continuous', but I could not find any clear definition of > 'knot'. May you explain what is its purpose? CONTINUOUS means that any rate between the specified min and max is fine, if no min or max is specified any rate is fine. KNOT means there are rates supported other than the standard rates defines by ALSA, but the other rates are enumerable. You'd typically specify them by explicitly listing them all and use a list constraint or you'd use one of the ratio constraints. > > BTW, if you may help me, while looking for SNDRV_PCM_RATE_KNOT, > I found this sequence in soc-pcm.c: > > if (codec_stream->rates > & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) > hw->rates |= cpu_stream->rates; > if (cpu_stream->rates > & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) > hw->rates |= codec_stream->rates; I think this code is wrong, but I don't think it is related to your problem. > > and it seems related to the problem I have: > > - the cpu dai (kirkwood i2s) may generate any continuous rate > 8000..192000 as shown in the patch. > If you set SNDRV_PCM_RATE_CONTINUOUS, the other rates flags will be ignored (IIRC). You need to explicitly specify the minimum and maximum rates if they exist. > - the HDMI transmitter (tda998x) accepts either i2s or s/pdif audio > input at any continuous rate, but when getting audio from s/pdif, the > lowest rate is 22.06kHz. > > In the associated codec, if I define: > > .rates = SNDRV_PCM_RATE_22050 | > SNDRV_PCM_RATE_32000 | > ... > SNDRV_PCM_RATE_192000 | > SNDRV_PCM_RATE_CONTINUOUS, > > audio works fine with streams at 33.075kHz (the kirkwood clock is > exactly 33.075kHz). But when I want a stream at 7850Hz, the kirkwood > i2s driver gets a clock at 8000Hz and the tda998x cannot do audio > output. > > Otherwise, removing SNDRV_PCM_RATE_CONTINUOUS in the codec, I can hear > the 7850Hz stream which is converted to 22.05 kHz by vlc, but when I > want a stream at 33.075kHz, vlc converts it to 32kHz. > > So, with the commit d9ad6296ec3b4a55b "ASoC: PCM_RATE: Check for KNOT > and CONTINUOUS flags", I cannot get the exact clock I want. > > Do you know the reason of this patch, and, if it must stay, is there > any possible bypass? In your codec driver you should specify the minimum and maximum supported rate by calling snd_pcm_hw_constraint_minmax() in the DAI startup callback. - Lars
[...] > In your codec driver you should specify the minimum and maximum supported > rate by calling snd_pcm_hw_constraint_minmax() in the DAI startup callback. It is also possible to set the min and max rate directly in the dai_driver struct.
At Tue, 26 Nov 2013 20:37:00 +0100, Lars-Peter Clausen wrote: > > On 11/26/2013 08:09 PM, Jean-Francois Moine wrote: > > On Tue, 26 Nov 2013 11:20:48 +0100 > > Takashi Iwai <tiwai@suse.de> wrote: > > > >> Not against the patch itself, but just found looking through it: > >> > >>> @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = { > >>> .rates = SNDRV_PCM_RATE_8000_192000 | > >>> SNDRV_PCM_RATE_CONTINUOUS | > >>> SNDRV_PCM_RATE_KNOT, > >> > >> Setting both CONTINUOUS and KNOT doesn't make sense. > > > > Hi Takashi, > > > > I understand 'continuous', but I could not find any clear definition of > > 'knot'. May you explain what is its purpose? > > CONTINUOUS means that any rate between the specified min and max is fine, if > no min or max is specified any rate is fine. KNOT means there are rates > supported other than the standard rates defines by ALSA, but the other rates > are enumerable. You'd typically specify them by explicitly listing them all > and use a list constraint or you'd use one of the ratio constraints. > > > > > BTW, if you may help me, while looking for SNDRV_PCM_RATE_KNOT, > > I found this sequence in soc-pcm.c: > > > > if (codec_stream->rates > > & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) > > hw->rates |= cpu_stream->rates; > > if (cpu_stream->rates > > & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) > > hw->rates |= codec_stream->rates; > > I think this code is wrong, but I don't think it is related to your problem. It's not completely wrong, but imperfect, indeed. Essentially, the rates bits are set by a line above the code snippet: hw->rates = codec_stream->rates & cpu_stream->rates; And, the code above handles exceptional rules for CONTINUOUS and KNOT. It would work if either codec or cpu DAI has KNOT or CONTINUOUS flag and another doesn't but only explicit rate bits, because CONTINUOUS does allow any rates and KNOW may allow any rates. (So, basically you need no other SNDRV_PCM_RATE_*bits if you have CONTINUOUS or KNOT bit.) And the actual rates are limited by rate_min and rate_max values. It won't work properly, however, if one side has KNOT and another has CONTINUOUS. Then it'd end up with CONTINUOUS|KNOT, which is wrong. Instead, the following should work (if I counted all possible cases properly): hw->rates = codec_stream->rates & cpu_stream->rates; if (codec_stream->rates & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) hw->rates |= cpu_stream->rates & ~SNDRV_PCM_RATE_CONTINUOUS; if (cpu_stream->rates & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS)) hw->rates |= codec_stream->rates & ~SNDRV_PCM_RATE_CONTINUOUS; The code looks tricky, so a proper comment should be added there. Takashi
On Tue, Nov 26, 2013 at 10:41:40AM +0100, Jean-Francois Moine wrote: > This patch removes the 32 bits format which is not supported by S/PDIF > output. Applied, thanks.
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index b27f826..0b18f65 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -33,6 +33,10 @@ SNDRV_PCM_FMTBIT_S24_LE | \ SNDRV_PCM_FMTBIT_S32_LE) +#define KIRKWOOD_SPDIF_FORMATS \ + (SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S24_LE) + static int kirkwood_i2s_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) { @@ -449,14 +453,14 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai[2] = { .channels_max = 2, .rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000, - .formats = KIRKWOOD_I2S_FORMATS, + .formats = KIRKWOOD_SPDIF_FORMATS, }, .capture = { .channels_min = 1, .channels_max = 2, .rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000, - .formats = KIRKWOOD_I2S_FORMATS, + .formats = KIRKWOOD_SPDIF_FORMATS, }, .ops = &kirkwood_i2s_dai_ops, }, @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = { .rates = SNDRV_PCM_RATE_8000_192000 | SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_KNOT, - .formats = KIRKWOOD_I2S_FORMATS, + .formats = KIRKWOOD_SPDIF_FORMATS, }, .capture = { .channels_min = 1, @@ -501,7 +505,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = { .rates = SNDRV_PCM_RATE_8000_192000 | SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_KNOT, - .formats = KIRKWOOD_I2S_FORMATS, + .formats = KIRKWOOD_SPDIF_FORMATS, }, .ops = &kirkwood_i2s_dai_ops, },
This patch removes the 32 bits format which is not supported by S/PDIF output. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- sound/soc/kirkwood/kirkwood-i2s.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)