[alsa-devel] AK4114 - capturing spdif input stops the stream
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
If the card cannot detect input frequency reliably (the driver developer should know), I guess it should not do the check in ak4114.c at all - I can prepare a simple patch for that.
The same will most probably apply to cards with AK4117 as the code looks the same.
I know it is a minor issue but if a user hits this problem, it will take him a lot of effort to fix his setup appropriately.
Thanks,
Pavel.
At Wed, 12 Mar 2008 23:50:08 +0100, Pavel Hofman wrote:
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
What about to call snd_ak4114_check_rate_and_errors() at PCM open? You can pass AK4114_CHECK_NO_RATE to flags argument to skip the check there, at least.
Takashi
Takashi Iwai napsal(a):
At Wed, 12 Mar 2008 23:50:08 +0100, Pavel Hofman wrote:
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
What about to call snd_ak4114_check_rate_and_errors() at PCM open? You can pass AK4114_CHECK_NO_RATE to flags argument to skip the check there, at least.
Well, I would not want to skip the check for cards with functioning rate detection. Plus snd_ak4114_check_rate_and_errors gets called periodically afterwards.
It would be great if the driver in slaved-clock mode cut its list of native sample rates to the only one currently fed to SPDIF input and detected by AK4114. A routine doing this would be called when switching clock to the slaved-clock mode and called again in snd_ak4114_check_rate_and_errors() when any change in input rate is detected (and the stream is not running). Going back to master-clock mode would restore the existing list of all the rates.
In case of input sample rate change when the PCM stream is running, the stream would be stopped. This is already implemented in the current version of snd_ak4114_check_rate_and_errors()
Unfortunately, I do not know how to change the rates list properly not to break something. If it is only about changing the HW params struct, that would be trivial.
Thanks for suggestions,
Pavel.
At Thu, 13 Mar 2008 13:49:20 +0100, Pavel Hofman wrote:
Takashi Iwai napsal(a):
At Wed, 12 Mar 2008 23:50:08 +0100, Pavel Hofman wrote:
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
What about to call snd_ak4114_check_rate_and_errors() at PCM open? You can pass AK4114_CHECK_NO_RATE to flags argument to skip the check there, at least.
Well, I would not want to skip the check for cards with functioning rate detection. Plus snd_ak4114_check_rate_and_errors gets called periodically afterwards.
Yes, but the check there is simply useless at open. It's only for running states. snd_ak4114_external_rate() would be simler, then.
It would be great if the driver in slaved-clock mode cut its list of native sample rates to the only one currently fed to SPDIF input and detected by AK4114. A routine doing this would be called when switching clock to the slaved-clock mode and called again in snd_ak4114_check_rate_and_errors() when any change in input rate is detected (and the stream is not running). Going back to master-clock mode would restore the existing list of all the rates.
In case of input sample rate change when the PCM stream is running, the stream would be stopped. This is already implemented in the current version of snd_ak4114_check_rate_and_errors()
Unfortunately, I do not know how to change the rates list properly not to break something. If it is only about changing the HW params struct, that would be trivial.
Suppose that the slave <-> master mode change doesn't happen during the stream is opened, it's relatively easy. So far, ice1724.c doesn't call spdif.ops.open/close callbacks. Call them in *_spdif_open/close functions with NULL check. Then add open hook in juli.c so that you can change rates_min and rates_max to the currently detected rate at open...
Well, it's faster to write a patch than texts.
Takashi
diff -r 547e051a88b5 pci/ice1712/ice1724.c --- a/pci/ice1712/ice1724.c Wed Mar 12 13:12:15 2008 +0100 +++ b/pci/ice1712/ice1724.c Thu Mar 13 15:03:14 2008 +0100 @@ -970,6 +970,8 @@ static int snd_vt1724_playback_spdif_ope VT1724_BUFFER_ALIGN); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, VT1724_BUFFER_ALIGN); + if (ice->spdif.ops.open) + ice->spdif.ops.open(ice, substream); return 0; }
@@ -980,6 +982,8 @@ static int snd_vt1724_playback_spdif_clo if (PRO_RATE_RESET) snd_vt1724_set_pro_rate(ice, PRO_RATE_DEFAULT, 0); ice->playback_con_substream = NULL; + if (ice->spdif.ops.close) + ice->spdif.ops.close(ice, substream);
return 0; } @@ -1002,6 +1006,8 @@ static int snd_vt1724_capture_spdif_open VT1724_BUFFER_ALIGN); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, VT1724_BUFFER_ALIGN); + if (ice->spdif.ops.open) + ice->spdif.ops.open(ice, substream); return 0; }
@@ -1012,6 +1018,8 @@ static int snd_vt1724_capture_spdif_clos if (PRO_RATE_RESET) snd_vt1724_set_pro_rate(ice, PRO_RATE_DEFAULT, 0); ice->capture_con_substream = NULL; + if (ice->spdif.ops.close) + ice->spdif.ops.close(ice, substream);
return 0; } diff -r 547e051a88b5 pci/ice1712/juli.c --- a/pci/ice1712/juli.c Wed Mar 12 13:12:15 2008 +0100 +++ b/pci/ice1712/juli.c Thu Mar 13 15:03:14 2008 +0100 @@ -75,6 +75,22 @@ static unsigned char juli_ak4114_read(vo static unsigned char juli_ak4114_read(void *private_data, unsigned char reg) { return snd_vt1724_read_i2c((struct snd_ice1712 *)private_data, AK4114_ADDR, reg); +} + +static void juli_spdif_in_open(struct snd_ice1712 *ice, + struct snd_pcm_substream *substream) +{ + struct juli_spec *spec = ice->spec; + struct snd_pcm_runtime *runtime = substream->runtime; + int rate; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + return; + rate = snd_ak4114_external_rate(spec->ak4114); + if (rate >= runtime->hw.rate_min && rate <= runtime->hw.rate_max) { + runtime->hw.rate_min = rate; + runtime->hw.rate_max = rate; + } }
/* @@ -210,6 +226,7 @@ static int __devinit juli_init(struct sn return err; } + ice->spdif.ops.open = juli_spdif_in_open; return 0; }
Takashi Iwai napsal(a):
At Thu, 13 Mar 2008 13:49:20 +0100, Pavel Hofman wrote:
Takashi Iwai napsal(a):
At Wed, 12 Mar 2008 23:50:08 +0100, Pavel Hofman wrote:
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
What about to call snd_ak4114_check_rate_and_errors() at PCM open? You can pass AK4114_CHECK_NO_RATE to flags argument to skip the check there, at least.
Well, I would not want to skip the check for cards with functioning rate detection. Plus snd_ak4114_check_rate_and_errors gets called periodically afterwards.
Yes, but the check there is simply useless at open. It's only for running states. snd_ak4114_external_rate() would be simler, then.
It would be great if the driver in slaved-clock mode cut its list of native sample rates to the only one currently fed to SPDIF input and detected by AK4114. A routine doing this would be called when switching clock to the slaved-clock mode and called again in snd_ak4114_check_rate_and_errors() when any change in input rate is detected (and the stream is not running). Going back to master-clock mode would restore the existing list of all the rates.
In case of input sample rate change when the PCM stream is running, the stream would be stopped. This is already implemented in the current version of snd_ak4114_check_rate_and_errors()
Unfortunately, I do not know how to change the rates list properly not to break something. If it is only about changing the HW params struct, that would be trivial.
Suppose that the slave <-> master mode change doesn't happen during the stream is opened, it's relatively easy. So far, ice1724.c doesn't call spdif.ops.open/close callbacks. Call them in *_spdif_open/close functions with NULL check. Then add open hook in juli.c so that you can change rates_min and rates_max to the currently detected rate at open...
Well, it's faster to write a patch than texts.
Takashi
Thanks a lot. I will test it in the evening. The code looks simple now but without your help I would be stuck forever :)
Pavel.
Takashi Iwai wrote:
At Thu, 13 Mar 2008 13:49:20 +0100, Pavel Hofman wrote:
Takashi Iwai napsal(a):
At Wed, 12 Mar 2008 23:50:08 +0100, Pavel Hofman wrote:
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
What about to call snd_ak4114_check_rate_and_errors() at PCM open? You can pass AK4114_CHECK_NO_RATE to flags argument to skip the check there, at least.
Well, I would not want to skip the check for cards with functioning rate detection. Plus snd_ak4114_check_rate_and_errors gets called periodically afterwards.
Yes, but the check there is simply useless at open. It's only for running states. snd_ak4114_external_rate() would be simler, then.
It would be great if the driver in slaved-clock mode cut its list of native sample rates to the only one currently fed to SPDIF input and detected by AK4114. A routine doing this would be called when switching clock to the slaved-clock mode and called again in snd_ak4114_check_rate_and_errors() when any change in input rate is detected (and the stream is not running). Going back to master-clock mode would restore the existing list of all the rates.
In case of input sample rate change when the PCM stream is running, the stream would be stopped. This is already implemented in the current version of snd_ak4114_check_rate_and_errors()
Unfortunately, I do not know how to change the rates list properly not to break something. If it is only about changing the HW params struct, that would be trivial.
Suppose that the slave <-> master mode change doesn't happen during the stream is opened, it's relatively easy. So far, ice1724.c doesn't call spdif.ops.open/close callbacks. Call them in *_spdif_open/close functions with NULL check. Then add open hook in juli.c so that you can change rates_min and rates_max to the currently detected rate at open...
Well, it's faster to write a patch than texts.
.......
Takashi, thanks for the patch, it works flawlessly.
Another issue - when the external clock changes rate and the stream is running (typically when starting the source playback which switches the source card (and its SPDIF-OUT) to a different frequency), the target card detects the change and the capture stream is stopped in snd_ak4114_check_rate_and_errors() by
snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING);
How should recording applications behave with stream in this mode? I would expect arecord to close, but it does nothing. Is this correct behaviour?
Thanks,
Pavel.
At Thu, 13 Mar 2008 20:18:34 +0100, Pavel Hofman wrote:
Takashi Iwai wrote:
At Thu, 13 Mar 2008 13:49:20 +0100, Pavel Hofman wrote:
Takashi Iwai napsal(a):
At Wed, 12 Mar 2008 23:50:08 +0100, Pavel Hofman wrote:
Hi,
here is my scenario:
ICE1724 card, trying to record from SPDIF input via AK4114, ICE1724 slaved to SPDIF clock from the receiver.
The card detects SPDIF input rate correctly (in my case ESI Juli where AK4114 is provided with independent clock signal to enable the rate detection). Let's say it is 192000.
Now I want to arecord the input stream, let's say in CD quality, using the plug plugin. The command fails, because of the check in ak4114.c:
res = external_rate(rcs1); if (!(flags & AK4114_CHECK_NO_RATE) && runtime && runtime->rate != res) { snd_pcm_stream_lock_irqsave(ak4114->capture_substream, _flags); if (snd_pcm_running(ak4114->capture_substream)) { printk(KERN_DEBUG "rate changed (%i <- %i)\n", runtime->rate, res); snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING); res = 1; } snd_pcm_stream_unlock_irqrestore(ak4114->capture_substream, _flags); }
The stream gets stopped because res = 192000 and runtime->rate = 44100.
The problem is that the capture device still offers all the available rates, instead of the single SPDIF input one. Thus, the plug plugin is not forced to convert from 192000 to 44100 and runtime->rate is 44100 instead of 192000.
Of course recording at 192000 works fine.
What would be the best way to force the SPDIF capture device to offer (advertise) only the current rate? This functionality would have to be applicable only to the few cards correctly detecting incoming rate (unlike most ice1724 cards without independent clock in slave mode).
What about to call snd_ak4114_check_rate_and_errors() at PCM open? You can pass AK4114_CHECK_NO_RATE to flags argument to skip the check there, at least.
Well, I would not want to skip the check for cards with functioning rate detection. Plus snd_ak4114_check_rate_and_errors gets called periodically afterwards.
Yes, but the check there is simply useless at open. It's only for running states. snd_ak4114_external_rate() would be simler, then.
It would be great if the driver in slaved-clock mode cut its list of native sample rates to the only one currently fed to SPDIF input and detected by AK4114. A routine doing this would be called when switching clock to the slaved-clock mode and called again in snd_ak4114_check_rate_and_errors() when any change in input rate is detected (and the stream is not running). Going back to master-clock mode would restore the existing list of all the rates.
In case of input sample rate change when the PCM stream is running, the stream would be stopped. This is already implemented in the current version of snd_ak4114_check_rate_and_errors()
Unfortunately, I do not know how to change the rates list properly not to break something. If it is only about changing the HW params struct, that would be trivial.
Suppose that the slave <-> master mode change doesn't happen during the stream is opened, it's relatively easy. So far, ice1724.c doesn't call spdif.ops.open/close callbacks. Call them in *_spdif_open/close functions with NULL check. Then add open hook in juli.c so that you can change rates_min and rates_max to the currently detected rate at open...
Well, it's faster to write a patch than texts.
.......
Takashi, thanks for the patch, it works flawlessly.
OK, now committed to HG tree.
Another issue - when the external clock changes rate and the stream is running (typically when starting the source playback which switches the source card (and its SPDIF-OUT) to a different frequency), the target card detects the change and the capture stream is stopped in snd_ak4114_check_rate_and_errors() by
snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING);
How should recording applications behave with stream in this mode? I would expect arecord to close, but it does nothing. Is this correct behaviour?
It's supposed to be drained then stopped. Maybe something wrong in the handling of DRAINING state in the capture direction.
Anyway, it's a bit strange situation as is. Since the sample rate is changed, you can't call simply prepare again like normal errors...
Takashi
Takashi Iwai wrote:
Takashi, thanks for the patch, it works flawlessly.
OK, now committed to HG tree.
Thanks a lot. I will just resolve the conflicts now :)
Another issue - when the external clock changes rate and the stream is running (typically when starting the source playback which switches the source card (and its SPDIF-OUT) to a different frequency), the target card detects the change and the capture stream is stopped in snd_ak4114_check_rate_and_errors() by
snd_pcm_stop(ak4114->capture_substream, SNDRV_PCM_STATE_DRAINING);
How should recording applications behave with stream in this mode? I would expect arecord to close, but it does nothing. Is this correct behaviour?
It's supposed to be drained then stopped. Maybe something wrong in the handling of DRAINING state in the capture direction.
Anyway, it's a bit strange situation as is. Since the sample rate is changed, you can't call simply prepare again like normal errors...
I would expect arecord to quit with some relevant message. In larger recording apps, e.g. Audacity, it should probably stop recording and display a warning message. No idea if the controlled stop with an appropriate error message is feasible with the existing API and code.
Thanks,
Pavel Hofman.
participants (2)
-
Pavel Hofman -
Takashi Iwai