[PATCH alsa-lib 0/4] pcm: hw: implement explicit silencing for snd_pcm_drain

[PATCH alsa-lib 0/4] pcm: hw: implement explicit silencing for snd_pcm_drain

[Jaroslav Kysela]

This is a complete API solution for the filling of the silence samples
for the drain operation. The default bahaviour is to aligns the valid
samples to one period with extra silence samples for next 1/10th of second.

Introduce SNDRV_PCM_INFO_PERFECT_DRAIN and SNDRV_PCM_HW_PARAMS_NO_DRAIN_SILENCE
flags to fully control the filling of the silence samples in the snd_pcm_drain
call. Those flags do the bidirectional setup for this operation, so the
silencing may be implemented eventually in the kernel space when there's
a demand in future. Also, some applications may not require to silence the
samples beyond the tail of the ring buffer at all.

There is also a new pcm_hw plugin configuration field allowing the 
overwrite of the default (auto) behaviour.

Related: https://lore.kernel.org/alsa-devel/20230420113324.877164-2-oswald.buddenhagen@gmx.de/
Related: https://lore.kernel.org/alsa-devel/20230405201219.2197789-2-oswald.buddenhagen@gmx.de/

Jaroslav Kysela (4):
  pcm: hw: setup explicit silencing for snd_pcm_drain by default
  pcm: hw: add drain_silence configuration keyword
  pcm: hw: introduce SNDRV_PCM_INFO_PERFECT_DRAIN
  pcm: hw: introduce SNDRV_PCM_HW_PARAMS_DRAIN_SILENCE

 include/pcm.h               |  1 +
 include/sound/uapi/asound.h |  4 ++
 src/pcm/pcm.c               | 88 +++++++++++++++++++++++++++++++------
 src/pcm/pcm_hw.c            | 74 ++++++++++++++++++++++++++++++-
 src/pcm/pcm_local.h         |  7 +++
 5 files changed, 160 insertions(+), 14 deletions(-)

-- 
2.39.2

[PATCH alsa-lib 1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default

[Jaroslav Kysela]

Some applications may not follow the period_size transfer blocks and
also the driver developers may not follow the consequeces of the
access beyond valid samples in the playback DMA buffer.

To avoid clicks, fill a little silence at the end of the playback
ring buffer when the snd_pcm_drain() is called.

Related: https://lore.kernel.org/alsa-devel/20230420113324.877164-2-oswald.buddenhagen@gmx.de/
Related: https://lore.kernel.org/alsa-devel/20230405201219.2197789-2-oswald.buddenhagen@gmx.de/
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 src/pcm/pcm.c       | 33 ++++++++++++++++++-------------
 src/pcm/pcm_hw.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++
 src/pcm/pcm_local.h |  4 ++++
 3 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 2b966d44..88b13ed4 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -6167,6 +6167,25 @@ int snd_pcm_hw_params_get_min_align(const snd_pcm_hw_params_t *params, snd_pcm_u
 	return 0;
 }
 
+#ifndef DOXYGEN
+void snd_pcm_sw_params_current_no_lock(snd_pcm_t *pcm, snd_pcm_sw_params_t *params)
+{
+	params->proto = SNDRV_PCM_VERSION;
+	params->tstamp_mode = pcm->tstamp_mode;
+	params->tstamp_type = pcm->tstamp_type;
+	params->period_step = pcm->period_step;
+	params->sleep_min = 0;
+	params->avail_min = pcm->avail_min;
+	sw_set_period_event(params, pcm->period_event);
+	params->xfer_align = 1;
+	params->start_threshold = pcm->start_threshold;
+	params->stop_threshold = pcm->stop_threshold;
+	params->silence_threshold = pcm->silence_threshold;
+	params->silence_size = pcm->silence_size;
+	params->boundary = pcm->boundary;
+}
+#endif
+
 /**
  * \brief Return current software configuration for a PCM
  * \param pcm PCM handle
@@ -6183,19 +6202,7 @@ int snd_pcm_sw_params_current(snd_pcm_t *pcm, snd_pcm_sw_params_t *params)
 		return -EIO;
 	}
 	__snd_pcm_lock(pcm); /* forced lock due to pcm field changes */
-	params->proto = SNDRV_PCM_VERSION;
-	params->tstamp_mode = pcm->tstamp_mode;
-	params->tstamp_type = pcm->tstamp_type;
-	params->period_step = pcm->period_step;
-	params->sleep_min = 0;
-	params->avail_min = pcm->avail_min;
-	sw_set_period_event(params, pcm->period_event);
-	params->xfer_align = 1;
-	params->start_threshold = pcm->start_threshold;
-	params->stop_threshold = pcm->stop_threshold;
-	params->silence_threshold = pcm->silence_threshold;
-	params->silence_size = pcm->silence_size;
-	params->boundary = pcm->boundary;
+	snd_pcm_sw_params_current_no_lock(pcm, params);
 	__snd_pcm_unlock(pcm);
 	return 0;
 }
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index daa3e1ff..d8f32bd9 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -98,6 +98,8 @@ typedef struct {
 	bool mmap_control_fallbacked;
 	struct snd_pcm_sync_ptr *sync_ptr;
 
+	bool prepare_reset_sw_params;
+
 	int period_event;
 	snd_timer_t *period_timer;
 	struct pollfd period_timer_pfd;
@@ -534,6 +536,7 @@ static int snd_pcm_hw_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t * params)
 		SYSMSG("SNDRV_PCM_IOCTL_SW_PARAMS failed (%i)", err);
 		goto out;
 	}
