* [PATCH - dmix v4 0/1] pcm: dmix: Align slave_hw_ptr to slave period
@ 2018-11-12 12:47 twischer
2018-11-12 12:47 ` [PATCH - dmix v4 1/1] pcm: dmix: Align slave_hw_ptr to slave period boundary twischer
0 siblings, 1 reply; 3+ messages in thread
From: twischer @ 2018-11-12 12:47 UTC (permalink / raw)
To: patch, tiwai; +Cc: alsa-devel
Hi Takashi,
in the following we are providing the patch as requested by you:
On 11/6/18 17:32, Takashi Iwai wrote:
> I guess we may provide an option for choosing one of the following
> modes:
>
> - current default mode;
> no latency, but wakeup may be delayed, and may require 2*periods
>
> - appl_ptr round up to the next period;
> the current workaround for periods <= 2.
> This imposes the start latency.
>
> - round down to the current period start;
> No latency, but will drop a few samples at start.
>
> ... and ideally, provide a mode "auto" for choosing the appropriate
> one depending on the situation by a smart guess.
Please provide your feedback.
Best regards
Timo
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH - dmix v4 1/1] pcm: dmix: Align slave_hw_ptr to slave period boundary 2018-11-12 12:47 [PATCH - dmix v4 0/1] pcm: dmix: Align slave_hw_ptr to slave period twischer @ 2018-11-12 12:47 ` twischer 2018-11-12 16:20 ` Takashi Iwai 0 siblings, 1 reply; 3+ messages in thread From: twischer @ 2018-11-12 12:47 UTC (permalink / raw) To: patch, tiwai; +Cc: Laxmi Devi, alsa-devel From: Laxmi Devi <Laxmi.Devi@in.bosch.com> These changes are required due to the kernel commit 07b7acb51d283d8469696c906b91f1882696a4d4 ("ASoC: rsnd: update pointer more accurate") Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr is not period aligned. Therefore snd_pcm_wait() will block for a longer time as required. With these rcar driver changes the exact position of the dma is returned. During snd_pcm_start they read hw_ptr as reference, and this hw_ptr is now not period aligned, and is a little ahead over the period while it is read. Therefore when the avail is calculated during snd_pcm_wait(), it is missing the avail_min by a few frames. An additional option hw_ptr_filter is provided to dmix configuration, to allow the user to configure the slave application and hw pointer alignment at startup Signed-off-by: Laxmi Devi <Laxmi.Devi@in.bosch.com> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 091fd78..1ff7527 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -1883,6 +1883,7 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, rec->max_periods = 0; rec->var_periodsize = 0; rec->direct_memory_access = 1; + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_AUTO; /* read defaults */ if (snd_config_search(root, "defaults.pcm.dmix_max_periods", &n) >= 0) { @@ -1924,6 +1925,28 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, rec->ipc_perm = perm; continue; } + if (strcmp(id, "hw_ptr_filter") == 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, "min_latency_min_frame_drop") == 0) + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_MIN_LATENCY_MIN_FRAME_DROP; + else if (strcmp(str, "no_frame_drop") == 0) + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_NO_FRAME_DROP; + else if (strcmp(str, "exact_wakeup") == 0) + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_EXACT_WAKEUP; + else if (strcmp(str, "auto") == 0) + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_AUTO; + else { + SNDERR("The field hw_ptr_filter 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 582f055..bd0d0a1 100644 --- a/src/pcm/pcm_direct.h +++ b/src/pcm/pcm_direct.h @@ -49,6 +49,13 @@ typedef void (mix_areas_u8_t)(unsigned int size, volatile signed int *sum, size_t dst_step, size_t src_step, size_t sum_step); +typedef enum snd_pcm_direct_hw_ptr_filter { + SND_PCM_HW_PTR_FLTR_MIN_LATENCY_MIN_FRAME_DROP = 0, /* use the hw_ptr as is and do no rounding */ + SND_PCM_HW_PTR_FLTR_NO_FRAME_DROP = 1, /* round the slave_appl_ptr up to slave_period */ + SND_PCM_HW_PTR_FLTR_EXACT_WAKEUP = 2, /* round slave_hw_ptr and slave_appl_ptr down to slave_period */ + SND_PCM_HW_PTR_FLTR_AUTO = 3 /* automatic selection */ +} snd_pcm_direct_hw_ptr_filter_t; + struct slave_params { snd_pcm_format_t format; int rate; @@ -159,6 +166,7 @@ struct snd_pcm_direct { unsigned int *bindings; unsigned int recoveries; /* mirror of executed recoveries on slave */ int direct_memory_access; /* use arch-optimized buffer RW */ + snd_pcm_direct_hw_ptr_filter_t hw_ptr_filter; union { struct { int shmid_sum; /* IPC global sum ring buffer memory identification */ @@ -341,6 +349,7 @@ struct snd_pcm_direct_open_conf { int max_periods; int var_periodsize; int direct_memory_access; + snd_pcm_direct_hw_ptr_filter_t hw_ptr_filter; snd_config_t *slave; snd_config_t *bindings; }; diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index 881154e..ad1bd90 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -55,6 +55,9 @@ const char *_snd_module_pcm_dmix = ""; #define STATE_RUN_PENDING 1024 #endif +#define SEC_TO_MS 1000 /* Seconds representing in Milli seconds */ +#define LOW_LATENCY_PERIOD_TIME 10 /* slave_period time for low latency requirements in ms */ + /* * */ @@ -560,14 +563,17 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm) static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) { dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; - if (pcm->buffer_size > pcm->period_size * 2) - return; - /* If we have too litte periods, better to align the start position - * to the period boundary so that the interrupt can be handled properly - * at the right time. - */ - dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) + + if (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_NO_FRAME_DROP || + (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_AUTO && + pcm->buffer_size <= pcm->period_size * 2)) + dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) / dmix->slave_period_size) * dmix->slave_period_size; + else if (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_EXACT_WAKEUP || + (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_AUTO && + (((dmix->slave_period_size * SEC_TO_MS) / pcm->rate) < LOW_LATENCY_PERIOD_TIME))) + dmix->slave_appl_ptr = dmix->slave_hw_ptr = ((dmix->slave_hw_ptr + / dmix->slave_period_size) * dmix->slave_period_size); } static int snd_pcm_dmix_reset(snd_pcm_t *pcm) @@ -1086,6 +1092,7 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, dmix->slowptr = opts->slowptr; dmix->max_periods = opts->max_periods; dmix->var_periodsize = opts->var_periodsize; + dmix->hw_ptr_filter = opts->hw_ptr_filter; dmix->sync_ptr = snd_pcm_dmix_sync_ptr; dmix->direct_memory_access = opts->direct_memory_access; @@ -1241,6 +1248,12 @@ pcm.name { ipc_key INT # unique IPC key ipc_key_add_uid BOOL # add current uid to unique IPC key ipc_perm INT # IPC permissions (octal, default 0600) + hw_ptr_filter STR # Slave application and hw pointer alignment type + # STR can be one of the below strings : + # min_latency_min_frame_drop + # no_frame_drop + # exact_wakeup + # auto (default) slave STR # or slave { # Slave definition @@ -1273,6 +1286,25 @@ added to the value set in <code>ipc_key</code>. This will avoid the confliction of the same IPC key with different users concurrently. +<code>hw_ptr_filter</code> specifies slave application and hw pointer alignment +type. By default hw_ptr_filter is auto. Below are the possible configurations : +- min_latency_min_frame_drop : minimal latency with minimal frames dropped at + startup. But wakeup of application (return from snd_pcm_wait() or poll()) + can take up to 2 * period. +- no_frame_drop : It is guaranteed that all frames will be played at startup. But + the latency will increase upto period-1 frames. +- exact_wakeup : It is guaranteed that a wakeup will happen for each period and + frames can be written from application. But on startup upto period-1 frames + will be dropped. +- auto : Selects the best approach depending on the used period and buffer size. + If the application buffer size is < 2 * application period, no_frame_drop + will be selected to avoid under runs. If the slave_period is < 10ms we + could expect that there are low latency requirements. Therefore exact_wakeup + will be chosen to avoid long wakeup times. Such wakeup delay could otherwise + end up with Xruns in case of a dependency to another sound device + (e.g. forwarding of microphone to speaker). Else min_latency_min_frame_drop + will be chosen. + Note that the dmix plugin itself supports only a single configuration. That is, it supports only the fixed rate (default 48000), format (\c S16), channels (2), and period_time (125000). -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH - dmix v4 1/1] pcm: dmix: Align slave_hw_ptr to slave period boundary 2018-11-12 12:47 ` [PATCH - dmix v4 1/1] pcm: dmix: Align slave_hw_ptr to slave period boundary twischer @ 2018-11-12 16:20 ` Takashi Iwai 0 siblings, 0 replies; 3+ messages in thread From: Takashi Iwai @ 2018-11-12 16:20 UTC (permalink / raw) To: twischer; +Cc: Laxmi Devi, alsa-devel On Mon, 12 Nov 2018 13:47:18 +0100, <twischer@de.adit-jv.com> wrote: > > From: Laxmi Devi <Laxmi.Devi@in.bosch.com> > > These changes are required due to the kernel > commit 07b7acb51d283d8469696c906b91f1882696a4d4 > ("ASoC: rsnd: update pointer more accurate") > > Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr > is not period aligned. Therefore snd_pcm_wait() will block for a longer > time as required. > > With these rcar driver changes the exact position of the dma is returned. > During snd_pcm_start they read hw_ptr as reference, and this hw_ptr > is now not period aligned, and is a little ahead over the period while it > is read. Therefore when the avail is calculated during snd_pcm_wait(), > it is missing the avail_min by a few frames. > > An additional option hw_ptr_filter is provided to dmix configuration, > to allow the user to configure the slave application and hw pointer > alignment at startup > > > Signed-off-by: Laxmi Devi <Laxmi.Devi@in.bosch.com> Please provide your own sign-off when you submitting a patch, too. The subject line should be adjusted better to match with the change; it's about adding an option to allow alignment of hwptr reporting. About the changes: the term "hw_ptr_filter" might sound confusing. IMO, this is about the (hw)ptr alignment, not about filtering. Also, a too long option string can easily lead to a misspell :) After all, we do either "no" alignment, "roundup" or "rounddown", or "auto" choice. thanks, Takashi > diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c > index 091fd78..1ff7527 100644 > --- a/src/pcm/pcm_direct.c > +++ b/src/pcm/pcm_direct.c > @@ -1883,6 +1883,7 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, > rec->max_periods = 0; > rec->var_periodsize = 0; > rec->direct_memory_access = 1; > + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_AUTO; > > /* read defaults */ > if (snd_config_search(root, "defaults.pcm.dmix_max_periods", &n) >= 0) { > @@ -1924,6 +1925,28 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, > rec->ipc_perm = perm; > continue; > } > + if (strcmp(id, "hw_ptr_filter") == 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, "min_latency_min_frame_drop") == 0) > + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_MIN_LATENCY_MIN_FRAME_DROP; > + else if (strcmp(str, "no_frame_drop") == 0) > + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_NO_FRAME_DROP; > + else if (strcmp(str, "exact_wakeup") == 0) > + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_EXACT_WAKEUP; > + else if (strcmp(str, "auto") == 0) > + rec->hw_ptr_filter = SND_PCM_HW_PTR_FLTR_AUTO; > + else { > + SNDERR("The field hw_ptr_filter 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 582f055..bd0d0a1 100644 > --- a/src/pcm/pcm_direct.h > +++ b/src/pcm/pcm_direct.h > @@ -49,6 +49,13 @@ typedef void (mix_areas_u8_t)(unsigned int size, > volatile signed int *sum, size_t dst_step, > size_t src_step, size_t sum_step); > > +typedef enum snd_pcm_direct_hw_ptr_filter { > + SND_PCM_HW_PTR_FLTR_MIN_LATENCY_MIN_FRAME_DROP = 0, /* use the hw_ptr as is and do no rounding */ > + SND_PCM_HW_PTR_FLTR_NO_FRAME_DROP = 1, /* round the slave_appl_ptr up to slave_period */ > + SND_PCM_HW_PTR_FLTR_EXACT_WAKEUP = 2, /* round slave_hw_ptr and slave_appl_ptr down to slave_period */ > + SND_PCM_HW_PTR_FLTR_AUTO = 3 /* automatic selection */ > +} snd_pcm_direct_hw_ptr_filter_t; > + > struct slave_params { > snd_pcm_format_t format; > int rate; > @@ -159,6 +166,7 @@ struct snd_pcm_direct { > unsigned int *bindings; > unsigned int recoveries; /* mirror of executed recoveries on slave */ > int direct_memory_access; /* use arch-optimized buffer RW */ > + snd_pcm_direct_hw_ptr_filter_t hw_ptr_filter; > union { > struct { > int shmid_sum; /* IPC global sum ring buffer memory identification */ > @@ -341,6 +349,7 @@ struct snd_pcm_direct_open_conf { > int max_periods; > int var_periodsize; > int direct_memory_access; > + snd_pcm_direct_hw_ptr_filter_t hw_ptr_filter; > snd_config_t *slave; > snd_config_t *bindings; > }; > diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c > index 881154e..ad1bd90 100644 > --- a/src/pcm/pcm_dmix.c > +++ b/src/pcm/pcm_dmix.c > @@ -55,6 +55,9 @@ const char *_snd_module_pcm_dmix = ""; > #define STATE_RUN_PENDING 1024 > #endif > > +#define SEC_TO_MS 1000 /* Seconds representing in Milli seconds */ > +#define LOW_LATENCY_PERIOD_TIME 10 /* slave_period time for low latency requirements in ms */ > + > /* > * > */ > @@ -560,14 +563,17 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm) > static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) > { > dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; > - if (pcm->buffer_size > pcm->period_size * 2) > - return; > - /* If we have too litte periods, better to align the start position > - * to the period boundary so that the interrupt can be handled properly > - * at the right time. > - */ > - dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) > + > + if (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_NO_FRAME_DROP || > + (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_AUTO && > + pcm->buffer_size <= pcm->period_size * 2)) > + dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) > / dmix->slave_period_size) * dmix->slave_period_size; > + else if (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_EXACT_WAKEUP || > + (dmix->hw_ptr_filter == SND_PCM_HW_PTR_FLTR_AUTO && > + (((dmix->slave_period_size * SEC_TO_MS) / pcm->rate) < LOW_LATENCY_PERIOD_TIME))) > + dmix->slave_appl_ptr = dmix->slave_hw_ptr = ((dmix->slave_hw_ptr > + / dmix->slave_period_size) * dmix->slave_period_size); > } > > static int snd_pcm_dmix_reset(snd_pcm_t *pcm) > @@ -1086,6 +1092,7 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, > dmix->slowptr = opts->slowptr; > dmix->max_periods = opts->max_periods; > dmix->var_periodsize = opts->var_periodsize; > + dmix->hw_ptr_filter = opts->hw_ptr_filter; > dmix->sync_ptr = snd_pcm_dmix_sync_ptr; > dmix->direct_memory_access = opts->direct_memory_access; > > @@ -1241,6 +1248,12 @@ pcm.name { > ipc_key INT # unique IPC key > ipc_key_add_uid BOOL # add current uid to unique IPC key > ipc_perm INT # IPC permissions (octal, default 0600) > + hw_ptr_filter STR # Slave application and hw pointer alignment type > + # STR can be one of the below strings : > + # min_latency_min_frame_drop > + # no_frame_drop > + # exact_wakeup > + # auto (default) > slave STR > # or > slave { # Slave definition > @@ -1273,6 +1286,25 @@ added to the value set in <code>ipc_key</code>. This will > avoid the confliction of the same IPC key with different users > concurrently. > > +<code>hw_ptr_filter</code> specifies slave application and hw pointer alignment > +type. By default hw_ptr_filter is auto. Below are the possible configurations : > +- min_latency_min_frame_drop : minimal latency with minimal frames dropped at > + startup. But wakeup of application (return from snd_pcm_wait() or poll()) > + can take up to 2 * period. > +- no_frame_drop : It is guaranteed that all frames will be played at startup. But > + the latency will increase upto period-1 frames. > +- exact_wakeup : It is guaranteed that a wakeup will happen for each period and > + frames can be written from application. But on startup upto period-1 frames > + will be dropped. > +- auto : Selects the best approach depending on the used period and buffer size. > + If the application buffer size is < 2 * application period, no_frame_drop > + will be selected to avoid under runs. If the slave_period is < 10ms we > + could expect that there are low latency requirements. Therefore exact_wakeup > + will be chosen to avoid long wakeup times. Such wakeup delay could otherwise > + end up with Xruns in case of a dependency to another sound device > + (e.g. forwarding of microphone to speaker). Else min_latency_min_frame_drop > + will be chosen. > + > Note that the dmix plugin itself supports only a single configuration. > That is, it supports only the fixed rate (default 48000), format > (\c S16), channels (2), and period_time (125000). > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-12 16:20 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-12 12:47 [PATCH - dmix v4 0/1] pcm: dmix: Align slave_hw_ptr to slave period twischer 2018-11-12 12:47 ` [PATCH - dmix v4 1/1] pcm: dmix: Align slave_hw_ptr to slave period boundary twischer 2018-11-12 16:20 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.