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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