+	hw->prepare_reset_sw_params = false;
 	if ((snd_pcm_tstamp_type_t) params->tstamp_type != pcm->tstamp_type) {
 		if (hw->version < SNDRV_PROTOCOL_VERSION(2, 0, 12)) {
 			int on = (snd_pcm_tstamp_type_t) params->tstamp_type ==
@@ -660,7 +663,18 @@ static int snd_pcm_hw_hwsync(snd_pcm_t *pcm)
 static int snd_pcm_hw_prepare(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
+	snd_pcm_sw_params_t sw_params;
 	int fd = hw->fd, err;
+
+	if (hw->prepare_reset_sw_params) {
+		snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
+		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SW_PARAMS, sw_params) < 0) {
+			err = -errno;
+			SYSMSG("SNDRV_PCM_IOCTL_SW_PARAMS failed (%i)", err);
+			return err;
+		}
+		hw->prepare_reset_sw_params = false;
+	}
 	if (ioctl(fd, SNDRV_PCM_IOCTL_PREPARE) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_PREPARE failed (%i)", err);
@@ -718,7 +732,40 @@ static int snd_pcm_hw_drop(snd_pcm_t *pcm)
 static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
+	snd_pcm_sw_params_t sw_params;
+	snd_pcm_uframes_t silence_size;
 	int err;
+
+	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
+		goto __skip_silence;
+	/* compute end silence size, align to period size + extra time */
+	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
+	if ((pcm->boundary % pcm->period_size) == 0) {
+		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
+		if (silence_size == pcm->period_size)
+			silence_size = 0;
+	} else {
+		/* it not not easy to compute the period cross point
+		 * in this case because period is not aligned to the boundary
+		 * - use the full range (one period) in this case
+		 */
+		silence_size = pcm->period_size;
+	}
+	silence_size += pcm->rate / 10;	/* 1/10th of second */
+	if (sw_params.silence_size < silence_size) {
+		/* fill the silence soon as possible (in the bellow ioctl
+		 * or the next period wake up)
+		 */
+		sw_params.silence_threshold = pcm->buffer_size;
+		sw_params.silence_size = silence_size;
+		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SW_PARAMS, &sw_params) < 0) {
+			err = -errno;
+			SYSMSG("SNDRV_PCM_IOCTL_SW_PARAMS failed (%i)", err);
+			return err;
+		}
+		hw->prepare_reset_sw_params = true;
+	}
+__skip_silence:
 	if (ioctl(hw->fd, SNDRV_PCM_IOCTL_DRAIN) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_DRAIN failed (%i)", err);
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index ae0c44bf..4a859cd1 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -366,6 +366,8 @@ struct _snd_pcm {
 	snd1_pcm_hw_param_get_max
 #define snd_pcm_hw_param_name		\
 	snd1_pcm_hw_param_name
+#define snd_pcm_sw_params_current_no_lock \
+	snd1_pcm_sw_params_current_no_lock
 
 int snd_pcm_new(snd_pcm_t **pcmp, snd_pcm_type_t type, const char *name,
 		snd_pcm_stream_t stream, int mode);
@@ -390,6 +392,8 @@ void snd_pcm_mmap_appl_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 void snd_pcm_mmap_hw_backward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 void snd_pcm_mmap_hw_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 
+void snd_pcm_sw_params_current_no_lock(snd_pcm_t *pcm, snd_pcm_sw_params_t *params);
+
 snd_pcm_sframes_t snd_pcm_mmap_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size);
 snd_pcm_sframes_t snd_pcm_mmap_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size);
 snd_pcm_sframes_t snd_pcm_mmap_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size);
-- 
2.39.2

