[alsa-devel] [PATCH 0/7] ALSA: Fix/improve PCM ack callback
Hi,
this is a series of patches to fix the potential bugs in PCM ack callback using the pcm-indirect helper functions, and also to enhance the ack callback to be called properly in all appl_ptr updates, as similarly done by Pierre and Subhransu's patches.
The changes in the pcm-indirect helper code and its users are fairly trivial, just passing the error code back.
ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.
Takashi
===
Takashi Iwai (7): ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers ALSA: mips: Deliver indirect-PCM transfer error ALSA: cs46xx: Deliver indirect-PCM transfer error ALSA: emu10k1: Deliver indirect-PCM transfer error ALSA: rme32: Deliver indirect-PCM transfer error staging: bcm2835-audio: Deliver indirect-PCM transfer error ALSA: pcm: Call ack() whenever appl_ptr is updated
.../vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +-- include/sound/pcm-indirect.h | 10 ++++- sound/core/pcm_native.c | 46 +++++++++++++++++----- sound/mips/hal2.c | 14 +++---- sound/pci/cs46xx/cs46xx_lib.c | 8 ++-- sound/pci/emu10k1/emupcm.c | 4 +- sound/pci/rme32.c | 10 ++--- 7 files changed, 63 insertions(+), 34 deletions(-)
The indirect-PCM helper codes have an implicit assumption that the appl_ptr always increases. But the PCM core may deal with the decrement of appl_ptr via rewind ioctls, and it may screw up the buffer pointer management.
This patch adds the negative appl_ptr diff in transfer functions and let returning an error instead of always accepting the appl_ptr updates. The callers are usually PCM ack callbacks, and they pass the error to the upper layer accordingly.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm-indirect.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h index 1df7acaaa535..7ade285328cf 100644 --- a/include/sound/pcm-indirect.h +++ b/include/sound/pcm-indirect.h @@ -43,7 +43,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream, /* * helper function for playback ack callback */ -static inline void +static inline int snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, struct snd_pcm_indirect *rec, snd_pcm_indirect_copy_t copy) @@ -56,6 +56,8 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, if (diff) { if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2)) diff += runtime->boundary; + if (diff < 0) + return -EINVAL; rec->sw_ready += (int)frames_to_bytes(runtime, diff); rec->appl_ptr = appl_ptr; } @@ -82,6 +84,7 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, rec->hw_ready += bytes; rec->sw_ready -= bytes; } + return 0; }
/* @@ -109,7 +112,7 @@ snd_pcm_indirect_playback_pointer(struct snd_pcm_substream *substream, /* * helper function for capture ack callback */ -static inline void +static inline int snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream, struct snd_pcm_indirect *rec, snd_pcm_indirect_copy_t copy) @@ -121,6 +124,8 @@ snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream, if (diff) { if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2)) diff += runtime->boundary; + if (diff < 0) + return -EINVAL; rec->sw_ready -= frames_to_bytes(runtime, diff); rec->appl_ptr = appl_ptr; } @@ -147,6 +152,7 @@ snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream, rec->hw_ready -= bytes; rec->sw_ready += bytes; } + return 0; }
/*
Now that the indirect-PCM transfer helper gives back an error, we should return the error from ack callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/mips/hal2.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c index 00fc9241d266..684dc4ddef41 100644 --- a/sound/mips/hal2.c +++ b/sound/mips/hal2.c @@ -616,10 +616,9 @@ static int hal2_playback_ack(struct snd_pcm_substream *substream) struct hal2_codec *dac = &hal2->dac;
dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2; - snd_pcm_indirect_playback_transfer(substream, - &dac->pcm_indirect, - hal2_playback_transfer); - return 0; + return snd_pcm_indirect_playback_transfer(substream, + &dac->pcm_indirect, + hal2_playback_transfer); }
static int hal2_capture_open(struct snd_pcm_substream *substream) @@ -707,10 +706,9 @@ static int hal2_capture_ack(struct snd_pcm_substream *substream) struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream); struct hal2_codec *adc = &hal2->adc;
- snd_pcm_indirect_capture_transfer(substream, - &adc->pcm_indirect, - hal2_capture_transfer); - return 0; + return snd_pcm_indirect_capture_transfer(substream, + &adc->pcm_indirect, + hal2_capture_transfer); }
static struct snd_pcm_ops hal2_playback_ops = {
Now that the indirect-PCM transfer helper gives back an error, we should return the error from ack callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/cs46xx/cs46xx_lib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c index 00fa52e9a2f2..ae2aad52ea6f 100644 --- a/sound/pci/cs46xx/cs46xx_lib.c +++ b/sound/pci/cs46xx/cs46xx_lib.c @@ -887,8 +887,8 @@ static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_cs46xx_pcm * cpcm = runtime->private_data; - snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy); - return 0; + return snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, + snd_cs46xx_pb_trans_copy); }
static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream, @@ -903,8 +903,8 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream, static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream) { struct snd_cs46xx *chip = snd_pcm_substream_chip(substream); - snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy); - return 0; + return snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, + snd_cs46xx_cp_trans_copy); }
static snd_pcm_uframes_t snd_cs46xx_playback_direct_pointer(struct snd_pcm_substream *substream)
Now that the indirect-PCM transfer helper gives back an error, we should return the error from ack callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/emu10k1/emupcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index ef1cf530c929..bdda29f335f6 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -1632,8 +1632,8 @@ static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substr struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
- snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy); - return 0; + return snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, + fx8010_pb_trans_copy); }
static int snd_emu10k1_fx8010_playback_hw_params(struct snd_pcm_substream *substream,
Now that the indirect-PCM transfer helper gives back an error, we should return the error from ack callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/rme32.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c index 96d15db65dfd..f9b424056d0f 100644 --- a/sound/pci/rme32.c +++ b/sound/pci/rme32.c @@ -1157,9 +1157,8 @@ static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream) if (rme32->running & (1 << SNDRV_PCM_STREAM_CAPTURE)) rec->hw_queue_size -= cprec->hw_ready; spin_unlock(&rme32->lock); - snd_pcm_indirect_playback_transfer(substream, rec, - snd_rme32_pb_trans_copy); - return 0; + return snd_pcm_indirect_playback_transfer(substream, rec, + snd_rme32_pb_trans_copy); }
static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream, @@ -1174,9 +1173,8 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream, static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream) { struct rme32 *rme32 = snd_pcm_substream_chip(substream); - snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm, - snd_rme32_cp_trans_copy); - return 0; + return snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm, + snd_rme32_cp_trans_copy); }
static snd_pcm_uframes_t
Now that the indirect-PCM transfer helper gives back an error, we should return the error from ack callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c index e8cf0b97bf02..3637ddf909a4 100644 --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c @@ -353,9 +353,8 @@ static int snd_bcm2835_pcm_ack(struct snd_pcm_substream *substream) struct snd_pcm_indirect *pcm_indirect = &alsa_stream->pcm_indirect;
pcm_indirect->hw_queue_size = runtime->hw.buffer_bytes_max; - snd_pcm_indirect_playback_transfer(substream, pcm_indirect, - snd_bcm2835_pcm_transfer); - return 0; + return snd_pcm_indirect_playback_transfer(substream, pcm_indirect, + snd_bcm2835_pcm_transfer); }
/* trigger callback */
Takashi Iwai tiwai@suse.de writes:
Now that the indirect-PCM transfer helper gives back an error, we should return the error from ack callbacks.
As far as bcm2835 goes:
Acked-by: Eric Anholt eric@anholt.net
Although the ack callback is supposed to be called at each appl_ptr or hw_ptr update, we missed a few opportunities: namely, forward, rewind and sync_ptr.
Formerly calling ack at rewind may have leaded to unexpected results due to the forgotten negative appl_ptr update in indirect-PCM helper, which is the major user of the PCM ack callback. But now we fixed this oversights, thus we can call ack callback safely even at rewind callback -- of course with the proper handling of the error from the callback.
This patch adds the calls of ack callback in the places mentioned in the above.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index ecde57afa45a..7bc4a0bbad6f 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2455,13 +2455,35 @@ static int do_pcm_hwsync(struct snd_pcm_substream *substream) } }
-/* increase the appl_ptr; returns the processed frames */ +/* update to the given appl_ptr and call ack callback if needed; + * when an error is returned, take back to the original value + */ +static int apply_appl_ptr(struct snd_pcm_substream *substream, + snd_pcm_uframes_t appl_ptr) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr; + int ret; + + runtime->control->appl_ptr = appl_ptr; + if (substream->ops->ack) { + ret = substream->ops->ack(substream); + if (ret < 0) { + runtime->control->appl_ptr = old_appl_ptr; + return ret; + } + } + return 0; +} + +/* increase the appl_ptr; returns the processed frames or a negative error */ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream, snd_pcm_uframes_t frames, snd_pcm_sframes_t avail) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_sframes_t appl_ptr; + int ret;
if (avail <= 0) return 0; @@ -2470,17 +2492,18 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr + frames; if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary) appl_ptr -= runtime->boundary; - runtime->control->appl_ptr = appl_ptr; - return frames; + ret = apply_appl_ptr(substream, appl_ptr); + return ret < 0 ? ret : frames; }
-/* decrease the appl_ptr; returns the processed frames */ +/* decrease the appl_ptr; returns the processed frames or a negative error */ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, snd_pcm_uframes_t frames, snd_pcm_sframes_t avail) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_sframes_t appl_ptr; + int ret;
if (avail <= 0) return 0; @@ -2489,8 +2512,8 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr - frames; if (appl_ptr < 0) appl_ptr += runtime->boundary; - runtime->control->appl_ptr = appl_ptr; - return frames; + ret = apply_appl_ptr(substream, appl_ptr); + return ret < 0 ? ret : frames; }
static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream, @@ -2620,10 +2643,15 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, return err; } snd_pcm_stream_lock_irq(substream); - if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) - control->appl_ptr = sync_ptr.c.control.appl_ptr; - else + if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) { + err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr); + if (err < 0) { + snd_pcm_stream_unlock_irq(substream); + return err; + } + } else { sync_ptr.c.control.appl_ptr = control->appl_ptr; + } if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN)) control->avail_min = sync_ptr.c.control.avail_min; else
Hi,
On May 22 2017 04:02, Takashi Iwai wrote:
Hi,
this is a series of patches to fix the potential bugs in PCM ack callback using the pcm-indirect helper functions, and also to enhance the ack callback to be called properly in all appl_ptr updates, as similarly done by Pierre and Subhransu's patches.
The changes in the pcm-indirect helper code and its users are fairly trivial, just passing the error code back.
ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.
Takashi
===
Takashi Iwai (7): ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers ALSA: mips: Deliver indirect-PCM transfer error ALSA: cs46xx: Deliver indirect-PCM transfer error ALSA: emu10k1: Deliver indirect-PCM transfer error ALSA: rme32: Deliver indirect-PCM transfer error staging: bcm2835-audio: Deliver indirect-PCM transfer error ALSA: pcm: Call ack() whenever appl_ptr is updated
.../vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +-- include/sound/pcm-indirect.h | 10 ++++- sound/core/pcm_native.c | 46 +++++++++++++++++----- sound/mips/hal2.c | 14 +++---- sound/pci/cs46xx/cs46xx_lib.c | 8 ++-- sound/pci/emu10k1/emupcm.c | 4 +- sound/pci/rme32.c | 10 ++--- 7 files changed, 63 insertions(+), 34 deletions(-)
I mostly agree with this patchset, except for one point.
The added helper function, apply_appl_ptr(), copies given data to the runtime for application-side pointer, then call 'struct snd_pcm_ops.ack'. We have similar code block in 'sound/core/pcm_lib.c', as well. See 'snd_pcm_lib_write1()' and 'snd_pcm_lib_read1()'.
In a point of code maintenance, it's better to share the function between two objects; pcm_native.o and pcm_lib.o.
For instance, I can propose a below patch. I expect you to squash it up within one of your patches.
========== 8< ----------
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index ab4b1d1e44ee..9d048f91bd3f 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2010,6 +2010,12 @@ typedef int (*transfer_f)(struct snd_pcm_substream *substream, unsigned int hwof unsigned long data, unsigned int off, snd_pcm_uframes_t size);
+/* This symbol is shared by several relocatable objects for the same shared + * object. + */ +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream, + snd_pcm_uframes_t appl_ptr); + static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, unsigned long data, snd_pcm_uframes_t size, @@ -2087,11 +2093,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, break; } appl_ptr += frames; - if (appl_ptr >= runtime->boundary) - appl_ptr -= runtime->boundary; - runtime->control->appl_ptr = appl_ptr; - if (substream->ops->ack) - substream->ops->ack(substream); + err = snd_pcm_apply_appl_ptr(substream, appl_ptr); + if (err < 0) + goto _end_unlock;
offset += frames; size -= frames; @@ -2321,9 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, appl_ptr += frames; if (appl_ptr >= runtime->boundary) appl_ptr -= runtime->boundary; - runtime->control->appl_ptr = appl_ptr; - if (substream->ops->ack) - substream->ops->ack(substream); + err = snd_pcm_apply_appl_ptr(substream, appl_ptr); + if (err < 0) + goto _end_unlock;
offset += frames; size -= frames; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 7bc4a0bbad6f..0b176b35ade1 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2456,9 +2456,11 @@ static int do_pcm_hwsync(struct snd_pcm_substream *substream) }
/* update to the given appl_ptr and call ack callback if needed; - * when an error is returned, take back to the original value + * when an error is returned, take back to the original value. + * The given argument should have valid value as application pointer on PCM + * buffer. */ -static int apply_appl_ptr(struct snd_pcm_substream *substream, +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream, snd_pcm_uframes_t appl_ptr) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -2492,7 +2494,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr + frames; if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary) appl_ptr -= runtime->boundary; - ret = apply_appl_ptr(substream, appl_ptr); + ret = snd_pcm_apply_appl_ptr(substream, appl_ptr); return ret < 0 ? ret : frames; }
@@ -2512,7 +2514,7 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr - frames; if (appl_ptr < 0) appl_ptr += runtime->boundary; - ret = apply_appl_ptr(substream, appl_ptr); + ret = snd_pcm_apply_appl_ptr(substream, appl_ptr); return ret < 0 ? ret : frames; }
@@ -2644,7 +2646,8 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, } snd_pcm_stream_lock_irq(substream); if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) { - err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr); + err = snd_pcm_apply_appl_ptr(substream, + sync_ptr.c.control.appl_ptr); if (err < 0) { snd_pcm_stream_unlock_irq(substream); return err;
Regards
Takashi Sakamoto
On Mon, 22 May 2017 05:49:00 +0200, Takashi Sakamoto wrote:
Hi,
On May 22 2017 04:02, Takashi Iwai wrote:
Hi,
this is a series of patches to fix the potential bugs in PCM ack callback using the pcm-indirect helper functions, and also to enhance the ack callback to be called properly in all appl_ptr updates, as similarly done by Pierre and Subhransu's patches.
The changes in the pcm-indirect helper code and its users are fairly trivial, just passing the error code back.
ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.
Takashi
===
Takashi Iwai (7): ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers ALSA: mips: Deliver indirect-PCM transfer error ALSA: cs46xx: Deliver indirect-PCM transfer error ALSA: emu10k1: Deliver indirect-PCM transfer error ALSA: rme32: Deliver indirect-PCM transfer error staging: bcm2835-audio: Deliver indirect-PCM transfer error ALSA: pcm: Call ack() whenever appl_ptr is updated
.../vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +-- include/sound/pcm-indirect.h | 10 ++++- sound/core/pcm_native.c | 46 +++++++++++++++++----- sound/mips/hal2.c | 14 +++---- sound/pci/cs46xx/cs46xx_lib.c | 8 ++-- sound/pci/emu10k1/emupcm.c | 4 +- sound/pci/rme32.c | 10 ++--- 7 files changed, 63 insertions(+), 34 deletions(-)
I mostly agree with this patchset, except for one point.
The added helper function, apply_appl_ptr(), copies given data to the runtime for application-side pointer, then call 'struct snd_pcm_ops.ack'. We have similar code block in 'sound/core/pcm_lib.c', as well. See 'snd_pcm_lib_write1()' and 'snd_pcm_lib_read1()'.
In a point of code maintenance, it's better to share the function between two objects; pcm_native.o and pcm_lib.o.
For instance, I can propose a below patch. I expect you to squash it up within one of your patches.
========== 8< ----------
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index ab4b1d1e44ee..9d048f91bd3f 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2010,6 +2010,12 @@ typedef int (*transfer_f)(struct snd_pcm_substream *substream, unsigned int hwof unsigned long data, unsigned int off, snd_pcm_uframes_t size);
+/* This symbol is shared by several relocatable objects for the same shared
- object.
- */
+int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
snd_pcm_uframes_t appl_ptr);static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, unsigned long data, snd_pcm_uframes_t size, @@ -2087,11 +2093,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, break; } appl_ptr += frames;
if (appl_ptr >= runtime->boundary)appl_ptr -= runtime->boundary;runtime->control->appl_ptr = appl_ptr;if (substream->ops->ack)substream->ops->ack(substream);
err = snd_pcm_apply_appl_ptr(substream, appl_ptr);if (err < 0)goto _end_unlock;offset += frames; size -= frames;
@@ -2321,9 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, appl_ptr += frames; if (appl_ptr >= runtime->boundary) appl_ptr -= runtime->boundary;
runtime->control->appl_ptr = appl_ptr;if (substream->ops->ack)substream->ops->ack(substream);
err = snd_pcm_apply_appl_ptr(substream, appl_ptr);if (err < 0)goto _end_unlock;offset += frames; size -= frames;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 7bc4a0bbad6f..0b176b35ade1 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2456,9 +2456,11 @@ static int do_pcm_hwsync(struct snd_pcm_substream *substream) }
/* update to the given appl_ptr and call ack callback if needed;
- when an error is returned, take back to the original value
- when an error is returned, take back to the original value.
- The given argument should have valid value as application pointer on PCM
*/
- buffer.
-static int apply_appl_ptr(struct snd_pcm_substream *substream, +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream, snd_pcm_uframes_t appl_ptr) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -2492,7 +2494,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr + frames; if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary) appl_ptr -= runtime->boundary;
- ret = apply_appl_ptr(substream, appl_ptr);
- ret = snd_pcm_apply_appl_ptr(substream, appl_ptr); return ret < 0 ? ret : frames;
}
@@ -2512,7 +2514,7 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr - frames; if (appl_ptr < 0) appl_ptr += runtime->boundary;
- ret = apply_appl_ptr(substream, appl_ptr);
- ret = snd_pcm_apply_appl_ptr(substream, appl_ptr); return ret < 0 ? ret : frames;
}
@@ -2644,7 +2646,8 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, } snd_pcm_stream_lock_irq(substream); if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
err = snd_pcm_apply_appl_ptr(substream,sync_ptr.c.control.appl_ptr);
These functions will be modified again by other upcoming patchsets, so I'd like not to touch so much for minor optimization at this stage. Let's postpone that after all settled down.
thanks,
Takashi
On May 22 2017 14:27, Takashi Iwai wrote:
These functions will be modified again by other upcoming patchsets, so I'd like not to touch so much for minor optimization at this stage.
If you address to your latest patchset[1], it might not be applied promptly because I have some comments (not posted yet, in this night). It's not good to avoid better bahaviour in a reason of patchset dependency, at least you seems to be too hasty.
[1] [alsa-devel] [PATCH 00/16] ALSA: Convert to new copy_silence PCM ops
Regards
Takashi Sakamoto
On Mon, 22 May 2017 08:31:13 +0200, Takashi Sakamoto wrote:
On May 22 2017 14:27, Takashi Iwai wrote:
These functions will be modified again by other upcoming patchsets, so I'd like not to touch so much for minor optimization at this stage.
If you address to your latest patchset[1], it might not be applied promptly because I have some comments (not posted yet, in this night). It's not good to avoid better bahaviour in a reason of patchset dependency, at least you seems to be too hasty.
Come on, your suggestion is an optimization change that can be applied anytime later easily. If it were a mandatory fix, we should apply or fold it immediately, but that's not the case. The only necessary thing is not to forget it.
Takashi
[1] [alsa-devel] [PATCH 00/16] ALSA: Convert to new copy_silence PCM ops
Regards
Takashi Sakamoto
On Mon, 22 May 2017 08:46:56 +0200, Takashi Iwai wrote:
On Mon, 22 May 2017 08:31:13 +0200, Takashi Sakamoto wrote:
On May 22 2017 14:27, Takashi Iwai wrote:
These functions will be modified again by other upcoming patchsets, so I'd like not to touch so much for minor optimization at this stage.
If you address to your latest patchset[1], it might not be applied promptly because I have some comments (not posted yet, in this night). It's not good to avoid better bahaviour in a reason of patchset dependency, at least you seems to be too hasty.
Come on, your suggestion is an optimization change that can be applied anytime later easily. If it were a mandatory fix, we should apply or fold it immediately, but that's not the case. The only necessary thing is not to forget it.
That implies: just submit your proposed change as an incremental patch on top of mine. Then I'll queue it.
thanks,
Takashi
On Sun, 21 May 2017 21:02:51 +0200, Takashi Iwai wrote:
Hi,
this is a series of patches to fix the potential bugs in PCM ack callback using the pcm-indirect helper functions, and also to enhance the ack callback to be called properly in all appl_ptr updates, as similarly done by Pierre and Subhransu's patches.
The changes in the pcm-indirect helper code and its users are fairly trivial, just passing the error code back.
ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.
Takashi
FYI, the patches have been merged to for-next branch.
Takashi
participants (3)
-
Eric Anholt -
Takashi Iwai -
Takashi Sakamoto