monotonic raw setup seems buggy
Hi,
I am writting an alsa application and while working on pcm status timings, I noticed that setting monotonic_raw timings in sw_params while the hardware device plugin is already running with monotonic timings did not produce an error.
Follow the pcm dump which shows monotonic_raw being set on all plugin instances but the already running hw plugin instance.
Is this a bug, or am I doing something the wrong way?
Plug PCM: Linear Integer <-> Linear Float conversion PCM (S32_LE) Its setup is: stream : PLAYBACK access : RW_NONINTERLEAVED format : FLOAT_LE subformat : STD channels : 2 rate : 48000 exact rate : 48000 (48000/1) msbits : 32 buffer_size : 8192 period_size : 1024 period_time : 21333 tstamp_mode : ENABLE tstamp_type : MONOTONIC_RAW period_step : 1 avail_min : 1024 period_event : 1 start_threshold : 1 stop_threshold : 8192 silence_threshold: 0 silence_size : 0 boundary : 4611686018427387904 Slave: Direct Stream Mixing PCM Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : S32_LE subformat : STD channels : 2 rate : 48000 exact rate : 48000 (48000/1) msbits : 32 buffer_size : 8192 period_size : 1024 period_time : 21333 tstamp_mode : ENABLE tstamp_type : MONOTONIC_RAW period_step : 1 avail_min : 1024 period_event : 1 start_threshold : 1 stop_threshold : 8192 silence_threshold: 0 silence_size : 0 boundary : 4611686018427387904 Hardware PCM card 0 'HDA ATI SB' device 0 subdevice 0 Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : S32_LE subformat : STD channels : 2 rate : 48000 exact rate : 48000 (48000/1) msbits : 32 buffer_size : 8192 period_size : 1024 period_time : 21333 tstamp_mode : ENABLE tstamp_type : MONOTONIC period_step : 1 avail_min : 1024 period_event : 0 start_threshold : 1 stop_threshold : 0 silence_threshold: 0 silence_size : 0 boundary : 4611686018427387904 appl_ptr : 0 hw_ptr : 1484839806
cheers,
Hi,
On this issue, I am doing something fundamentaly wrong, but I don't see how to do it right.
While configuring a pcm, I should not use sw_params if it is a "direct-ed" (direct::dmix) real hw pcm: in pcm_direct.c, snd_pcm_direct_sw_params function is empty and it seems coherent with the fact the real hw pcm is actually shared and was probably already configured.
How do I know a pcm is actually a "pipeline" of pcms which ends up with a "direct-ed" real hw pcm? That would tell my code how to behave regarding sw_params usage.
Is this related to the "topology" and "ucm" apis?
regards,
Hey,
On Wed, 25 Mar 2020, sylvain.bertrand@gmail.com wrote:
On this issue, I am doing something fundamentaly wrong, but I don't see how to do it right.
While configuring a pcm, I should not use sw_params if it is a "direct-ed" (direct::dmix) real hw pcm: in pcm_direct.c, snd_pcm_direct_sw_params function is empty and it seems coherent with the fact the real hw pcm is actually shared and was probably already configured.
how does the problem appear in your program?
Applications should just use the ALSA PCM API and not have any special casing for different types of PCMs (unless the differences show up via the public PCM API). If applications started doing plugin specifics, writing and deploying new ALSA plugins would become much harder and kind of defeat the whole purpose of the plugin API. In case of dmix, the pcm.c:snd_pcm_sw_params() should do the right thing and your application should get the cached values.
Br, Kai
Dne 26. 03. 20 v 13:02 Kai Vehmanen napsal(a):
Hey,
On Wed, 25 Mar 2020, sylvain.bertrand@gmail.com wrote:
On this issue, I am doing something fundamentaly wrong, but I don't see how to do it right.
While configuring a pcm, I should not use sw_params if it is a "direct-ed" (direct::dmix) real hw pcm: in pcm_direct.c, snd_pcm_direct_sw_params function is empty and it seems coherent with the fact the real hw pcm is actually shared and was probably already configured.
how does the problem appear in your program?
Applications should just use the ALSA PCM API and not have any special casing for different types of PCMs (unless the differences show up via the public PCM API). If applications started doing plugin specifics, writing and deploying new ALSA plugins would become much harder and kind of defeat the whole purpose of the plugin API. In case of dmix, the pcm.c:snd_pcm_sw_params() should do the right thing and your application should get the cached values.
I agree. Also, the snd_pcm_direct_sw_params() does nothing, because the sw_params are already cached in the pcm structure (see comment). It means that the dmix (direct) plugins operates with those cached values. Just set sw_params like for any other PCM handle. The dmix uses those values (if possible).
Jaroslav
Br, Kai
On Thu, Mar 26, 2020 at 03:36:23PM +0100, Jaroslav Kysela wrote:
I agree. Also, the snd_pcm_direct_sw_params() does nothing, because the sw_params are already cached in the pcm structure (see comment). It means that the dmix (direct) plugins operates with those cached values. Just set sw_params like for any other PCM handle. The dmix uses those values (if possible).
This is the "if possible" which would impacts the way how code should do setup right, but:
Let's take the case of a classic plugin "pipeline": pcm:plug->...->direct::dmix->hw
From the top plugin (usually plug) to the direct::plugin, the "sw_params" pcm
op is usually pcm_generic.c:snd_pcm_generic_sw_params which does recurse down. This recursion down will stop once pcm_direct.c:snd_pcm_direct_sw_params is reached, then will recurse up, without error.
But pcm.c:snd_pcm_sw_params will copy anyway the provided sw_params into each recursed back pcm if the "sw_params" pcm op return no error code, which is the case.
Then looking at pcm.c:snd_pcm_sw_params_current, I get those "wrong" sw_params, then I get no way to know something went wrong.
Why "wrong", because they may significantly differ from the bottom hw plugin sw_params which some fields are used to configure the kernel driver.
for instance, a fast_op status call will recurse down to pcm_dmix.c:snd_pcm_dmix_status, which will call the hw plugin fast op status function which will use _its_ tstamp_type field for the ioctl call, but will "override" the trigger_tstamp field computed with the "wrong" sw_params tstamp_type!
It happens that the monotonic_raw and monotonic clocks can have audio significant difference. Additionally, the other sw_params field might cause similar issues.
regards,
On Thu, 26 Mar 2020 21:04:15 +0100, sylvain.bertrand@gmail.com wrote:
On Thu, Mar 26, 2020 at 03:36:23PM +0100, Jaroslav Kysela wrote:
I agree. Also, the snd_pcm_direct_sw_params() does nothing, because the sw_params are already cached in the pcm structure (see comment). It means that the dmix (direct) plugins operates with those cached values. Just set sw_params like for any other PCM handle. The dmix uses those values (if possible).
This is the "if possible" which would impacts the way how code should do setup right, but:
Let's take the case of a classic plugin "pipeline": pcm:plug->...->direct::dmix->hw
From the top plugin (usually plug) to the direct::plugin, the "sw_params" pcm
op is usually pcm_generic.c:snd_pcm_generic_sw_params which does recurse down. This recursion down will stop once pcm_direct.c:snd_pcm_direct_sw_params is reached, then will recurse up, without error.
But pcm.c:snd_pcm_sw_params will copy anyway the provided sw_params into each recursed back pcm if the "sw_params" pcm op return no error code, which is the case.
Then looking at pcm.c:snd_pcm_sw_params_current, I get those "wrong" sw_params, then I get no way to know something went wrong.
Why "wrong", because they may significantly differ from the bottom hw plugin sw_params which some fields are used to configure the kernel driver.
for instance, a fast_op status call will recurse down to pcm_dmix.c:snd_pcm_dmix_status, which will call the hw plugin fast op status function which will use _its_ tstamp_type field for the ioctl call, but will "override" the trigger_tstamp field computed with the "wrong" sw_params tstamp_type!
It happens that the monotonic_raw and monotonic clocks can have audio significant difference. Additionally, the other sw_params field might cause similar issues.
The tstamp type handling in dmix is certainly buggy, yes. It should have been restricted with the slave PCM unless it's compatible.
Takashi
Dne 27. 03. 20 v 9:08 Takashi Iwai napsal(a):
On Thu, 26 Mar 2020 21:04:15 +0100, sylvain.bertrand@gmail.com wrote:
On Thu, Mar 26, 2020 at 03:36:23PM +0100, Jaroslav Kysela wrote:
I agree. Also, the snd_pcm_direct_sw_params() does nothing, because the sw_params are already cached in the pcm structure (see comment). It means that the dmix (direct) plugins operates with those cached values. Just set sw_params like for any other PCM handle. The dmix uses those values (if possible).
This is the "if possible" which would impacts the way how code should do setup right, but:
Let's take the case of a classic plugin "pipeline": pcm:plug->...->direct::dmix->hw
From the top plugin (usually plug) to the direct::plugin, the "sw_params" pcm
op is usually pcm_generic.c:snd_pcm_generic_sw_params which does recurse down. This recursion down will stop once pcm_direct.c:snd_pcm_direct_sw_params is reached, then will recurse up, without error.
But pcm.c:snd_pcm_sw_params will copy anyway the provided sw_params into each recursed back pcm if the "sw_params" pcm op return no error code, which is the case.
Then looking at pcm.c:snd_pcm_sw_params_current, I get those "wrong" sw_params, then I get no way to know something went wrong.
Why "wrong", because they may significantly differ from the bottom hw plugin sw_params which some fields are used to configure the kernel driver.
for instance, a fast_op status call will recurse down to pcm_dmix.c:snd_pcm_dmix_status, which will call the hw plugin fast op status function which will use _its_ tstamp_type field for the ioctl call, but will "override" the trigger_tstamp field computed with the "wrong" sw_params tstamp_type!
It happens that the monotonic_raw and monotonic clocks can have audio significant difference. Additionally, the other sw_params field might cause similar issues.
The tstamp type handling in dmix is certainly buggy, yes. It should have been restricted with the slave PCM unless it's compatible.
Yes, it's a bug which should be fixed in dmix instead to use a workaround in the app. The easiest way is to return an error in set sw_params, but it may cause problems for the app which assumes that this timestamp mode can be set freely. Perhaps, we can add a timestamp translation layer (not easy, I know).
Jaroslav
On Fri, Mar 27, 2020 at 10:40:06AM +0100, Jaroslav Kysela wrote:
Yes, it's a bug which should be fixed in dmix instead to use a workaround in the app. The easiest way is to return an error in set sw_params, but it may cause problems for the app which assumes that this timestamp mode can be set freely. Perhaps, we can add a timestamp translation layer (not easy, I know).
I understand that, in the case of a shared hw, reasonable defaults should be enforce to avoid that any audio application which would be first to configure this hw and throwing some audio configuration tantrum, forces it upon all the other audio applications (this is the answer to "why has dmix some defaults?").
Speaking strictly, snd_pcm_sw_params_set_tstamp_type() has a return value, namely application code must expect a return code which could be an error code. Additionnaly an audio app using the kernel interface/a hw plugin pcm, would have to track anyway the type of timestamp clock at the time of state change: the trigger timestamp of a kernel status ioctl used the type of timestamp clock at the time of the state change, not the type of timestamp clock provided in the ioctl (btw, what about some documentation addition?).
In the use case of a shared device, it seems the right way would be to send an error back to the application (aka "making snd_pcm_sw_params_set_tstamp_type() recurse down the plugin pipeline"), because after giving some thoughts about it I don't see how it is possible to convert a timestamp from one clock type to another: backlog all adjtime deltas, plus the initial values, plus any natural drift? The linux devs in charge of time keeping may be able to answer this.
On 3/28/20 1:26 PM, sylvain.bertrand@gmail.com wrote:
On Fri, Mar 27, 2020 at 10:40:06AM +0100, Jaroslav Kysela wrote:
Yes, it's a bug which should be fixed in dmix instead to use a workaround in the app. The easiest way is to return an error in set sw_params, but it may cause problems for the app which assumes that this timestamp mode can be set freely. Perhaps, we can add a timestamp translation layer (not easy, I know).
I understand that, in the case of a shared hw, reasonable defaults should be enforce to avoid that any audio application which would be first to configure this hw and throwing some audio configuration tantrum, forces it upon all the other audio applications (this is the answer to "why has dmix some defaults?").
Speaking strictly, snd_pcm_sw_params_set_tstamp_type() has a return value, namely application code must expect a return code which could be an error code. Additionnaly an audio app using the kernel interface/a hw plugin pcm, would have to track anyway the type of timestamp clock at the time of state change: the trigger timestamp of a kernel status ioctl used the type of timestamp clock at the time of the state change, not the type of timestamp clock provided in the ioctl (btw, what about some documentation addition?).
In the use case of a shared device, it seems the right way would be to send an error back to the application (aka "making snd_pcm_sw_params_set_tstamp_type() recurse down the plugin pipeline"), because after giving some thoughts about it I don't see how it is possible to convert a timestamp from one clock type to another: backlog all adjtime deltas, plus the initial values, plus any natural drift? The linux devs in charge of time keeping may be able to answer this.
I don't think it's possible unless the timestamps are taken really close to each other. There was some work some by Chris Hall in 2016 to revisit how the conversions were done and the past taken into account is a couple of ms, see ("time: Add history to cross timestamp interface supporting slower devices").
if your device is "shared", which implies a mixer, the notion of precise timestamps is rather questionable so you might be able to get-by with local interpolation in your plug-ins. Trying a full-blown conversion of the kernel-reported time is not really useful if the mixing granularity is in the ms range or more.
FWIW you also want to take MONOTONIC_RAW with a grain of salt. In a corner case w/ long tests lasting 48 hours we saw the timestamps reported by the kernel drift over time. the drift was minor (double-digit ppb - yes parts per billion) but the fixed-point math for the counters at some point impacts the results. Reading directly the TSC from userspace and doing floating-point math bypassed the rounding errors.
On Sat, Mar 28, 2020 at 02:15:34PM -0500, Pierre-Louis Bossart wrote:
I don't think it's possible unless the timestamps are taken really close to each other. There was some work some by Chris Hall in 2016 to revisit how the conversions were done and the past taken into account is a couple of ms, see ("time: Add history to cross timestamp interface supporting slower devices").
if your device is "shared", which implies a mixer, the notion of precise timestamps is rather questionable so you might be able to get-by with local interpolation in your plug-ins. Trying a full-blown conversion of the kernel-reported time is not really useful if the mixing granularity is in the ms range or more.
FWIW you also want to take MONOTONIC_RAW with a grain of salt. In a corner case w/ long tests lasting 48 hours we saw the timestamps reported by the kernel drift over time. the drift was minor (double-digit ppb - yes parts per billion) but the fixed-point math for the counters at some point impacts the results. Reading directly the TSC from userspace and doing floating-point math bypassed the rounding errors.
This is how I got into this: I was writting a naive audio application and arrived at the point I needed timing information to do exactly that, a rough, but enough, audio linear interpolation (with ffmpeg timestamp). I naively configured alsa to use monotonic_raw, because avoiding ntp for audio timing was a good idea, and when I did sample on my side the monotonic raw clock, I realised that everything was off 100s of ms (alsa defaults to monotonic and ignores monotonic_raw setting in the case of a shared device). I followed the white rabbit, and here I stand. The cherry on top was the inconsistency about the trigger timestamp (which is not meant to be close to the other timestamps).
This pushes to fix snd_pcm_sw_params_set_tstamp_type(): recursive along a pcm plugin "pipeline" and return an error in the case of a setting difference from the one chosen by dmix. I am not confident at all since I have only a minimal perspective on alsa.
On 3/28/20 3:37 PM, sylvain.bertrand@gmail.com wrote:
On Sat, Mar 28, 2020 at 02:15:34PM -0500, Pierre-Louis Bossart wrote:
I don't think it's possible unless the timestamps are taken really close to each other. There was some work some by Chris Hall in 2016 to revisit how the conversions were done and the past taken into account is a couple of ms, see ("time: Add history to cross timestamp interface supporting slower devices").
if your device is "shared", which implies a mixer, the notion of precise timestamps is rather questionable so you might be able to get-by with local interpolation in your plug-ins. Trying a full-blown conversion of the kernel-reported time is not really useful if the mixing granularity is in the ms range or more.
FWIW you also want to take MONOTONIC_RAW with a grain of salt. In a corner case w/ long tests lasting 48 hours we saw the timestamps reported by the kernel drift over time. the drift was minor (double-digit ppb - yes parts per billion) but the fixed-point math for the counters at some point impacts the results. Reading directly the TSC from userspace and doing floating-point math bypassed the rounding errors.
This is how I got into this: I was writting a naive audio application and arrived at the point I needed timing information to do exactly that, a rough, but enough, audio linear interpolation (with ffmpeg timestamp). I naively configured alsa to use monotonic_raw, because avoiding ntp for audio timing was a good idea, and when I did sample on my side the monotonic raw clock, I realised that everything was off 100s of ms (alsa defaults to monotonic and ignores monotonic_raw setting in the case of a shared device). I followed the white rabbit, and here I stand. The cherry on top was the inconsistency about the trigger timestamp (which is not meant to be close to the other timestamps).
This pushes to fix snd_pcm_sw_params_set_tstamp_type(): recursive along a pcm plugin "pipeline" and return an error in the case of a setting difference from the one chosen by dmix. I am not confident at all since I have only a minimal perspective on alsa.
Using MONOTONIC_RAW is very nice on paper, until you realize you can't program a timer using the information. You can only read the timestamp and not really do much if you want to sleep/wait.
In practice, if you really really need super-precise information you'll get use rdtsc(), and apply you own formulas. And otherwise stick with MONOTONIC, it's rather unlikely you will ever notice the NTP changes. PulseAudio, CRAS and a number of Android HALs use MONOTONIC and nobody ever complained.
On Sat, Mar 28, 2020 at 04:34:01PM -0500, Pierre-Louis Bossart wrote:
Using MONOTONIC_RAW is very nice on paper, until you realize you can't program a timer using the information. You can only read the timestamp and not really do much if you want to sleep/wait.
In practice, if you really really need super-precise information you'll get use rdtsc(), and apply you own formulas. And otherwise stick with MONOTONIC, it's rather unlikely you will ever notice the NTP changes. PulseAudio, CRAS and a number of Android HALs use MONOTONIC and nobody ever complained.
The pb is not about using monotonic_raw, the thing is: it is documented valid to use it which I did as expected from a naive reading of the api documentation and found those issues. I can reasonably believe it will be the case for any new alsa programmer.
For my code, in the end, I think I'll use the best "audio timestamp" I can get from the status ioctl for linear interpolation with ffmpeg timestamps.
But this is off topic here.
The topic is discussing how to fix this bug, since I had to dig a bit in alsa. It appears to me the recursive fix might be a good way, since it is done for other api functions, but I am not Jaroslav Kysela neither Takashi Iwai then far from grasping all the details of alsa.
On Sat, 28 Mar 2020 23:20:21 +0100, sylvain.bertrand@gmail.com wrote:
On Sat, Mar 28, 2020 at 04:34:01PM -0500, Pierre-Louis Bossart wrote:
Using MONOTONIC_RAW is very nice on paper, until you realize you can't program a timer using the information. You can only read the timestamp and not really do much if you want to sleep/wait.
In practice, if you really really need super-precise information you'll get use rdtsc(), and apply you own formulas. And otherwise stick with MONOTONIC, it's rather unlikely you will ever notice the NTP changes. PulseAudio, CRAS and a number of Android HALs use MONOTONIC and nobody ever complained.
The pb is not about using monotonic_raw, the thing is: it is documented valid to use it which I did as expected from a naive reading of the api documentation and found those issues. I can reasonably believe it will be the case for any new alsa programmer.
For my code, in the end, I think I'll use the best "audio timestamp" I can get from the status ioctl for linear interpolation with ffmpeg timestamps.
But this is off topic here.
The topic is discussing how to fix this bug, since I had to dig a bit in alsa. It appears to me the recursive fix might be a good way, since it is done for other api functions, but I am not Jaroslav Kysela neither Takashi Iwai then far from grasping all the details of alsa.
A problem for now is that we used to allow the arbitrary parameter in sw_params because it's sw_params. Propagating an error would break this assumption, and that's the (rather only) concern when we introduce the error for an invalid tstamp type.
OTOH, although the translation of timestamp can work around this compatibility problem, it would result in an inaccurate timing that applications don't expect, either; apps set up a different tstamp type just because they want accurate timing, after all, so it'd becomes rather useless.
So, judging from the both above, I find returning an error is a better approach. Above all, it's simpler.
And for dmix, we may add a new asoundrc option to specify the tstamp type. sw_params returns an error if an incompatible tstamp type is specified. This will leave users the least possibility to use the expected tstamp type while keeping the system consistent.
thanks,
Takashi
On Sun, 29 Mar 2020 09:43:13 +0200, Takashi Iwai wrote:
On Sat, 28 Mar 2020 23:20:21 +0100, sylvain.bertrand@gmail.com wrote:
On Sat, Mar 28, 2020 at 04:34:01PM -0500, Pierre-Louis Bossart wrote:
Using MONOTONIC_RAW is very nice on paper, until you realize you can't program a timer using the information. You can only read the timestamp and not really do much if you want to sleep/wait.
In practice, if you really really need super-precise information you'll get use rdtsc(), and apply you own formulas. And otherwise stick with MONOTONIC, it's rather unlikely you will ever notice the NTP changes. PulseAudio, CRAS and a number of Android HALs use MONOTONIC and nobody ever complained.
The pb is not about using monotonic_raw, the thing is: it is documented valid to use it which I did as expected from a naive reading of the api documentation and found those issues. I can reasonably believe it will be the case for any new alsa programmer.
For my code, in the end, I think I'll use the best "audio timestamp" I can get from the status ioctl for linear interpolation with ffmpeg timestamps.
But this is off topic here.
The topic is discussing how to fix this bug, since I had to dig a bit in alsa. It appears to me the recursive fix might be a good way, since it is done for other api functions, but I am not Jaroslav Kysela neither Takashi Iwai then far from grasping all the details of alsa.
A problem for now is that we used to allow the arbitrary parameter in sw_params because it's sw_params. Propagating an error would break this assumption, and that's the (rather only) concern when we introduce the error for an invalid tstamp type.
OTOH, although the translation of timestamp can work around this compatibility problem, it would result in an inaccurate timing that applications don't expect, either; apps set up a different tstamp type just because they want accurate timing, after all, so it'd becomes rather useless.
So, judging from the both above, I find returning an error is a better approach. Above all, it's simpler.
And for dmix, we may add a new asoundrc option to specify the tstamp type. sw_params returns an error if an incompatible tstamp type is specified. This will leave users the least possibility to use the expected tstamp type while keeping the system consistent.
Below is a totally untested patch. Let me know if this works.
thanks,
Takashi
--- diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d99005461b..37dc30780623 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -991,8 +991,13 @@ int snd_pcm_direct_hw_free(snd_pcm_t *pcm ATTRIBUTE_UNUSED) return 0; }
-int snd_pcm_direct_sw_params(snd_pcm_t *pcm ATTRIBUTE_UNUSED, snd_pcm_sw_params_t * params ATTRIBUTE_UNUSED) +int snd_pcm_direct_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) { + snd_pcm_direct_t *dmix = pcm->private_data; + + if ((int)params->tstamp_type != dmix->tstamp_type) + return -EINVAL; + /* values are cached in the pcm structure */ return 0; } @@ -1318,6 +1323,15 @@ int snd_pcm_direct_initialize_slave(snd_pcm_direct_t *dmix, snd_pcm_t *spcm, str return ret; }
+ if (dmix->tstamp_type != -1) { + ret = snd_pcm_sw_params_set_tstamp_type(spcm, &sw_params, + dmix->tstamp_type); + if (ret < 0) { + SNDERR("unable to set tstamp type"); + return ret; + } + } + if (dmix->type != SND_PCM_TYPE_DMIX && dmix->type != SND_PCM_TYPE_DSHARE) goto __skip_silencing; @@ -1341,6 +1355,9 @@ int snd_pcm_direct_initialize_slave(snd_pcm_direct_t *dmix, snd_pcm_t *spcm, str return ret; }
+ /* copy back the slave setup */ + dmix->tstamp_type = sw_params.tstamp_type; + if (dmix->type == SND_PCM_TYPE_DSHARE) { const snd_pcm_channel_area_t *dst_areas; dst_areas = snd_pcm_mmap_areas(spcm); @@ -1878,6 +1895,7 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, rec->var_periodsize = 0; rec->direct_memory_access = 1; rec->hw_ptr_alignment = SND_PCM_HW_PTR_ALIGNMENT_AUTO; + rec->tstamp_type = -1;
/* read defaults */ if (snd_config_search(root, "defaults.pcm.dmix_max_periods", &n) >= 0) { @@ -1941,6 +1959,27 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
continue; } + if (strcmp(id, "tstamp_type") == 0) { + const char *str; + err = snd_config_get_string(n, &str); + if (err < 0) { + SNDERR("Invalid type for %s", id); + return -EINVAL; + } + if (strcmp(str, "default") == 0) + rec->tstamp_type = -1; + else if (strcmp(str, "gettimeofday") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY; + else if (strcmp(str, "monotonic") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_MONOTONIC; + else if (strcmp(str, "monotonic_raw") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW; + else { + SNDERR("The field tstamp_type is invalid : %s", str); + return -EINVAL; + } + continue; + } if (strcmp(id, "ipc_gid") == 0) { char *group; char *endp; diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h index 221edbe16879..63dd645ba45a 100644 --- a/src/pcm/pcm_direct.h +++ b/src/pcm/pcm_direct.h @@ -173,6 +173,7 @@ struct snd_pcm_direct { unsigned int recoveries; /* mirror of executed recoveries on slave */ int direct_memory_access; /* use arch-optimized buffer RW */ snd_pcm_direct_hw_ptr_alignment_t hw_ptr_alignment; + int tstamp_type; union { struct { int shmid_sum; /* IPC global sum ring buffer memory identification */ @@ -357,6 +358,7 @@ struct snd_pcm_direct_open_conf { int var_periodsize; int direct_memory_access; snd_pcm_direct_hw_ptr_alignment_t hw_ptr_alignment; + int tstamp_type; snd_config_t *slave; snd_config_t *bindings; }; diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index d533f40c5892..410f34a63d54 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -1237,6 +1237,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index 59448cfb5883..f497c00a360b 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -929,6 +929,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 24f472c72c8e..bbb4a344dd9d 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -780,6 +780,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 05ed935f1f16..89d4125b875d 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -928,6 +928,8 @@ int INTERNAL(snd_pcm_hw_params_set_buffer_size_last)(snd_pcm_t *pcm, snd_pcm_hw_
int snd_pcm_sw_params_set_tstamp_mode(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_tstamp_t val); int INTERNAL(snd_pcm_sw_params_get_tstamp_mode)(const snd_pcm_sw_params_t *params, snd_pcm_tstamp_t *val); +int snd_pcm_sw_params_set_tstamp_type(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_tstamp_type_t val); +int snd_pcm_sw_params_get_tstamp_type(const snd_pcm_sw_params_t *params, snd_pcm_tstamp_type_t *val); int snd_pcm_sw_params_set_avail_min(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val); int INTERNAL(snd_pcm_sw_params_get_avail_min)(const snd_pcm_sw_params_t *params, snd_pcm_uframes_t *val); int snd_pcm_sw_params_set_start_threshold(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val);
Hi,
Lol, I was working on it too. I got almost exactly the same code related to the addition of a configuration variable: - in 'struct snd_pcm_direct_open_conf' I used the type 'snd_pcm_tstamp_type_t' instead of 'int' for the added tstamp_type field. - idem for the 'struct snd_pcm_direct' tstamp_type field.
Then, I was hesistating to make snd_pcm_sw_params_set_tstamp_type recursive or/and what you did, namely check at the time of sw_params installation.
I'll start to test your patch.
regards,
On Wed, 01 Apr 2020 17:25:38 +0200, sylvain.bertrand@gmail.com wrote:
Hi,
Lol, I was working on it too. I got almost exactly the same code related to the addition of a configuration variable:
- in 'struct snd_pcm_direct_open_conf' I used the type 'snd_pcm_tstamp_type_t' instead of 'int' for the added tstamp_type field.
- idem for the 'struct snd_pcm_direct' tstamp_type field.
Heh, my initial patch was also with that strong type, but I changed it because I wanted to allow the "system default" value, which isn't represented with that type.
thanks,
Takashi
On Wed, Apr 01, 2020 at 05:59:52PM +0200, Takashi Iwai wrote:
Heh, my initial patch was also with that strong type, but I changed it because I wanted to allow the "system default" value, which isn't represented with that type.
I stand corrected. I noticed that after sending my email :(
The patch actually did not install the tstamp configuration option. I did add the installation of the option all direct based plugins (dmix/dsnoop/dshare), that with a global configuration option.
I did some testing and quickly found out some defaults for sw_params are installed in pcm_params.c:_snd_pcm_hw_params_internal from a snd_pcm_hw_params installation call.
Those defaults are defined in pcm_params.c:snd_pcm_sw_params_default which tstamp value will break in the case of a plugin pipeline ending with already running direct plugin.
It means that all calls to snd_pcm_sw_params done internally should have their sw_params values investigated to see if they work with a plugin pipeline ending with an already running direct plugin.
For the first case, in pcm_params.c:_snd_pcm_hw_params_internal, it seems that "valid" default sw_params for a plugin pipeline should be made sort of recursive or "something else".
I'll start to try to find a way to deal with with this first case and will start to investigate those internal sw_params uses.
Meanwhile, below is the modified patch.
cheers,
On Wed, Apr 01, 2020 at 08:21:26PM +0000, sylvain.bertrand@gmail.com wrote:
For the first case, in pcm_params.c:_snd_pcm_hw_params_internal, it seems that "valid" default sw_params for a plugin pipeline should be made sort of recursive or "something else".
Ok, I found what was wrong: the "recursion" is kind of already done for pcm->tstamp_type, then it seems we only need to check directly pcm(direct)->tstamp_type at sw_params installation.
I fixed the patch, and started to test.
regards,
Sylvain
---
diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf index a091b810..0e01c887 100644 --- a/src/conf/alsa.conf +++ b/src/conf/alsa.conf @@ -69,6 +69,7 @@ defaults.pcm.minperiodtime 5000 # in us defaults.pcm.ipc_key 5678293 defaults.pcm.ipc_gid audio defaults.pcm.ipc_perm 0660 +defaults.pcm.tstamp_type "default" defaults.pcm.dmix.max_periods 0 defaults.pcm.dmix.channels 2 defaults.pcm.dmix.rate 48000 diff --git a/src/conf/pcm/dmix.conf b/src/conf/pcm/dmix.conf index 7fa5c8b2..50e573da 100644 --- a/src/conf/pcm/dmix.conf +++ b/src/conf/pcm/dmix.conf @@ -56,6 +56,10 @@ pcm.!dmix { @func refer name defaults.pcm.ipc_perm } + tstamp_type { + @func refer + name defaults.pcm.tstamp_type + } slave { pcm { type hw diff --git a/src/conf/pcm/dsnoop.conf b/src/conf/pcm/dsnoop.conf index abbd44f7..f4336e5f 100644 --- a/src/conf/pcm/dsnoop.conf +++ b/src/conf/pcm/dsnoop.conf @@ -49,6 +49,10 @@ pcm.!dsnoop { @func refer name defaults.pcm.ipc_perm } + tstamp_type { + @func refer + name defaults.pcm.tstamp_type + } slave { pcm { type hw diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d99005..aa60a477 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -991,8 +991,11 @@ int snd_pcm_direct_hw_free(snd_pcm_t *pcm ATTRIBUTE_UNUSED) return 0; }
-int snd_pcm_direct_sw_params(snd_pcm_t *pcm ATTRIBUTE_UNUSED, snd_pcm_sw_params_t * params ATTRIBUTE_UNUSED) +int snd_pcm_direct_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) { + if (params->tstamp_type != pcm->tstamp_type) + return -EINVAL; + /* values are cached in the pcm structure */ return 0; } @@ -1318,6 +1321,15 @@ int snd_pcm_direct_initialize_slave(snd_pcm_direct_t *dmix, snd_pcm_t *spcm, str return ret; }
+ if (dmix->tstamp_type != -1) { + ret = snd_pcm_sw_params_set_tstamp_type(spcm, &sw_params, + dmix->tstamp_type); + if (ret < 0) { + SNDERR("unable to set tstamp type"); + return ret; + } + } + if (dmix->type != SND_PCM_TYPE_DMIX && dmix->type != SND_PCM_TYPE_DSHARE) goto __skip_silencing; @@ -1878,6 +1890,7 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, rec->var_periodsize = 0; rec->direct_memory_access = 1; rec->hw_ptr_alignment = SND_PCM_HW_PTR_ALIGNMENT_AUTO; + rec->tstamp_type = -1;
/* read defaults */ if (snd_config_search(root, "defaults.pcm.dmix_max_periods", &n) >= 0) { @@ -1941,6 +1954,27 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
continue; } + if (strcmp(id, "tstamp_type") == 0) { + const char *str; + err = snd_config_get_string(n, &str); + if (err < 0) { + SNDERR("Invalid type for %s", id); + return -EINVAL; + } + if (strcmp(str, "default") == 0) + rec->tstamp_type = -1; + else if (strcmp(str, "gettimeofday") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY; + else if (strcmp(str, "monotonic") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_MONOTONIC; + else if (strcmp(str, "monotonic_raw") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW; + else { + SNDERR("The field tstamp_type is invalid : %s", str); + return -EINVAL; + } + continue; + } if (strcmp(id, "ipc_gid") == 0) { char *group; char *endp; diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h index 221edbe1..8a236970 100644 --- a/src/pcm/pcm_direct.h +++ b/src/pcm/pcm_direct.h @@ -173,6 +173,7 @@ struct snd_pcm_direct { unsigned int recoveries; /* mirror of executed recoveries on slave */ int direct_memory_access; /* use arch-optimized buffer RW */ snd_pcm_direct_hw_ptr_alignment_t hw_ptr_alignment; + int tstamp_type; /* cached from conf, can be -1(default) on top of real types */ union { struct { int shmid_sum; /* IPC global sum ring buffer memory identification */ @@ -357,6 +358,7 @@ struct snd_pcm_direct_open_conf { int var_periodsize; int direct_memory_access; snd_pcm_direct_hw_ptr_alignment_t hw_ptr_alignment; + int tstamp_type; snd_config_t *slave; snd_config_t *bindings; }; diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index d533f40c..843fa316 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -1038,6 +1038,7 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, dmix->ipc_key = opts->ipc_key; dmix->ipc_perm = opts->ipc_perm; dmix->ipc_gid = opts->ipc_gid; + dmix->tstamp_type = opts->tstamp_type; dmix->semid = -1; dmix->shmid = -1;
@@ -1237,6 +1238,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index 59448cfb..6a99452b 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -723,6 +723,7 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, dshare->ipc_key = opts->ipc_key; dshare->ipc_perm = opts->ipc_perm; dshare->ipc_gid = opts->ipc_gid; + dshare->tstamp_type = opts->tstamp_type; dshare->semid = -1; dshare->shmid = -1;
@@ -929,6 +930,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 24f472c7..c64df381 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -591,6 +591,7 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, dsnoop->ipc_key = opts->ipc_key; dsnoop->ipc_perm = opts->ipc_perm; dsnoop->ipc_gid = opts->ipc_gid; + dsnoop->tstamp_type = opts->tstamp_type; dsnoop->semid = -1; dsnoop->shmid = -1;
@@ -780,6 +781,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 05ed935f..89d4125b 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -928,6 +928,8 @@ int INTERNAL(snd_pcm_hw_params_set_buffer_size_last)(snd_pcm_t *pcm, snd_pcm_hw_
int snd_pcm_sw_params_set_tstamp_mode(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_tstamp_t val); int INTERNAL(snd_pcm_sw_params_get_tstamp_mode)(const snd_pcm_sw_params_t *params, snd_pcm_tstamp_t *val); +int snd_pcm_sw_params_set_tstamp_type(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_tstamp_type_t val); +int snd_pcm_sw_params_get_tstamp_type(const snd_pcm_sw_params_t *params, snd_pcm_tstamp_type_t *val); int snd_pcm_sw_params_set_avail_min(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val); int INTERNAL(snd_pcm_sw_params_get_avail_min)(const snd_pcm_sw_params_t *params, snd_pcm_uframes_t *val); int snd_pcm_sw_params_set_start_threshold(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val);
No pb so far (I tested mostly monotonic_raw and default)
Can anybody test it with the pulseaudio alsa plugin?
Because building pulseaudio is an accute pain on a lean gnu/linux distro like mine.
regards,
On Mon, 06 Apr 2020 15:49:57 +0200, sylvain.bertrand@gmail.com wrote:
No pb so far (I tested mostly monotonic_raw and default)
Can anybody test it with the pulseaudio alsa plugin?
Well, using dmix/dsnoop together with PA is a very rare usage, so it's not really cared much.
If the patch worked for you, basically it's OK to accept. So, could you post the proper patch for the merge?
thanks,
Takashi
On Tue, Apr 14, 2020 at 05:18:27PM +0200, Takashi Iwai wrote:
If the patch worked for you, basically it's OK to accept.
I did not manage to make it fail. But I found another and unrelated issue, see github #41 and the patch attempt I did post on the mailing list earlier.
So, could you post the proper patch for the merge?
I'll try to format that properly.
participants (5)
-
Jaroslav Kysela -
Kai Vehmanen -
Pierre-Louis Bossart -
sylvain.bertrand＠gmail.com -
Takashi Iwai