Re: [PATCH alsa-lib 1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default

[Oswald Buddenhagen]

On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>Some applications may not follow the period_size transfer blocks and
>also the driver developers may not follow the consequeces of the
>access beyond valid samples in the playback DMA buffer.
>
i find this way too vague.

>To avoid clicks, fill a little silence at the end of the playback

>ring buffer when the snd_pcm_drain() is called.
>
no 'the' here.
(see 
https://www.eltconcourse.com/training/inservice/lexicogrammar/article_system.html 
for more than you ever wanted to know about articles.)

>--- a/src/pcm/pcm_hw.c
>+++ b/src/pcm/pcm_hw.c
>@@ -718,7 +732,40 @@ static int snd_pcm_hw_drop(snd_pcm_t *pcm)
> static int snd_pcm_hw_drain(snd_pcm_t *pcm)
> {
> 	snd_pcm_hw_t *hw = pcm->private_data;
>+	snd_pcm_sw_params_t sw_params;
>+	snd_pcm_uframes_t silence_size;
> 	int err;
>+

>+	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
>+		goto __skip_silence;
>
arguably, you should use the inverse condition and a block instead of a 
goto.
if this is a measure to keep the indentation down, factoring it out to a 
separate snd_pcm_hw_auto_silence() function should do the job.

>+	/* compute end silence size, align to period size + extra time */
>+	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
>
if you swap these lines here, there will be less churn in the followup
patch.
in the comment, better use a colon instead of a comma.

>+	if ((pcm->boundary % pcm->period_size) == 0) {
>+		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
>+		if (silence_size == pcm->period_size)
>+			silence_size = 0;
>
you can avoid the condition by rewriting it like this:

   silence_size = pcm->period_size - ((*pcm->appl.ptr + pcm->period_size - 1) % pcm->period_size) - 1;

(it may be possible to simplify this further, but this makes my head 
hurt ...)

>+	} else {
>+		/* it not not easy to compute the period cross point
>
"it is not"
"crossing point"

>+		 * in this case because period is not aligned to the boundary
>
"the period"

>+		 * - use the full range (one period) in this case
>+		 */
>+		silence_size = pcm->period_size;
>+	}
>+	silence_size += pcm->rate / 10;	/* 1/10th of second */
>+	if (sw_params.silence_size < silence_size) {
>+		/* fill the silence soon as possible (in the bellow ioctl
>
"as soon as possible"
"in the ioctl below"

>+		 * or the next period wake up)
>+		 */
>+		sw_params.silence_threshold = pcm->buffer_size;
>+		sw_params.silence_size = silence_size;
>
so at this point i got the thought "huh, that can exceed the buffer 
size. is that ok?" ...
and yes, it is. but ...

the kernel doesn't check silence_threshold, so user space can trigger 
the snd_BUG_ON() in snd_pcm_playback_silence(). whoops.
(yes, this predates my patch.)
i'm not sure what the best way to deal with this is. anyway, different 
tree, different patch.

regards

Re: [PATCH alsa-lib 1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default

[Oswald Buddenhagen]

On Wed, May 03, 2023 at 01:20:37PM +0200, Oswald Buddenhagen wrote:
>On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>>+		 * or the next period wake up)
>>+		 */
>>+		sw_params.silence_threshold = pcm->buffer_size;
>>+		sw_params.silence_size = silence_size;
>>
>so at this point i got the thought "huh, that can exceed the buffer 
>size. is that ok?" ...
>and yes, it is. but ...
>
>the kernel doesn't check silence_threshold, so user space can trigger 
>the snd_BUG_ON() in snd_pcm_playback_silence(). whoops.
>(yes, this predates my patch.)
>i'm not sure what the best way to deal with this is. anyway, different 
>tree, different patch.

actually, that analysis is garbage, because i didn't look at enough 
context. :}

the kernel _does_ check the values in snd_pcm_sw_params(), which means 
that silence_size exceeding silence_threshold would lead to EINVAL, and 
therefore silencing being broken. this will inevitably happen with small 
buffer sizes, where the 1/10th sec extension dominates.

as snd_pcm_sw_params() checks the parameters (and snd_pcm_hw_params() 
resets the sw params to defaults, so inverse calling order cannot bypass 
it), the concern about the crash is invalid. phew.

regards

Re: [PATCH alsa-lib 1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default

[Jaroslav Kysela]

On 03. 05. 23 22:19, Oswald Buddenhagen wrote:
> On Wed, May 03, 2023 at 01:20:37PM +0200, Oswald Buddenhagen wrote:
>> On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>>> +		 * or the next period wake up)
>>> +		 */
>>> +		sw_params.silence_threshold = pcm->buffer_size;
>>> +		sw_params.silence_size = silence_size;
>>>
>> so at this point i got the thought "huh, that can exceed the buffer
>> size. is that ok?" ...
>> and yes, it is. but ...
>>
>> the kernel doesn't check silence_threshold, so user space can trigger
>> the snd_BUG_ON() in snd_pcm_playback_silence(). whoops.
>> (yes, this predates my patch.)
>> i'm not sure what the best way to deal with this is. anyway, different
>> tree, different patch.
> 
> actually, that analysis is garbage, because i didn't look at enough
> context. :}
> 
> the kernel _does_ check the values in snd_pcm_sw_params(), which means
> that silence_size exceeding silence_threshold would lead to EINVAL, and
> therefore silencing being broken. this will inevitably happen with small
> buffer sizes, where the 1/10th sec extension dominates.

Yes, I overlooked this. Thanks. Fixed in:

https://github.com/alsa-project/alsa-lib/commit/58077e2f0d896ff848f3551518b1d9fc6c97d695

					Jaroslav

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

Re: [PATCH alsa-lib 1/4] pcm: hw: setup explicit silencing for snd_pcm_drain by default

[Oswald Buddenhagen]

On Tue, May 02, 2023 at 01:50:07PM +0200, Jaroslav Kysela wrote:
>--- a/src/pcm/pcm_hw.c
>+++ b/src/pcm/pcm_hw.c
>@@ -718,7 +732,40 @@ static int snd_pcm_hw_drop(snd_pcm_t *pcm)
> static int snd_pcm_hw_drain(snd_pcm_t *pcm)
> {
> 	snd_pcm_hw_t *hw = pcm->private_data;
>+	snd_pcm_sw_params_t sw_params;
>+	snd_pcm_uframes_t silence_size;
> 	int err;
>+
>+	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
>+		goto __skip_silence;
>+	/* compute end silence size, align to period size + extra time */
>+	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
>+	if ((pcm->boundary % pcm->period_size) == 0) {
>+		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
>+		if (silence_size == pcm->period_size)
>+			silence_size = 0;
>+	} else {
>+		/* it not not easy to compute the period cross point
>+		 * in this case because period is not aligned to the boundary
>+		 * - use the full range (one period) in this case
>+		 */
>+		silence_size = pcm->period_size;
>+	}
>+	silence_size += pcm->rate / 10;	/* 1/10th of second */
>+	if (sw_params.silence_size < silence_size) {
>+		/* fill the silence soon as possible (in the bellow ioctl
>+		 * or the next period wake up)
>+		 */
>+		sw_params.silence_threshold = pcm->buffer_size;
>+		sw_params.silence_size = silence_size;
>
i was reworking my kernel patch along these lines (it's easier to deploy 
when the kernel is the only thing you're hacking on), and ran into this 
behavior:

check thresholded silence fill, sil start 0, sil fill 0, app 66000
now sil start 66000, sil fill 0
noise dist 23997
drain raw fill 0
drain extended fill 4800
frames 3
filling 3 at 18000
period elapsed
check thresholded silence fill, sil start 66000, sil fill 3, app 66000
now sil start 66000, sil fill 3
noise dist 18000
drain raw fill 0
drain extended fill 4800
frames 4800
filling 4800 at 18003
period elapsed
check thresholded silence fill, sil start 66000, sil fill 4803, app 66000
now sil start 66000, sil fill 4803
noise dist 16800
drain raw fill 0
drain extended fill 4800
frames 4800
filling 1197 at 22803
filling 3603 at 0
period elapsed
check thresholded silence fill, sil start 66000, sil fill 9603, app 66000
now sil start 66000, sil fill 9603
noise dist 15600
drain raw fill 0
drain extended fill 4800
frames 4800
filling 4800 at 3603
period elapsed
check thresholded silence fill, sil start 66000, sil fill 14403, app 66000
now sil start 66000, sil fill 14403
noise dist 6755399441055758400

what you're seeing is this: when the drain is issued, the buffer is 
initially full, and after every played period some padding is added, 
totalling way beyond what was intended.
this doesn't break anything, but it's a bit inefficient, and just ugly.

this is a result of silence_threshold being the buffer size.
and setting it to the silence_size of course doesn't work reliably when 
that is smaller than the period size.

while pondering how to fix that, my thoughts returned to my yet unaired 
gripe with the silencing parameters being anything but intuitive. would 
you mind explaining why they are like that?

why not interpret silence_size as the minimum number of playable samples 
(that is, noise_dist in the kernel code) that must be maintained at all 
times, and simply ignore silence_threshold?

unless i'm missing something, this is exactly what one would want for 
maintaining underrun resilience, which i think is the purpose of the 
thresholded silence fill mode (at least my comment updates which claim 
so were accepted).

and doing that would "magically" fix your patch.

can you think of any plausible use case that this would break?

but i guess you'll be paranoid about backwards compat anyway, so an 
alternative proposal would be using silence_threshold == ULONG_MAX to 
trigger the new semantics. of course this would have the downside that 
it wouldn't "magically" fix your code (and i suspect some other code as 
well), and kernel version dependent behavior would have to be coded for 
the (presumably) common usage.

regards

[PATCH alsa-lib 2/4] pcm: hw: add drain_silence configuration keyword

[Jaroslav Kysela]

  # Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
  [drain_silence INT]

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 src/pcm/pcm_hw.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index d8f32bd9..30ff843c 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -111,6 +111,7 @@ typedef struct {
 		int max;
 	} rates;
 	int channels;
+	int drain_silence;
 	/* for chmap */
 	unsigned int chmap_caps;
 	snd_pcm_chmap_query_t **chmap_override;
@@ -738,8 +739,16 @@ static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 
 	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
 		goto __skip_silence;
-	/* compute end silence size, align to period size + extra time */
+	if (hw->drain_silence == 0)
+		goto __skip_silence;
 	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
+	if (hw->drain_silence > 0) {
+		silence_size = hw->drain_silence;
+		if (silence_size > pcm->buffer_size)
+			silence_size = pcm->buffer_size;
+		goto __manual_silence;
+	}
+	/* compute end silence size, align to period size + extra time */
 	if ((pcm->boundary % pcm->period_size) == 0) {
 		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
 		if (silence_size == pcm->period_size)
@@ -752,6 +761,7 @@ static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 		silence_size = pcm->period_size;
 	}
 	silence_size += pcm->rate / 10;	/* 1/10th of second */
+__manual_silence:
 	if (sw_params.silence_size < silence_size) {
 		/* fill the silence soon as possible (in the bellow ioctl
 		 * or the next period wake up)
@@ -1818,6 +1828,7 @@ pcm.name {
 	[rate INT]		# Restrict only to the given rate
 	  or [rate [INT INT]]	# Restrict only to the given rate range (min max)
 	[chmap MAP]		# Override channel maps; MAP is a string array
+	[drain_silence INT]	# Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
 }
 \endcode
 
@@ -1850,7 +1861,7 @@ int _snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 	long card = -1, device = 0, subdevice = -1;
 	const char *str;
 	int err, sync_ptr_ioctl = 0;
-	int min_rate = 0, max_rate = 0, channels = 0;
+	int min_rate = 0, max_rate = 0, channels = 0, drain_silence = -1;
 	snd_pcm_format_t format = SND_PCM_FORMAT_UNKNOWN;
 	snd_config_t *n;
 	int nonblock = 1; /* non-block per default */
@@ -1991,6 +2002,16 @@ int _snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 			}
 			continue;
 		}
+		if (strcmp(id, "drain_silence") == 0) {
+			long val;
+			err = snd_config_get_integer(n, &val);
+			if (err < 0) {
+				SNDERR("Invalid type for %s", id);
+				goto fail;
+			}
+			drain_silence = val;
+			continue;
+		}
 		SNDERR("Unknown field %s", id);
 		err = -EINVAL;
 		goto fail;
@@ -2033,6 +2054,7 @@ int _snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 	}
 	if (chmap)
 		hw->chmap_override = chmap;
+	hw->drain_silence = drain_silence;
 
 	return 0;
 
-- 
2.39.2

Re: [PATCH alsa-lib 2/4] pcm: hw: add drain_silence configuration keyword

[Oswald Buddenhagen]

On Tue, May 02, 2023 at 01:50:08PM +0200, Jaroslav Kysela wrote:
>  # Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
>  [drain_silence INT]
>
i find this wholly inadequate as a description.
specifically, it's missing a motivation.

and how would one use this in a meaningful way, given that the actual 
silence size is dependent on the period size and preferably the pointer 
alignment?

what i could imagine _hypothetically_ making sense is making the 1/10th 
sec "overshoot" configurable, as it's hardware-dependent. but in 
practice, i don't see how that would be actually useful, as the cost of 
doing too much is negligible, and the default you chose seems more than 
safe enough.

regards

Re: [PATCH alsa-lib 2/4] pcm: hw: add drain_silence configuration keyword

[Jaroslav Kysela]

On 03. 05. 23 13:24, Oswald Buddenhagen wrote:
> On Tue, May 02, 2023 at 01:50:08PM +0200, Jaroslav Kysela wrote:
>>   # Add silence in drain (-1 = auto /default/, 0 = off, > 0 silenced frames)
>>   [drain_silence INT]
>>
> i find this wholly inadequate as a description.
> specifically, it's missing a motivation.
> 
> and how would one use this in a meaningful way, given that the actual
> silence size is dependent on the period size and preferably the pointer
> alignment?
> 
> what i could imagine _hypothetically_ making sense is making the 1/10th
> sec "overshoot" configurable, as it's hardware-dependent. but in
> practice, i don't see how that would be actually useful, as the cost of
> doing too much is negligible, and the default you chose seems more than
> safe enough.

The positive value is a bit bonus. I just picked an easy understandable way. 
But looking to this issue for the second time, I changed the meaning for the 
positive value to milliseconds. In this way, it's time/rate related.

The main purpose for this option is to turn the code off. The other silence 
size calculation schemes may be described by another configuration field in 
future on demand.

Thanks for the review of all patches - I picked some proposals and pushed 
changes to the alsa-lib repository:

https://github.com/alsa-project/alsa-lib/commits/master

					Jaroslav

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

Re: [PATCH alsa-lib 2/4] pcm: hw: add drain_silence configuration keyword

[Oswald Buddenhagen]

On Wed, May 03, 2023 at 04:22:03PM +0200, Jaroslav Kysela wrote:
>On 03. 05. 23 13:24, Oswald Buddenhagen wrote:
>> what i could imagine _hypothetically_ making sense is making the 
>> 1/10th
>> sec "overshoot" configurable, as it's hardware-dependent. but in
>> practice, i don't see how that would be actually useful, as the cost of
>> doing too much is negligible, and the default you chose seems more than
>> safe enough.
>
>The positive value is a bit bonus. I just picked an easy understandable way. 
>But looking to this issue for the second time, I changed the meaning for the 
>positive value to milliseconds. In this way, it's time/rate related.
>
i think it's a bad idea to add "bonus" features that have no clear use 
case. it's basically dead code, and you can't use these values for 
something actually useful later.

>Thanks for the review of all patches - I picked some proposals and 
>pushed changes to the alsa-lib repository:
>
well, and you ignored some of them for no obvious reason.

generally, i don't think maintainers should be exempt from replying to 
comments and posting v2+ patchsets.

regards

[PATCH alsa-lib 3/4] pcm: hw: introduce SNDRV_PCM_INFO_PERFECT_DRAIN

[Jaroslav Kysela]

The driver may not require to touch the sample stream
for the drain operation at all. Handle this situation
in alsa-lib.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 include/pcm.h               |  1 +
 include/sound/uapi/asound.h |  1 +
 src/pcm/pcm.c               | 23 +++++++++++++++++++++++
 src/pcm/pcm_hw.c            |  4 +++-
 src/pcm/pcm_local.h         |  2 ++
 5 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/pcm.h b/include/pcm.h
index b5a514fa..f15462d9 100644
--- a/include/pcm.h
+++ b/include/pcm.h
@@ -722,6 +722,7 @@ int snd_pcm_hw_params_is_half_duplex(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_is_joint_duplex(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_can_sync_start(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_can_disable_period_wakeup(const snd_pcm_hw_params_t *params);
+int snd_pcm_hw_params_does_perfect_drain(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_supports_audio_wallclock_ts(const snd_pcm_hw_params_t *params); /* deprecated, use audio_ts_type */
 int snd_pcm_hw_params_supports_audio_ts_type(const snd_pcm_hw_params_t *params, int type);
 int snd_pcm_hw_params_get_rate_numden(const snd_pcm_hw_params_t *params,
diff --git a/include/sound/uapi/asound.h b/include/sound/uapi/asound.h
index fc18c024..0b8834f2 100644
--- a/include/sound/uapi/asound.h
+++ b/include/sound/uapi/asound.h
@@ -281,6 +281,7 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define SNDRV_PCM_INFO_DOUBLE		0x00000004	/* Double buffering needed for PCM start/stop */
 #define SNDRV_PCM_INFO_BATCH		0x00000010	/* double buffering */
 #define SNDRV_PCM_INFO_SYNC_APPLPTR	0x00000020	/* need the explicit sync of appl_ptr update */
+#define SNDRV_PCM_INFO_PERFECT_DRAIN	0x00000040	/* silencing at the end of stream is not required */
 #define SNDRV_PCM_INFO_INTERLEAVED	0x00000100	/* channels are interleaved */
 #define SNDRV_PCM_INFO_NONINTERLEAVED	0x00000200	/* channels are not interleaved */
 #define SNDRV_PCM_INFO_COMPLEX		0x00000400	/* complex frame organization (mmap only) */
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 88b13ed4..099d83ee 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -3707,6 +3707,29 @@ int snd_pcm_hw_params_can_disable_period_wakeup(const snd_pcm_hw_params_t *param
 	return !!(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP);
 }
 
+/**
+ * \brief Check if hardware does perfect drain
+ * \param params Configuration space
+ * \retval 0 Hardware doesn't do perfect drain
+ * \retval 1 Hardware does perfect drain
+ *
+ * This function should only be called when the configuration space
+ * contains a single configuration. Call #snd_pcm_hw_params to choose
+ * a single configuration from the configuration space.
+ *
+ * The perfect drain means that the hardware does not use samples
+ * beyond the stream application pointer.
+ */
+int snd_pcm_hw_params_does_perfect_drain(const snd_pcm_hw_params_t *params)
+{
+	assert(params);
+	if (CHECK_SANITY(params->info == ~0U)) {
+		SNDMSG("invalid PCM info field");
+		return 0; /* FIXME: should be a negative error? */
+	}
+	return !!(params->info & SNDRV_PCM_INFO_PERFECT_DRAIN);
+}
+
 /**
  * \brief Check if hardware supports audio wallclock timestamps
  * \param params Configuration space
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 30ff843c..ea0c2ef2 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -99,6 +99,7 @@ typedef struct {
 	struct snd_pcm_sync_ptr *sync_ptr;
 
 	bool prepare_reset_sw_params;
+	bool perfect_drain;
 
 	int period_event;
 	snd_timer_t *period_timer;
@@ -398,6 +399,7 @@ static int snd_pcm_hw_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
 	params->info &= ~0xf0000000;
 	if (pcm->tstamp_type != SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY)
 		params->info |= SND_PCM_INFO_MONOTONIC;
+	hw->perfect_drain = !!(params->info & SND_PCM_INFO_PERFECT_DRAIN);
 	return query_status_data(hw);
 }
 
@@ -739,7 +741,7 @@ static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 
 	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
 		goto __skip_silence;
-	if (hw->drain_silence == 0)
+	if (hw->drain_silence == 0 || hw->perfect_drain)
 		goto __skip_silence;
 	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
 	if (hw->drain_silence > 0) {
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 4a859cd1..b039dda0 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -79,6 +79,8 @@
 #define SND_PCM_INFO_DOUBLE SNDRV_PCM_INFO_DOUBLE
 /** device transfers samples in batch */
 #define SND_PCM_INFO_BATCH SNDRV_PCM_INFO_BATCH
+/** device does perfect drain (silencing not required) */
+#define SND_PCM_INFO_PERFECT_DRAIN SNDRV_PCM_INFO_PERFECT_DRAIN
 /** device accepts interleaved samples */
 #define SND_PCM_INFO_INTERLEAVED SNDRV_PCM_INFO_INTERLEAVED
 /** device accepts non-interleaved samples */
-- 
2.39.2

Re: [PATCH alsa-lib 3/4] pcm: hw: introduce SNDRV_PCM_INFO_PERFECT_DRAIN

[Oswald Buddenhagen]

On Tue, May 02, 2023 at 01:50:09PM +0200, Jaroslav Kysela wrote:
>The driver may not require to touch the sample stream
>
"touching"

>for the drain operation at all.
>Handle this situation in alsa-lib.
>
this is weird without already knowing the context. i'd instead write:

   Handle the driver informing us that it is not necessary to set up 
   silencing upon playback draining. This will be the case for drivers 
   which are guaranteed to not read any samples beyond the application 
   pointer.

>--- a/src/pcm/pcm.c
>+++ b/src/pcm/pcm.c
>@@ -3707,6 +3707,29 @@ int snd_pcm_hw_params_can_disable_period_wakeup(const snd_pcm_hw_params_t *param
> 	return !!(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP);
> }
> 
>+/**
>+ * \brief Check if hardware does perfect drain
>
"(is a) perfect drain" vs. "does draining".

>+ * \param params Configuration space
>+ * \retval 0 Hardware doesn't do perfect drain
>+ * \retval 1 Hardware does perfect drain
>+ *
>+ * This function should only be called when the configuration space
>
"should be called only when"

>+ * contains a single configuration. Call #snd_pcm_hw_params to choose
>+ * a single configuration from the configuration space.
>+ *
>+ * The perfect drain means that the hardware does not use samples
>
see above. i guess one way to write it here would be

   "Perfect drain" means [...]

>+ * beyond the stream application pointer.
>+ */

regards

Re: [PATCH alsa-lib 3/4] pcm: hw: introduce SNDRV_PCM_INFO_PERFECT_DRAIN

[Takashi Iwai]

On Tue, 02 May 2023 13:50:09 +0200,
Jaroslav Kysela wrote:
> 
> The driver may not require to touch the sample stream
> for the drain operation at all. Handle this situation
> in alsa-lib.
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

Ideally speaking, the checks and the setups of those new bits should
be coupled with the PCM protocol version check (and the version
bump).

But it seems that you've already applied the series, and practically
seen, those bits should be either not set or harmless, so let's cross
fingers.


thanks,

Takashi

Re: [PATCH alsa-lib 3/4] pcm: hw: introduce SNDRV_PCM_INFO_PERFECT_DRAIN

[Jaroslav Kysela]

On 04. 05. 23 10:18, Takashi Iwai wrote:
> On Tue, 02 May 2023 13:50:09 +0200,
> Jaroslav Kysela wrote:
>>
>> The driver may not require to touch the sample stream
>> for the drain operation at all. Handle this situation
>> in alsa-lib.
>>
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> 
> Ideally speaking, the checks and the setups of those new bits should
> be coupled with the PCM protocol version check (and the version
> bump).
> 
> But it seems that you've already applied the series, and practically
> seen, those bits should be either not set or harmless, so let's cross
> fingers.

Exactly, the current kernel code should skip those new flags, so they are used 
in alsa-lib only. It's just something like a "reservation" for the kernel 
space until things are really used there. We can bump the protocol version 
later (perhaps with other changes).

			Jaroslav

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

Re: [PATCH alsa-lib 3/4] pcm: hw: introduce SNDRV_PCM_INFO_PERFECT_DRAIN

[Takashi Iwai]

On Thu, 04 May 2023 10:31:50 +0200,
Jaroslav Kysela wrote:
> 
> On 04. 05. 23 10:18, Takashi Iwai wrote:
> > On Tue, 02 May 2023 13:50:09 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> The driver may not require to touch the sample stream
> >> for the drain operation at all. Handle this situation
> >> in alsa-lib.
> >> 
> >> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> > 
> > Ideally speaking, the checks and the setups of those new bits should
> > be coupled with the PCM protocol version check (and the version
> > bump).
> > 
> > But it seems that you've already applied the series, and practically
> > seen, those bits should be either not set or harmless, so let's cross
> > fingers.
> 
> Exactly, the current kernel code should skip those new flags, so they
> are used in alsa-lib only. It's just something like a "reservation"
> for the kernel space until things are really used there. We can bump
> the protocol version later (perhaps with other changes).

If you'd like to include the new drain stuff in ALSA 1.2.9 release, we
can take the uapi change to 6.4-rc1, too.  But then let's bump the PCM
protocol version along with it.

So, if you think it's worth aligning with 6.4-rc1, let me know ASAP.
I planned the submission to Linus in tomorrow, and I'd need to prepare
and merge the uapi updates soon.


thanks,

Takashi

[PATCH alsa-lib 4/4] pcm: hw: introduce SNDRV_PCM_HW_PARAMS_DRAIN_SILENCE

[Jaroslav Kysela]

The application may not require to touch the sample stream
for the drain operation at all. Handle this situation
in alsa-lib.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 include/sound/uapi/asound.h |  3 +++
 src/pcm/pcm.c               | 32 ++++++++++++++++++++++++++++++++
 src/pcm/pcm_hw.c            |  3 ++-
 src/pcm/pcm_local.h         |  1 +
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/include/sound/uapi/asound.h b/include/sound/uapi/asound.h
index 0b8834f2..f970179e 100644
--- a/include/sound/uapi/asound.h
+++ b/include/sound/uapi/asound.h
@@ -390,6 +390,9 @@ typedef int snd_pcm_hw_param_t;
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
 #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
 #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
+#define SNDRV_PCM_HW_PARAMS_NO_DRAIN_SILENCE	(1<<3)	/* supress drain with the filling
+							 * of the silence samples
+							 */
 
 struct snd_interval {
 	unsigned int min, max;
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 099d83ee..5872ee6f 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -4958,6 +4958,38 @@ int snd_pcm_hw_params_get_period_wakeup(snd_pcm_t *pcm, snd_pcm_hw_params_t *par
 	return 0;
 }
 
+/**
+ * \brief Restrict a configuration space to allow the drain with the filling of silence samples
+ * \param pcm PCM handle
+ * \param params Configuration space
+ * \param val 0 = disable, 1 = enable (default) drain with the filling of silence samples
+ * \return 0 otherwise a negative error code
+ */
+int snd_pcm_hw_params_set_drain_silence(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val)
+{
+	assert(pcm && params);
+	if (val)
+		params->flags &= ~SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE;
+	else
+		params->flags |= SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE;
+	params->rmask = ~0;
+	return snd_pcm_hw_refine(pcm, params);
+}
+
+/**
+ * \brief Extract drain with the filling of silence samples from a configuration space
+ * \param pcm PCM handle
+ * \param params Configuration space
+ * \param val 0 = disable, 1 = enable
+ * \return 0 otherwise a negative error code
+ */
+int snd_pcm_hw_params_get_drain_silence(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val)
+{
+	assert(pcm && params && val);
+	*val = params->flags & SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE ? 0 : 1;
+	return 0;
+}
+
 /**
  * \brief Extract period time from a configuration space
  * \param params Configuration space
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index ea0c2ef2..90d5d07a 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -399,7 +399,8 @@ static int snd_pcm_hw_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
 	params->info &= ~0xf0000000;
 	if (pcm->tstamp_type != SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY)
 		params->info |= SND_PCM_INFO_MONOTONIC;
-	hw->perfect_drain = !!(params->info & SND_PCM_INFO_PERFECT_DRAIN);
+	hw->perfect_drain = !!(params->info & SND_PCM_INFO_PERFECT_DRAIN) ||
+			    !!(params->flags & SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE);
 	return query_status_data(hw);
 }
 
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index b039dda0..abea6654 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -107,6 +107,7 @@
 #define SND_PCM_HW_PARAMS_NORESAMPLE SNDRV_PCM_HW_PARAMS_NORESAMPLE
 #define SND_PCM_HW_PARAMS_EXPORT_BUFFER SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER
 #define SND_PCM_HW_PARAMS_NO_PERIOD_WAKEUP SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP
+#define SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE SNDRV_PCM_HW_PARAMS_NO_DRAIN_SILENCE
 
 #define SND_PCM_INFO_MONOTONIC	0x80000000
 
-- 
2.39.2

Re: [PATCH alsa-lib 4/4] pcm: hw: introduce SNDRV_PCM_HW_PARAMS_DRAIN_SILENCE

[Oswald Buddenhagen]

On Tue, May 02, 2023 at 01:50:10PM +0200, Jaroslav Kysela wrote:
>The application may not require to touch the sample stream
>for the drain operation at all. Handle this situation
>in alsa-lib.
>
i find this too vague.

  This allows an application which uses an mmapped buffer to inform us 
  that it is handling the silence padding itself.

>Signed-off-by: Jaroslav Kysela <perex@perex.cz>
>---
> include/sound/uapi/asound.h |  3 +++
> src/pcm/pcm.c               | 32 ++++++++++++++++++++++++++++++++
> src/pcm/pcm_hw.c            |  3 ++-
> src/pcm/pcm_local.h         |  1 +
> 4 files changed, 38 insertions(+), 1 deletion(-)
>
>diff --git a/include/sound/uapi/asound.h b/include/sound/uapi/asound.h
>index 0b8834f2..f970179e 100644
>--- a/include/sound/uapi/asound.h
>+++ b/include/sound/uapi/asound.h
>@@ -390,6 +390,9 @@ typedef int snd_pcm_hw_param_t;
> #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
> #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
> #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
>+#define SNDRV_PCM_HW_PARAMS_NO_DRAIN_SILENCE	(1<<3)	/* supress drain with the filling
>+							 * of the silence samples
>+							 */
"suppress automatic silence fill when draining playback"

>--- a/src/pcm/pcm.c
>+++ b/src/pcm/pcm.c
>@@ -4958,6 +4958,38 @@ int snd_pcm_hw_params_get_period_wakeup(snd_pcm_t *pcm, snd_pcm_hw_params_t *par
> 	return 0;
> }
> 
>+/**
>+ * \brief Restrict a configuration space to allow the drain with the 
>filling of silence samples
>
there is no way i'd understand what this means. in fact, i'm having a 
hard time even though i more or less know it already.

>+ * \param pcm PCM handle
>+ * \param params Configuration space
>+ * \param val 0 = disable, 1 = enable (default) drain with the filling of silence samples
>
"padding the playback buffer with silence when drain() is invoked"

>+ * \return 0 otherwise a negative error code

add description:

"When disabled, the application must ensure that enough samples are 
silenced, as most hardware will read beyond the application pointer when 
drain() is invoked."

>+/**
>+ * \brief Extract drain with the filling of silence samples from a configuration space
>
this is also kinda incomprehensible.

>+ * \param pcm PCM handle
>+ * \param params Configuration space
>+ * \param val 0 = disable, 1 = enable
>
as this returns the status quo, this should be disabled/enabled.

>+ * \return 0 otherwise a negative error code
>
there is no "otherwise" here.

>+ */
>+int snd_pcm_hw_params_get_drain_silence(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val)
>+{
>+	assert(pcm && params && val);
>+	*val = params->flags & SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE ? 0 : 1;
>
i know i'm paranoid, but i'd put extra parens around the condition.

>+	return 0;
>+}
>+

>--- a/src/pcm/pcm_hw.c
>+++ b/src/pcm/pcm_hw.c
>@@ -399,7 +399,8 @@ static int snd_pcm_hw_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
>-	hw->perfect_drain = !!(params->info & SND_PCM_INFO_PERFECT_DRAIN);
>+	hw->perfect_drain = !!(params->info & SND_PCM_INFO_PERFECT_DRAIN) ||
>+			    !!(params->flags & SND_PCM_HW_PARAMS_NO_DRAIN_SILENCE);
> 
pedantically, you can remove the double negations as this point.

regards