monotonic raw setup seems buggy

monotonic raw setup seems buggy

[sylvain.bertrand]

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,

-- 
Sylvain

sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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,

-- 
Sylvain

Re: sw_params for a direct-ed(dmix) hw pcm

[Kai Vehmanen]

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

Re: sw_params for a direct-ed(dmix) hw pcm

[Jaroslav Kysela]

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
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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,

-- 
Sylvain

Re: sw_params for a direct-ed(dmix) hw pcm

[Takashi Iwai]

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

Re: sw_params for a direct-ed(dmix) hw pcm

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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.

-- 
Sylvain

Re: sw_params for a direct-ed(dmix) hw pcm

[Pierre-Louis Bossart]

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.

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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.

-- 
Sylvain

Re: sw_params for a direct-ed(dmix) hw pcm

[Pierre-Louis Bossart]

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.

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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.

-- 
Sylvain

Re: sw_params for a direct-ed(dmix) hw pcm

[Takashi Iwai]

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

Re: sw_params for a direct-ed(dmix) hw pcm

[Takashi Iwai]

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);

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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,

-- 
Sylvain 

Re: sw_params for a direct-ed(dmix) hw pcm

[Takashi Iwai]

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

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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,

-- 
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..b6f5c731 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 = (int)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 221edbe1..63dd645b 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 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);

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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);

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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,

-- 
Sylvain

Re: sw_params for a direct-ed(dmix) hw pcm

[Takashi Iwai]

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

Re: sw_params for a direct-ed(dmix) hw pcm

[sylvain.bertrand]

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.

-- 
Sylvain