[alsa-devel] [PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT
Hi,
This RFC patchset is my concept work of another solution against a series of issues in ALSA PCM core, for which Iwai Takashi work in his hasty proposals[2][3]. Included changes are also available in my personal repository on github.com[4].
The aim of this work is to declare existent issues and to propose solutions as moderated as possible. Code refactoring is done to assist it.
Patch 01-05: code refactoring phase, with an alias of function prototype. Patch 06-09: driver API changing phase. The .copy_frames is introduced. Patch 10-11: integration phase for proxy drivers, to get new configuration.
I need to do this work with a little time (half a day), thus changes are as minimal as possible, especially two drivers are just modified even if drivers in below list should be changed: * drivers/media/pci/solo6x10/solo6x10-g723.c * sound/drivers/dummy.c * sound/isa/gus/gus_pcm.c * sound/isa/sb/emu8000_pcm.c * sound/pci/es1938.c * sound/pci/korg1212/korg1212.c * sound/pci/nm256/nm256.c * sound/pci/rme32.c * sound/pci/rme96.c * sound/pci/rme9652/hdsp.c * sound/pci/rme9652/rme9652.c * sound/sh/sh_dac_audio.c * sound/soc/blackfin/bf5xx-ac97-pcm.c * sound/soc/blackfin/bf5xx-i2s-pcm.c
Furthermore, I apologize that this is untested. I wish you to check the concept.
[1] [alsa-devel] [PATCH RFC 00/26] Kill set_fs() in ALSA codes http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120542 [2] [alsa-devel] [PATCH 00/16] ALSA: Convert to new copy_silence PCM ops http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120895.html [3] [alsa-devel] [PATCH 0/4] ALSA: Extend PCM buffer-copy helpers http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120957.html [4] takaswie/sound https://github.com/takaswie/sound/tree/topic/add-pcm-frame-ops
Takashi Sakamoto (11): ALSA: pcm: introduce an alias of prototype to copy PCM frames ALSA: pcm: use new function alias instead of local one ALSA: pcm: refactoring silence operation ALSA: pcm: add alternative calls ALSA: core: remove duplicated codes to handle PCM frames ALSA: pcm: add new operation; copy_frames ALSA: rme9652: a sample for drivers which support interleaved buffer ALSA: gus: a sample for drivers which support non-interleaved buffer ALSA: pcm: remove copy and silence callbacks ALSA: pcm: add client_space parameter to runtime of PCM substream for PCM proxy drivers ALSA: pcm: add new configuration for PCM proxy drivers
drivers/usb/gadget/Kconfig | 3 +- include/sound/pcm.h | 13 +- sound/core/Kconfig | 15 +- sound/core/pcm_lib.c | 448 +++++++++++++++++++++++++++++---------------- sound/core/pcm_native.c | 10 + sound/isa/gus/gus_pcm.c | 100 +++++----- sound/pci/rme9652/hdsp.c | 59 +++--- 7 files changed, 398 insertions(+), 250 deletions(-)
Usage of the same function prototype between core and driver implementation dedicates code lucidity. Especially, in a view of device driver developers, if core implementation includes functions with the prototype, it's good examples.
In current design of ALSA PCM core, when devices doesn't use DMA between kernel space and device memory or platforms don't have good cache coherent protocol, then drivers need to have own implementation for data transmittion, without existent implementation inner ALSA PCM core.
However, for this purpose, ALSA PCM core includes inefficient implementation. Additionally, in a point to which I addressed, it includes complicated codes.
This commit introduces 'snd_pcm_copy_frames_t' to produce unified interface for the devices and core implementations.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- include/sound/pcm.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index c609b891c4c2..6cb8df081787 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -63,6 +63,10 @@ struct snd_pcm_substream; struct snd_pcm_audio_tstamp_config; /* definitions further down */ struct snd_pcm_audio_tstamp_report;
+typedef int (*snd_pcm_copy_frames_t)(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned int long data, + unsigned int off, snd_pcm_uframes_t count); + struct snd_pcm_ops { int (*open)(struct snd_pcm_substream *substream); int (*close)(struct snd_pcm_substream *substream);
In a previous commit, a new alias of function prototype is added. This alias is good to refactoring existent codes.
As a first step, this commit applies refactoring to kernel APIs for data transmission. The alias is the same as helper functions innner 'pcm_lib.c'. For followed commits, the helper functions are once assigned to function local variables.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_lib.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index ab4b1d1e44ee..db0246c94874 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2006,15 +2006,11 @@ static int snd_pcm_lib_write_transfer(struct snd_pcm_substream *substream, return 0; }
-typedef int (*transfer_f)(struct snd_pcm_substream *substream, unsigned int hwoff, - unsigned long data, unsigned int off, - snd_pcm_uframes_t size); - static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, unsigned long data, snd_pcm_uframes_t size, int nonblock, - transfer_f transfer) + snd_pcm_copy_frames_t copy_frames) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t xfer = 0; @@ -2072,7 +2068,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr; appl_ofs = appl_ptr % runtime->buffer_size; snd_pcm_stream_unlock_irq(substream); - err = transfer(substream, appl_ofs, data, offset, frames); + err = copy_frames(substream, appl_ofs, data, offset, frames); snd_pcm_stream_lock_irq(substream); if (err < 0) goto _end_unlock; @@ -2126,10 +2122,12 @@ static int pcm_sanity_check(struct snd_pcm_substream *substream) return 0; }
-snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, const void __user *buf, snd_pcm_uframes_t size) +snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, + const void __user *buf, snd_pcm_uframes_t size) { struct snd_pcm_runtime *runtime; int nonblock; + snd_pcm_copy_frames_t copy_frames; int err;
err = pcm_sanity_check(substream); @@ -2141,10 +2139,12 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, const v if (runtime->access != SNDRV_PCM_ACCESS_RW_INTERLEAVED && runtime->channels > 1) return -EINVAL; + + copy_frames = snd_pcm_lib_write_transfer; + return snd_pcm_lib_write1(substream, (unsigned long)buf, size, nonblock, - snd_pcm_lib_write_transfer); + copy_frames); } - EXPORT_SYMBOL(snd_pcm_lib_write);
static int snd_pcm_lib_writev_transfer(struct snd_pcm_substream *substream, @@ -2193,6 +2193,7 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, { struct snd_pcm_runtime *runtime; int nonblock; + snd_pcm_copy_frames_t copy_frames; int err;
err = pcm_sanity_check(substream); @@ -2203,10 +2204,12 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream,
if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL; + + copy_frames = snd_pcm_lib_writev_transfer; + return snd_pcm_lib_write1(substream, (unsigned long)bufs, frames, - nonblock, snd_pcm_lib_writev_transfer); + nonblock, copy_frames); } - EXPORT_SYMBOL(snd_pcm_lib_writev);
static int snd_pcm_lib_read_transfer(struct snd_pcm_substream *substream, @@ -2232,7 +2235,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, unsigned long data, snd_pcm_uframes_t size, int nonblock, - transfer_f transfer) + snd_pcm_copy_frames_t copy_frames) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t xfer = 0; @@ -2304,7 +2307,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, appl_ptr = runtime->control->appl_ptr; appl_ofs = appl_ptr % runtime->buffer_size; snd_pcm_stream_unlock_irq(substream); - err = transfer(substream, appl_ofs, data, offset, frames); + err = copy_frames(substream, appl_ofs, data, offset, frames); snd_pcm_stream_lock_irq(substream); if (err < 0) goto _end_unlock; @@ -2338,10 +2341,12 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, return xfer > 0 ? (snd_pcm_sframes_t)xfer : err; }
-snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, void __user *buf, snd_pcm_uframes_t size) +snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, + void __user *buf, snd_pcm_uframes_t size) { struct snd_pcm_runtime *runtime; int nonblock; + snd_pcm_copy_frames_t copy_frames; int err; err = pcm_sanity_check(substream); @@ -2351,9 +2356,12 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, void __u nonblock = !!(substream->f_flags & O_NONBLOCK); if (runtime->access != SNDRV_PCM_ACCESS_RW_INTERLEAVED) return -EINVAL; - return snd_pcm_lib_read1(substream, (unsigned long)buf, size, nonblock, snd_pcm_lib_read_transfer); -}
+ copy_frames = snd_pcm_lib_read_transfer; + + return snd_pcm_lib_read1(substream, (unsigned long)buf, size, nonblock, + copy_frames); +} EXPORT_SYMBOL(snd_pcm_lib_read);
static int snd_pcm_lib_readv_transfer(struct snd_pcm_substream *substream, @@ -2398,6 +2406,7 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, { struct snd_pcm_runtime *runtime; int nonblock; + snd_pcm_copy_frames_t copy_frames; int err;
err = pcm_sanity_check(substream); @@ -2410,9 +2419,12 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, nonblock = !!(substream->f_flags & O_NONBLOCK); if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL; - return snd_pcm_lib_read1(substream, (unsigned long)bufs, frames, nonblock, snd_pcm_lib_readv_transfer); -}
+ copy_frames = snd_pcm_lib_readv_transfer; + + return snd_pcm_lib_read1(substream, (unsigned long)bufs, frames, + nonblock, copy_frames); +} EXPORT_SYMBOL(snd_pcm_lib_readv);
/*
In a former commit, a new alias of function prototype is added. This alias is good to refactoring existent codes.
As a second step, this commit applies refactoring for data transmission of silent PCM frames. Two helper functions are added to handle in both cases of interleaved and non-interleaved frame alignment. They're independent, thus easy to read and understand.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_lib.c | 96 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 67 insertions(+), 29 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index db0246c94874..f06d9f4f9574 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,6 +42,53 @@ #define trace_hw_ptr_error(substream, reason) #endif
+static int writei_silence(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char *hwbuf; + + if (substream->ops->silence) + return substream->ops->silence(substream, -1, hwoff, count); + + hwbuf = runtime->dma_area + frames_to_bytes(runtime, hwoff); + snd_pcm_format_set_silence(runtime->format, hwbuf, + count * runtime->channels); + + return 0; +} + +static int writen_silence(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + unsigned int channels = runtime->channels; + snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; + char *hwbuf; + int c; + int err; + + if (substream->ops->silence) { + for (c = 0; c < channels; ++c) { + err = substream->ops->silence(substream, c, hwoff, + count); + if (err < 0) + return err; + } + } else { + for (c = 0; c < channels; ++c) { + hwbuf = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + snd_pcm_format_set_silence(runtime->format, hwbuf, + count); + } + } + + return 0; +} + /* * fill ring buffer with silence * runtime->silence_start: starting pointer to silence area @@ -51,10 +98,13 @@ * * when runtime->silence_size >= runtime->boundary - fill processed area with silence immediately */ -void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr) +void snd_pcm_playback_silence(struct snd_pcm_substream *substream, + snd_pcm_uframes_t new_hw_ptr) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t frames, ofs, transfer; + snd_pcm_copy_frames_t copy_frames; + int err;
if (runtime->silence_size < runtime->boundary) { snd_pcm_sframes_t noise_dist, n; @@ -104,36 +154,24 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram return; if (frames == 0) return; + + if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || + runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) + copy_frames = writei_silence; + else + copy_frames = writen_silence; + ofs = runtime->silence_start % runtime->buffer_size; while (frames > 0) { - transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames; - if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || - runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { - if (substream->ops->silence) { - int err; - err = substream->ops->silence(substream, -1, ofs, transfer); - snd_BUG_ON(err < 0); - } else { - char *hwbuf = runtime->dma_area + frames_to_bytes(runtime, ofs); - snd_pcm_format_set_silence(runtime->format, hwbuf, transfer * runtime->channels); - } - } else { - unsigned int c; - unsigned int channels = runtime->channels; - if (substream->ops->silence) { - for (c = 0; c < channels; ++c) { - int err; - err = substream->ops->silence(substream, c, ofs, transfer); - snd_BUG_ON(err < 0); - } - } else { - size_t dma_csize = runtime->dma_bytes / channels; - for (c = 0; c < channels; ++c) { - char *hwbuf = runtime->dma_area + (c * dma_csize) + samples_to_bytes(runtime, ofs); - snd_pcm_format_set_silence(runtime->format, hwbuf, transfer); - } - } - } + if (ofs + frames > runtime->buffer_size) + transfer = runtime->buffer_size - ofs; + else + transfer = frames; + + err = copy_frames(substream, ofs, (unsigned long)NULL, 0, + transfer); + snd_BUG_ON(err < 0); + runtime->silence_filled += transfer; frames -= transfer; ofs = 0;
When investigating current implementation for data copy between kernel/user spaces, helper functions includes much conditional processing. This brings lack of lucidity. Furthermore, this is not good in a point of hit ratio of i-cache.
This commit adds some helper functions for each cases to transfer PCM frames; to read and write, for interleaved and non-interleaved frame alignments. These helper functions also performs to transfer silent PCM frames when source buffer is NULL.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_lib.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 121 insertions(+), 8 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index f06d9f4f9574..38baa91fd0f6 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,6 +42,100 @@ #define trace_hw_ptr_error(substream, reason) #endif
+static int writei_from_space1(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char __user *buf = (char __user *)data; + char *dst = runtime->dma_area + frames_to_bytes(runtime, hwoff); + + if (buf == NULL) { + snd_pcm_format_set_silence(runtime->format, dst, + count * runtime->channels); + } else { + if (copy_from_user(dst, buf, frames_to_bytes(runtime, count))) + return -EFAULT; + } + + return 0; +} + +static int writen_from_space1(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char __user **bufs = (char __user **)data; + char __user *buf; + char *dst; + int channels = runtime->channels; + snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; + int c; + + for (c = 0; c < channels; ++c) { + dst = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + + if (bufs == NULL || bufs[c] == NULL) { + snd_pcm_format_set_silence(runtime->format, dst, count); + continue; + } + buf = bufs[c] + samples_to_bytes(runtime, off); + + dst = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + if (copy_from_user(dst, buf, samples_to_bytes(runtime, count))) + return -EFAULT; + } + + return 0; +} + +static int readi_to_space1(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char __user *buf = (char __user *)data; + char *src = runtime->dma_area + frames_to_bytes(runtime, hwoff); + + if (buf == NULL) + return -EINVAL; + buf += frames_to_bytes(runtime, off); + + if (copy_to_user(buf, src, frames_to_bytes(runtime, count))) + return -EFAULT; + + return 0; +} + +static int readn_to_space1(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char __user **bufs = (char __user **)data; + char __user *buf; + char *src; + unsigned int channels = runtime->channels; + snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; + int c; + + for (c = 0; c < channels; ++c) { + if (bufs == NULL || bufs[c] == NULL) + continue; + buf = bufs[c] + samples_to_bytes(runtime, off); + + src = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + if (copy_to_user(buf, src, samples_to_bytes(runtime, count))) + return -EFAULT; + } + + return 0; +} + static int writei_silence(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -156,10 +250,17 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, return;
if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || - runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) - copy_frames = writei_silence; - else - copy_frames = writen_silence; + runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { + if (substream->ops->silence) + copy_frames = writei_silence; + else + copy_frames = writei_from_space1; + } else { + if (substream->ops->silence) + copy_frames = writen_silence; + else + copy_frames = writen_from_space1; + }
ofs = runtime->silence_start % runtime->buffer_size; while (frames > 0) { @@ -2178,7 +2279,10 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, runtime->channels > 1) return -EINVAL;
- copy_frames = snd_pcm_lib_write_transfer; + if (substream->ops->copy) + copy_frames = snd_pcm_lib_write_transfer; + else + copy_frames = writei_from_space1;
return snd_pcm_lib_write1(substream, (unsigned long)buf, size, nonblock, copy_frames); @@ -2243,7 +2347,10 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL;
- copy_frames = snd_pcm_lib_writev_transfer; + if (substream->ops->copy) + copy_frames = snd_pcm_lib_writev_transfer; + else + copy_frames = writen_from_space1;
return snd_pcm_lib_write1(substream, (unsigned long)bufs, frames, nonblock, copy_frames); @@ -2395,7 +2502,10 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_INTERLEAVED) return -EINVAL;
- copy_frames = snd_pcm_lib_read_transfer; + if (substream->ops->copy) + copy_frames = snd_pcm_lib_read_transfer; + else + copy_frames = readi_to_space1;
return snd_pcm_lib_read1(substream, (unsigned long)buf, size, nonblock, copy_frames); @@ -2458,7 +2568,10 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL;
- copy_frames = snd_pcm_lib_readv_transfer; + if (substream->ops->copy) + copy_frames = snd_pcm_lib_readv_transfer; + else + copy_frames = readn_to_space1;
return snd_pcm_lib_read1(substream, (unsigned long)bufs, frames, nonblock, copy_frames);
In a previous commit, common data copying operation is passed to each of specific helper functions.
This commit removes duplicated codes in old implementations. As a result, such old functions becomes wrappers of driver-side implementation of 'struct snd_pcm_ops.silence' and 'struct snd_pcm_ops.copy'.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_lib.c | 159 ++++++++++++++++++--------------------------------- 1 file changed, 57 insertions(+), 102 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 38baa91fd0f6..8dfe964e8931 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -140,17 +140,7 @@ static int writei_silence(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) { - struct snd_pcm_runtime *runtime = substream->runtime; - char *hwbuf; - - if (substream->ops->silence) - return substream->ops->silence(substream, -1, hwoff, count); - - hwbuf = runtime->dma_area + frames_to_bytes(runtime, hwoff); - snd_pcm_format_set_silence(runtime->format, hwbuf, - count * runtime->channels); - - return 0; + return substream->ops->silence(substream, -1, hwoff, count); }
static int writen_silence(struct snd_pcm_substream *substream, @@ -159,25 +149,13 @@ static int writen_silence(struct snd_pcm_substream *substream, { struct snd_pcm_runtime *runtime = substream->runtime; unsigned int channels = runtime->channels; - snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; - char *hwbuf; int c; int err;
- if (substream->ops->silence) { - for (c = 0; c < channels; ++c) { - err = substream->ops->silence(substream, c, hwoff, - count); - if (err < 0) - return err; - } - } else { - for (c = 0; c < channels; ++c) { - hwbuf = runtime->dma_area + (c * dma_csize) + - samples_to_bytes(runtime, hwoff); - snd_pcm_format_set_silence(runtime->format, hwbuf, - count); - } + for (c = 0; c < channels; ++c) { + err = substream->ops->silence(substream, c, hwoff, count); + if (err < 0) + return err; }
return 0; @@ -2129,20 +2107,16 @@ static int wait_for_avail(struct snd_pcm_substream *substream, static int snd_pcm_lib_write_transfer(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, - snd_pcm_uframes_t frames) + snd_pcm_uframes_t count) { struct snd_pcm_runtime *runtime = substream->runtime; - int err; - char __user *buf = (char __user *) data + frames_to_bytes(runtime, off); - if (substream->ops->copy) { - if ((err = substream->ops->copy(substream, -1, hwoff, buf, frames)) < 0) - return err; - } else { - char *hwbuf = runtime->dma_area + frames_to_bytes(runtime, hwoff); - if (copy_from_user(hwbuf, buf, frames_to_bytes(runtime, frames))) - return -EFAULT; - } - return 0; + char __user *buf = (char __user *) data; + + if (buf == NULL && substream->ops->silence) + return substream->ops->silence(substream, -1, hwoff, count); + + buf += frames_to_bytes(runtime, off); + return substream->ops->copy(substream, -1, hwoff, buf, count); }
static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, @@ -2295,37 +2269,30 @@ static int snd_pcm_lib_writev_transfer(struct snd_pcm_substream *substream, snd_pcm_uframes_t frames) { struct snd_pcm_runtime *runtime = substream->runtime; - int err; - void __user **bufs = (void __user **)data; + char __user **bufs = (char __user **)data; + char __user *buf; int channels = runtime->channels; int c; - if (substream->ops->copy) { - if (snd_BUG_ON(!substream->ops->silence)) - return -EINVAL; - for (c = 0; c < channels; ++c, ++bufs) { - if (*bufs == NULL) { - if ((err = substream->ops->silence(substream, c, hwoff, frames)) < 0) - return err; - } else { - char __user *buf = *bufs + samples_to_bytes(runtime, off); - if ((err = substream->ops->copy(substream, c, hwoff, buf, frames)) < 0) - return err; - } - } - } else { - /* default transfer behaviour */ - size_t dma_csize = runtime->dma_bytes / channels; - for (c = 0; c < channels; ++c, ++bufs) { - char *hwbuf = runtime->dma_area + (c * dma_csize) + samples_to_bytes(runtime, hwoff); - if (*bufs == NULL) { - snd_pcm_format_set_silence(runtime->format, hwbuf, frames); - } else { - char __user *buf = *bufs + samples_to_bytes(runtime, off); - if (copy_from_user(hwbuf, buf, samples_to_bytes(runtime, frames))) - return -EFAULT; - } + int err; + + if (snd_BUG_ON(!substream->ops->silence)) + return -EINVAL; + + for (c = 0; c < channels; ++c) { + if (bufs == NULL || bufs[c] == NULL) { + err = substream->ops->silence(substream, c, hwoff, + frames); + if (err < 0) + return err; + } else { + buf = bufs[c] + samples_to_bytes(runtime, off); + err = substream->ops->copy(substream, c, hwoff, buf, + frames); + if (err < 0) + return err; } } + return 0; }
@@ -2363,17 +2330,13 @@ static int snd_pcm_lib_read_transfer(struct snd_pcm_substream *substream, snd_pcm_uframes_t frames) { struct snd_pcm_runtime *runtime = substream->runtime; - int err; - char __user *buf = (char __user *) data + frames_to_bytes(runtime, off); - if (substream->ops->copy) { - if ((err = substream->ops->copy(substream, -1, hwoff, buf, frames)) < 0) - return err; - } else { - char *hwbuf = runtime->dma_area + frames_to_bytes(runtime, hwoff); - if (copy_to_user(buf, hwbuf, frames_to_bytes(runtime, frames))) - return -EFAULT; - } - return 0; + char __user *buf = (char __user *)data; + + if (buf == NULL) + return -EINVAL; + + buf += frames_to_bytes(runtime, off); + return substream->ops->copy(substream, -1, hwoff, buf, frames); }
static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, @@ -2518,33 +2481,25 @@ static int snd_pcm_lib_readv_transfer(struct snd_pcm_substream *substream, snd_pcm_uframes_t frames) { struct snd_pcm_runtime *runtime = substream->runtime; - int err; - void __user **bufs = (void __user **)data; - int channels = runtime->channels; + char __user **bufs = (char __user **)data; + char __user *buf; + unsigned int channels = runtime->channels; int c; - if (substream->ops->copy) { - for (c = 0; c < channels; ++c, ++bufs) { - char __user *buf; - if (*bufs == NULL) - continue; - buf = *bufs + samples_to_bytes(runtime, off); - if ((err = substream->ops->copy(substream, c, hwoff, buf, frames)) < 0) - return err; - } - } else { - snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; - for (c = 0; c < channels; ++c, ++bufs) { - char *hwbuf; - char __user *buf; - if (*bufs == NULL) - continue; - - hwbuf = runtime->dma_area + (c * dma_csize) + samples_to_bytes(runtime, hwoff); - buf = *bufs + samples_to_bytes(runtime, off); - if (copy_to_user(buf, hwbuf, samples_to_bytes(runtime, frames))) - return -EFAULT; - } + int err; + + if (bufs == NULL) + return -EINVAL; + + for (c = 0; c < channels; ++c) { + if (bufs[c] == NULL) + continue; + + buf = bufs[c] + samples_to_bytes(runtime, off); + err = substream->ops->copy(substream, c, hwoff, buf, frames); + if (err < 0) + return err; } + return 0; }
In former commits, I introduced an alias for copy operation to PCM frames. As a result, ALSA PCM core has wrapper functions for driver implementation of 'struct snd_pcm_ops.silence' and 'struct snd_pcm_ops.copy'. Some helper functions in ALSA PCM core declares that these two operations can be unified.
In this concept, this commit adds new operation; copy_frames. This is callbacked directly without the wrappers for efficiency. Especially, drivers for non-interleaved frames alignment can execute loop in its own and reduce the number of function calls.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 40 ++++++++++++++++++++++++++-------------- 2 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 6cb8df081787..07e5469a0b55 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -82,6 +82,7 @@ struct snd_pcm_ops { struct timespec *system_ts, struct timespec *audio_ts, struct snd_pcm_audio_tstamp_config *audio_tstamp_config, struct snd_pcm_audio_tstamp_report *audio_tstamp_report); + snd_pcm_copy_frames_t copy_frames; int (*copy)(struct snd_pcm_substream *substream, int channel, snd_pcm_uframes_t pos, void __user *buf, snd_pcm_uframes_t count); diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 8dfe964e8931..13a0c44f5cd1 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -227,17 +227,21 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, if (frames == 0) return;
- if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || - runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { - if (substream->ops->silence) - copy_frames = writei_silence; - else - copy_frames = writei_from_space1; + if (substream->ops->copy_frames) { + copy_frames = substream->ops->copy_frames; } else { - if (substream->ops->silence) - copy_frames = writen_silence; - else - copy_frames = writen_from_space1; + if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || + runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { + if (substream->ops->silence) + copy_frames = writei_silence; + else + copy_frames = writei_from_space1; + } else { + if (substream->ops->silence) + copy_frames = writen_silence; + else + copy_frames = writen_from_space1; + } }
ofs = runtime->silence_start % runtime->buffer_size; @@ -2253,7 +2257,9 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, runtime->channels > 1) return -EINVAL;
- if (substream->ops->copy) + if (substream->ops->copy_frames) + copy_frames = substream->ops->copy_frames; + else if (substream->ops->copy) copy_frames = snd_pcm_lib_write_transfer; else copy_frames = writei_from_space1; @@ -2314,7 +2320,9 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL;
- if (substream->ops->copy) + if (substream->ops->copy_frames) + copy_frames = substream->ops->copy_frames; + else if (substream->ops->copy) copy_frames = snd_pcm_lib_writev_transfer; else copy_frames = writen_from_space1; @@ -2465,7 +2473,9 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_INTERLEAVED) return -EINVAL;
- if (substream->ops->copy) + if (substream->ops->copy_frames) + copy_frames = substream->ops->copy_frames; + else if (substream->ops->copy) copy_frames = snd_pcm_lib_read_transfer; else copy_frames = readi_to_space1; @@ -2523,7 +2533,9 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL;
- if (substream->ops->copy) + if (substream->ops->copy_frames) + copy_frames = substream->ops->copy_frames; + else if (substream->ops->copy) copy_frames = snd_pcm_lib_readv_transfer; else copy_frames = readn_to_space1;
This is an example of new operation for copy_frames callback, in a case of drivers for interleaved frame alignment.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/pci/rme9652/hdsp.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c index fc0face6cdc6..430c6cbcb5f6 100644 --- a/sound/pci/rme9652/hdsp.c +++ b/sound/pci/rme9652/hdsp.c @@ -3930,6 +3930,31 @@ static int snd_hdsp_playback_copy(struct snd_pcm_substream *substream, int chann return count; }
+static int playback_copy_frames(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct hdsp *hdsp = snd_pcm_substream_chip(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + char __user *src = (char __user *)data; + char *channel_buf; + + channel_buf = hdsp_channel_buffer_location(hdsp, + substream->pstr->stream, runtime->channels); + if (snd_BUG_ON(!channel_buf)) + return -EIO; + channel_buf += hwoff * 4; + + if (src == NULL) { + memset(channel_buf, 0, count * 4); + } else { + if (copy_from_user(channel_buf, src, count * 4)) + return -EFAULT; + } + + return 0; +} + static int snd_hdsp_capture_copy(struct snd_pcm_substream *substream, int channel, snd_pcm_uframes_t pos, void __user *dst, snd_pcm_uframes_t count) { @@ -3947,6 +3972,26 @@ static int snd_hdsp_capture_copy(struct snd_pcm_substream *substream, int channe return count; }
+static int capture_copy_frames(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct hdsp *hdsp = snd_pcm_substream_chip(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + char __user *dst = (char __user *)data; + char *channel_buf; + + channel_buf = hdsp_channel_buffer_location(hdsp, + substream->pstr->stream, runtime->channels); + if (snd_BUG_ON(!channel_buf)) + return -EIO; + channel_buf += hwoff * 4; + + if (copy_to_user(dst, channel_buf + hwoff * 4, count * 4)) + return -EFAULT; + return count; +} + static int snd_hdsp_hw_silence(struct snd_pcm_substream *substream, int channel, snd_pcm_uframes_t pos, snd_pcm_uframes_t count) { @@ -4871,6 +4916,7 @@ static const struct snd_pcm_ops snd_hdsp_playback_ops = { .pointer = snd_hdsp_hw_pointer, .copy = snd_hdsp_playback_copy, .silence = snd_hdsp_hw_silence, + .copy_frames = playback_copy_frames, };
static const struct snd_pcm_ops snd_hdsp_capture_ops = { @@ -4882,6 +4928,7 @@ static const struct snd_pcm_ops snd_hdsp_capture_ops = { .trigger = snd_hdsp_trigger, .pointer = snd_hdsp_hw_pointer, .copy = snd_hdsp_capture_copy, + .copy_frames = capture_copy_frames, };
static int snd_hdsp_create_hwdep(struct snd_card *card, struct hdsp *hdsp)
This is an example of new operation for copy_frames callback, in a case of drivers for non-interleaved frame alignment.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/isa/gus/gus_pcm.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/sound/isa/gus/gus_pcm.c b/sound/isa/gus/gus_pcm.c index 33c1891f469a..08bc1f9931a2 100644 --- a/sound/isa/gus/gus_pcm.c +++ b/sound/isa/gus/gus_pcm.c @@ -417,6 +417,63 @@ static int snd_gf1_pcm_playback_silence(struct snd_pcm_substream *substream, return 0; }
+static int playback_copy_frames(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct gus_pcm_private *pcmp = runtime->private_data; + struct snd_gus_card *gus = pcmp->gus; + char __user **bufs = (char __user **)data; + char __user *buf; + unsigned int channels = runtime->channels; + unsigned int len = samples_to_bytes(runtime, count); + unsigned int bpos; + char *dst; + int w16; + int invert; + int c; + int err; + + w16 = !!(snd_pcm_format_width(runtime->format) == 16); + invert = snd_pcm_format_unsigned(runtime->format); + + for (c = 0; c < channels; ++c) { + bpos = samples_to_bytes(runtime, hwoff) + + c * (pcmp->dma_size / 2); + if (snd_BUG_ON(bpos > pcmp->dma_size)) + return -EIO; + if (snd_BUG_ON(bpos + len > pcmp->dma_size)) + return -EIO; + dst = runtime->dma_area + bpos; + + if (bufs == NULL || bufs[c] == NULL) { + snd_pcm_format_set_silence(runtime->format, + runtime->dma_area + bpos, + count); + } else { + buf = bufs[c] + samples_to_bytes(runtime, off); + + if (copy_from_user(runtime->dma_area + bpos, buf, len)) + return -EFAULT; + } + + if (len > 32) { + err = snd_gf1_pcm_block_change(substream, bpos, + pcmp->memory + bpos, len); + } else { + err = snd_gf1_pcm_poke_block(gus, + runtime->dma_area + bpos, + pcmp->memory + bpos, len, w16, + invert); + } + if (err < 0) + return err; + } + + return 0; +} + static int snd_gf1_pcm_playback_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *hw_params) { @@ -838,6 +895,7 @@ static struct snd_pcm_ops snd_gf1_pcm_playback_ops = { .pointer = snd_gf1_pcm_playback_pointer, .copy = snd_gf1_pcm_playback_copy, .silence = snd_gf1_pcm_playback_silence, + .copy_frames = playback_copy_frames, };
static struct snd_pcm_ops snd_gf1_pcm_capture_ops = {
A new operation, copy_frames, can replaces the copy and silence callbacks. This commit purge them involving driver-side implementations.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- include/sound/pcm.h | 5 -- sound/core/pcm_lib.c | 136 ++--------------------------------------------- sound/isa/gus/gus_pcm.c | 64 ---------------------- sound/pci/rme9652/hdsp.c | 50 ----------------- 4 files changed, 3 insertions(+), 252 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 07e5469a0b55..cf97e478b664 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -83,11 +83,6 @@ struct snd_pcm_ops { struct snd_pcm_audio_tstamp_config *audio_tstamp_config, struct snd_pcm_audio_tstamp_report *audio_tstamp_report); snd_pcm_copy_frames_t copy_frames; - int (*copy)(struct snd_pcm_substream *substream, int channel, - snd_pcm_uframes_t pos, - void __user *buf, snd_pcm_uframes_t count); - int (*silence)(struct snd_pcm_substream *substream, int channel, - snd_pcm_uframes_t pos, snd_pcm_uframes_t count); struct page *(*page)(struct snd_pcm_substream *substream, unsigned long offset); int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma); diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 13a0c44f5cd1..7b8d35823de6 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -136,31 +136,6 @@ static int readn_to_space1(struct snd_pcm_substream *substream, return 0; }
-static int writei_silence(struct snd_pcm_substream *substream, - unsigned int hwoff, unsigned long data, - unsigned int off, snd_pcm_uframes_t count) -{ - return substream->ops->silence(substream, -1, hwoff, count); -} - -static int writen_silence(struct snd_pcm_substream *substream, - unsigned int hwoff, unsigned long data, - unsigned int off, snd_pcm_uframes_t count) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - unsigned int channels = runtime->channels; - int c; - int err; - - for (c = 0; c < channels; ++c) { - err = substream->ops->silence(substream, c, hwoff, count); - if (err < 0) - return err; - } - - return 0; -} - /* * fill ring buffer with silence * runtime->silence_start: starting pointer to silence area @@ -232,15 +207,9 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, } else { if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { - if (substream->ops->silence) - copy_frames = writei_silence; - else - copy_frames = writei_from_space1; + copy_frames = writei_from_space1; } else { - if (substream->ops->silence) - copy_frames = writen_silence; - else - copy_frames = writen_from_space1; + copy_frames = writen_from_space1; } }
@@ -2108,21 +2077,6 @@ static int wait_for_avail(struct snd_pcm_substream *substream, return err; } -static int snd_pcm_lib_write_transfer(struct snd_pcm_substream *substream, - unsigned int hwoff, - unsigned long data, unsigned int off, - snd_pcm_uframes_t count) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - char __user *buf = (char __user *) data; - - if (buf == NULL && substream->ops->silence) - return substream->ops->silence(substream, -1, hwoff, count); - - buf += frames_to_bytes(runtime, off); - return substream->ops->copy(substream, -1, hwoff, buf, count); -} - static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, unsigned long data, snd_pcm_uframes_t size, @@ -2232,7 +2186,7 @@ static int pcm_sanity_check(struct snd_pcm_substream *substream) if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; runtime = substream->runtime; - if (snd_BUG_ON(!substream->ops->copy && !runtime->dma_area)) + if (snd_BUG_ON(!substream->ops->copy_frames && !runtime->dma_area)) return -EINVAL; if (runtime->status->state == SNDRV_PCM_STATE_OPEN) return -EBADFD; @@ -2259,8 +2213,6 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream,
if (substream->ops->copy_frames) copy_frames = substream->ops->copy_frames; - else if (substream->ops->copy) - copy_frames = snd_pcm_lib_write_transfer; else copy_frames = writei_from_space1;
@@ -2269,39 +2221,6 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, } EXPORT_SYMBOL(snd_pcm_lib_write);
-static int snd_pcm_lib_writev_transfer(struct snd_pcm_substream *substream, - unsigned int hwoff, - unsigned long data, unsigned int off, - snd_pcm_uframes_t frames) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - char __user **bufs = (char __user **)data; - char __user *buf; - int channels = runtime->channels; - int c; - int err; - - if (snd_BUG_ON(!substream->ops->silence)) - return -EINVAL; - - for (c = 0; c < channels; ++c) { - if (bufs == NULL || bufs[c] == NULL) { - err = substream->ops->silence(substream, c, hwoff, - frames); - if (err < 0) - return err; - } else { - buf = bufs[c] + samples_to_bytes(runtime, off); - err = substream->ops->copy(substream, c, hwoff, buf, - frames); - if (err < 0) - return err; - } - } - - return 0; -} - snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, void __user **bufs, snd_pcm_uframes_t frames) @@ -2322,8 +2241,6 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream,
if (substream->ops->copy_frames) copy_frames = substream->ops->copy_frames; - else if (substream->ops->copy) - copy_frames = snd_pcm_lib_writev_transfer; else copy_frames = writen_from_space1;
@@ -2332,21 +2249,6 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, } EXPORT_SYMBOL(snd_pcm_lib_writev);
-static int snd_pcm_lib_read_transfer(struct snd_pcm_substream *substream, - unsigned int hwoff, - unsigned long data, unsigned int off, - snd_pcm_uframes_t frames) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - char __user *buf = (char __user *)data; - - if (buf == NULL) - return -EINVAL; - - buf += frames_to_bytes(runtime, off); - return substream->ops->copy(substream, -1, hwoff, buf, frames); -} - static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, unsigned long data, snd_pcm_uframes_t size, @@ -2475,8 +2377,6 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream,
if (substream->ops->copy_frames) copy_frames = substream->ops->copy_frames; - else if (substream->ops->copy) - copy_frames = snd_pcm_lib_read_transfer; else copy_frames = readi_to_space1;
@@ -2485,34 +2385,6 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, } EXPORT_SYMBOL(snd_pcm_lib_read);
-static int snd_pcm_lib_readv_transfer(struct snd_pcm_substream *substream, - unsigned int hwoff, - unsigned long data, unsigned int off, - snd_pcm_uframes_t frames) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - char __user **bufs = (char __user **)data; - char __user *buf; - unsigned int channels = runtime->channels; - int c; - int err; - - if (bufs == NULL) - return -EINVAL; - - for (c = 0; c < channels; ++c) { - if (bufs[c] == NULL) - continue; - - buf = bufs[c] + samples_to_bytes(runtime, off); - err = substream->ops->copy(substream, c, hwoff, buf, frames); - if (err < 0) - return err; - } - - return 0; -} - snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, void __user **bufs, snd_pcm_uframes_t frames) @@ -2535,8 +2407,6 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream,
if (substream->ops->copy_frames) copy_frames = substream->ops->copy_frames; - else if (substream->ops->copy) - copy_frames = snd_pcm_lib_readv_transfer; else copy_frames = readn_to_space1;
diff --git a/sound/isa/gus/gus_pcm.c b/sound/isa/gus/gus_pcm.c index 08bc1f9931a2..48bef91f46ef 100644 --- a/sound/isa/gus/gus_pcm.c +++ b/sound/isa/gus/gus_pcm.c @@ -355,68 +355,6 @@ static int snd_gf1_pcm_poke_block(struct snd_gus_card *gus, unsigned char *buf, return 0; }
-static int snd_gf1_pcm_playback_copy(struct snd_pcm_substream *substream, - int voice, - snd_pcm_uframes_t pos, - void __user *src, - snd_pcm_uframes_t count) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct gus_pcm_private *pcmp = runtime->private_data; - unsigned int bpos, len; - - bpos = samples_to_bytes(runtime, pos) + (voice * (pcmp->dma_size / 2)); - len = samples_to_bytes(runtime, count); - if (snd_BUG_ON(bpos > pcmp->dma_size)) - return -EIO; - if (snd_BUG_ON(bpos + len > pcmp->dma_size)) - return -EIO; - if (copy_from_user(runtime->dma_area + bpos, src, len)) - return -EFAULT; - if (snd_gf1_pcm_use_dma && len > 32) { - return snd_gf1_pcm_block_change(substream, bpos, pcmp->memory + bpos, len); - } else { - struct snd_gus_card *gus = pcmp->gus; - int err, w16, invert; - - w16 = (snd_pcm_format_width(runtime->format) == 16); - invert = snd_pcm_format_unsigned(runtime->format); - if ((err = snd_gf1_pcm_poke_block(gus, runtime->dma_area + bpos, pcmp->memory + bpos, len, w16, invert)) < 0) - return err; - } - return 0; -} - -static int snd_gf1_pcm_playback_silence(struct snd_pcm_substream *substream, - int voice, - snd_pcm_uframes_t pos, - snd_pcm_uframes_t count) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct gus_pcm_private *pcmp = runtime->private_data; - unsigned int bpos, len; - - bpos = samples_to_bytes(runtime, pos) + (voice * (pcmp->dma_size / 2)); - len = samples_to_bytes(runtime, count); - if (snd_BUG_ON(bpos > pcmp->dma_size)) - return -EIO; - if (snd_BUG_ON(bpos + len > pcmp->dma_size)) - return -EIO; - snd_pcm_format_set_silence(runtime->format, runtime->dma_area + bpos, count); - if (snd_gf1_pcm_use_dma && len > 32) { - return snd_gf1_pcm_block_change(substream, bpos, pcmp->memory + bpos, len); - } else { - struct snd_gus_card *gus = pcmp->gus; - int err, w16, invert; - - w16 = (snd_pcm_format_width(runtime->format) == 16); - invert = snd_pcm_format_unsigned(runtime->format); - if ((err = snd_gf1_pcm_poke_block(gus, runtime->dma_area + bpos, pcmp->memory + bpos, len, w16, invert)) < 0) - return err; - } - return 0; -} - static int playback_copy_frames(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -893,8 +831,6 @@ static struct snd_pcm_ops snd_gf1_pcm_playback_ops = { .prepare = snd_gf1_pcm_playback_prepare, .trigger = snd_gf1_pcm_playback_trigger, .pointer = snd_gf1_pcm_playback_pointer, - .copy = snd_gf1_pcm_playback_copy, - .silence = snd_gf1_pcm_playback_silence, .copy_frames = playback_copy_frames, };
diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c index 430c6cbcb5f6..d1a86b16444b 100644 --- a/sound/pci/rme9652/hdsp.c +++ b/sound/pci/rme9652/hdsp.c @@ -3913,23 +3913,6 @@ static char *hdsp_channel_buffer_location(struct hdsp *hdsp, return hdsp->playback_buffer + (mapped_channel * HDSP_CHANNEL_BUFFER_BYTES); }
-static int snd_hdsp_playback_copy(struct snd_pcm_substream *substream, int channel, - snd_pcm_uframes_t pos, void __user *src, snd_pcm_uframes_t count) -{ - struct hdsp *hdsp = snd_pcm_substream_chip(substream); - char *channel_buf; - - if (snd_BUG_ON(pos + count > HDSP_CHANNEL_BUFFER_BYTES / 4)) - return -EINVAL; - - channel_buf = hdsp_channel_buffer_location (hdsp, substream->pstr->stream, channel); - if (snd_BUG_ON(!channel_buf)) - return -EIO; - if (copy_from_user(channel_buf + pos * 4, src, count * 4)) - return -EFAULT; - return count; -} - static int playback_copy_frames(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -3955,23 +3938,6 @@ static int playback_copy_frames(struct snd_pcm_substream *substream, return 0; }
-static int snd_hdsp_capture_copy(struct snd_pcm_substream *substream, int channel, - snd_pcm_uframes_t pos, void __user *dst, snd_pcm_uframes_t count) -{ - struct hdsp *hdsp = snd_pcm_substream_chip(substream); - char *channel_buf; - - if (snd_BUG_ON(pos + count > HDSP_CHANNEL_BUFFER_BYTES / 4)) - return -EINVAL; - - channel_buf = hdsp_channel_buffer_location (hdsp, substream->pstr->stream, channel); - if (snd_BUG_ON(!channel_buf)) - return -EIO; - if (copy_to_user(dst, channel_buf + pos * 4, count * 4)) - return -EFAULT; - return count; -} - static int capture_copy_frames(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -3992,19 +3958,6 @@ static int capture_copy_frames(struct snd_pcm_substream *substream, return count; }
-static int snd_hdsp_hw_silence(struct snd_pcm_substream *substream, int channel, - snd_pcm_uframes_t pos, snd_pcm_uframes_t count) -{ - struct hdsp *hdsp = snd_pcm_substream_chip(substream); - char *channel_buf; - - channel_buf = hdsp_channel_buffer_location (hdsp, substream->pstr->stream, channel); - if (snd_BUG_ON(!channel_buf)) - return -EIO; - memset(channel_buf + pos * 4, 0, count * 4); - return count; -} - static int snd_hdsp_reset(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -4914,8 +4867,6 @@ static const struct snd_pcm_ops snd_hdsp_playback_ops = { .prepare = snd_hdsp_prepare, .trigger = snd_hdsp_trigger, .pointer = snd_hdsp_hw_pointer, - .copy = snd_hdsp_playback_copy, - .silence = snd_hdsp_hw_silence, .copy_frames = playback_copy_frames, };
@@ -4927,7 +4878,6 @@ static const struct snd_pcm_ops snd_hdsp_capture_ops = { .prepare = snd_hdsp_prepare, .trigger = snd_hdsp_trigger, .pointer = snd_hdsp_hw_pointer, - .copy = snd_hdsp_capture_copy, .copy_frames = capture_copy_frames, };
In current design of ALSA PCM core, copying PCM frames is performed between user/kernel spaces. However, some in-kernel drivers wish to copy between kernel/kernel spaces. I call such drivers as 'proxy' drivers.
This commit adds a infrastructure to assist the proxy drivers. New helper functions are added to copy innner kernel space. A new member, 'client_space', is added into runtime of PCM substream to utilize these helper functions. Usually, this member has 1 to represent target PCM frames on user space. When this member is 0, typical copying functions are executed between kernel/kernel space.
Reference: http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120542.html Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 147 +++++++++++++++++++++++++++++++++++++++++++----- sound/core/pcm_native.c | 8 +++ 3 files changed, 141 insertions(+), 15 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index cf97e478b664..ffd21fe0b284 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -387,6 +387,7 @@ struct snd_pcm_runtime { /* -- mmap -- */ struct snd_pcm_mmap_status *status; struct snd_pcm_mmap_control *control; + int client_space; /* Where the client puts PCM frames. Usually, 1 means user space. */
/* -- locking / scheduling -- */ snd_pcm_uframes_t twake; /* do transfer (!poll) wakeup if non-zero */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 7b8d35823de6..7700be3bb0c8 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,6 +42,22 @@ #define trace_hw_ptr_error(substream, reason) #endif
+static int writei_from_space0(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char *buf = (char *)data; + char *dst = runtime->dma_area + frames_to_bytes(runtime, hwoff); + + if (buf != NULL) + snd_pcm_format_set_silence(runtime->format, dst, count); + else + memcpy(dst, buf, frames_to_bytes(runtime, count)); + + return 0; +} + static int writei_from_space1(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -61,6 +77,36 @@ static int writei_from_space1(struct snd_pcm_substream *substream, return 0; }
+static int writen_from_space0(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char **bufs = (char **)data; + char *buf; + char *dst; + int channels = runtime->channels; + snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; + int c; + + for (c = 0; c < channels; ++c) { + dst = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + + if (bufs == NULL || bufs[c] == NULL) { + snd_pcm_format_set_silence(runtime->format, dst, count); + continue; + } + buf = bufs[c] + samples_to_bytes(runtime, off); + + dst = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + memcpy(dst, buf, samples_to_bytes(runtime, count)); + } + + return 0; +} + static int writen_from_space1(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -92,6 +138,19 @@ static int writen_from_space1(struct snd_pcm_substream *substream, return 0; }
+static int readi_to_space0(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char *buf = (char *)data + frames_to_bytes(runtime, off); + char *src = runtime->dma_area + frames_to_bytes(runtime, hwoff); + + memcpy(buf, src, frames_to_bytes(runtime, count)); + + return 0; +} + static int readi_to_space1(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -110,6 +169,31 @@ static int readi_to_space1(struct snd_pcm_substream *substream, return 0; }
+static int readn_to_space0(struct snd_pcm_substream *substream, + unsigned int hwoff, unsigned long data, + unsigned int off, snd_pcm_uframes_t count) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + char **bufs = (char **)data; + char *buf; + char *src; + unsigned int channels = runtime->channels; + snd_pcm_uframes_t dma_csize = runtime->dma_bytes / channels; + int c; + + for (c = 0; c < channels; ++c) { + if (bufs == NULL || bufs[c] == NULL) + continue; + buf = bufs[c] + samples_to_bytes(runtime, off); + + src = runtime->dma_area + (c * dma_csize) + + samples_to_bytes(runtime, hwoff); + memcpy(buf, src, samples_to_bytes(runtime, count)); + } + + return 0; +} + static int readn_to_space1(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) @@ -203,13 +287,21 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, return;
if (substream->ops->copy_frames) { + if (runtime->client_space == 0) + return; copy_frames = substream->ops->copy_frames; } else { if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { - copy_frames = writei_from_space1; + if (runtime->client_space == 0) + copy_frames = writei_from_space0; + else + copy_frames = writei_from_space1; } else { - copy_frames = writen_from_space1; + if (rnutime->client-space == 0) + copy_frames = writen_from_space0; + else + copy_frames = writen_from_space1; } }
@@ -2211,10 +2303,16 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, runtime->channels > 1) return -EINVAL;
- if (substream->ops->copy_frames) + if (substream->ops->copy_frames) { + if (runtime->client_space == 0) + return -ENXIO; copy_frames = substream->ops->copy_frames; - else - copy_frames = writei_from_space1; + } else { + if (runtime->client_space == 0) + copy_frames = writei_from_space0; + else + copy_frames = writei_from_space1; + }
return snd_pcm_lib_write1(substream, (unsigned long)buf, size, nonblock, copy_frames); @@ -2239,10 +2337,17 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL;
- if (substream->ops->copy_frames) - copy_frames = substream->ops->copy_frames; - else - copy_frames = writen_from_space1; + if (substream->ops->copy_frames) { + if (runtime->client_space == 0) + return -ENXIO; + else + copy_frames = substream->ops->copy_frames; + } else { + if (runtime->client_space == 0) + copy_frames = writen_from_space0; + else + copy_frames = writen_from_space1; + }
return snd_pcm_lib_write1(substream, (unsigned long)bufs, frames, nonblock, copy_frames); @@ -2375,10 +2480,16 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_INTERLEAVED) return -EINVAL;
- if (substream->ops->copy_frames) + if (substream->ops->copy_frames) { + if (runtime->client_space == 0) + return -ENXIO; copy_frames = substream->ops->copy_frames; - else - copy_frames = readi_to_space1; + } else { + if (runtime->client_space == 0) + copy_frames = readi_to_space0; + else + copy_frames = readi_to_space1; + }
return snd_pcm_lib_read1(substream, (unsigned long)buf, size, nonblock, copy_frames); @@ -2405,10 +2516,16 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, if (runtime->access != SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) return -EINVAL;
- if (substream->ops->copy_frames) + if (substream->ops->copy_frames) { + if (runtime->client_space == 0) + return -ENXIO; copy_frames = substream->ops->copy_frames; - else - copy_frames = readn_to_space1; + } else { + if (runtime->client_space == 0) + copy_frames = readn_to_space0; + else + copy_frames = readn_to_space1; + }
return snd_pcm_lib_read1(substream, (unsigned long)bufs, frames, nonblock, copy_frames); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index ecde57afa45a..3428583ac0d7 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -599,6 +599,14 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, if ((usecs = period_to_usecs(runtime)) >= 0) pm_qos_add_request(&substream->latency_pm_qos_req, PM_QOS_CPU_DMA_LATENCY, usecs); + + /* + * Usual client puts PCM frames on user space, on the other + * hand PCM proxy drivers puts on kernel space. This is a + * switch handlers for PCM frames in different spaces. + */ + runtime->client_space = 1; + return 0; _error: /* hardware might be unusable from this time,
In a previous commit, ALSA PCM core got support for 'proxy' drivers. However, in current plave, such drivers are quite minor. They're not used so often. It's good for users to switch enabling/disabling the feature, to reduce memory footprint.
This commit adds a new configuration; CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT. Existent proxy drivers get dependency on the configuration.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- drivers/usb/gadget/Kconfig | 3 +-- include/sound/pcm.h | 2 ++ sound/core/Kconfig | 15 +++++++++++- sound/core/pcm_lib.c | 59 +++++++++++++++++++++++++++++----------------- sound/core/pcm_native.c | 14 ++++++----- 5 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index c164d6b788c3..735b5cfa35ef 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -365,10 +365,9 @@ config USB_CONFIGFS_F_FS config USB_CONFIGFS_F_UAC1 bool "Audio Class 1.0" depends on USB_CONFIGFS - depends on SND select USB_LIBCOMPOSITE - select SND_PCM select USB_F_UAC1 + select SND_PCM_PROXY_DRIVER_SUPPORT help This Audio function implements 1 AudioControl interface, 1 AudioStreaming Interface each for USB-OUT and USB-IN. diff --git a/include/sound/pcm.h b/include/sound/pcm.h index ffd21fe0b284..08d5ac117138 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -387,7 +387,9 @@ struct snd_pcm_runtime { /* -- mmap -- */ struct snd_pcm_mmap_status *status; struct snd_pcm_mmap_control *control; +#if IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) int client_space; /* Where the client puts PCM frames. Usually, 1 means user space. */ +#endif
/* -- locking / scheduling -- */ snd_pcm_uframes_t twake; /* do transfer (!poll) wakeup if non-zero */ diff --git a/sound/core/Kconfig b/sound/core/Kconfig index 9749f9e8b45c..f9660db5759e 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -58,6 +58,19 @@ config SND_SEQ_DUMMY To compile this driver as a module, choose M here: the module will be called snd-seq-dummy.
+config SND_PCM_PROXY_DRIVER_SUPPORT + bool "PCM Proxy driver support" + default n + depends on SND_PCM + help + This feature is for PCM proxy driver support, which drives the + other PCM drivers in kernel space. In a view of userspace + applications, the driver gives non-ALSA interfaces to drive PCM + hardwares. This config adds infrastructure for such drivers, to + handle PCM frames in kernel space. + The proxy drivers have a limitation of available ALSA drivers, + which can delegate data transmission to stuffs in ALSA PCM core. + config SND_OSSEMUL select SOUND_OSS_CORE bool @@ -77,7 +90,7 @@ config SND_MIXER_OSS config SND_PCM_OSS tristate "OSS PCM (digital audio) API" select SND_OSSEMUL - select SND_PCM + select SND_PCM_PROXY_DRIVER_SUPPORT help To enable OSS digital audio (PCM) emulation (/dev/dsp*), say Y here and read file:Documentation/sound/alsa/OSS-Emulation.txt. diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 7700be3bb0c8..2d9d1b3b6fe0 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,7 +42,7 @@ #define trace_hw_ptr_error(substream, reason) #endif
-static int writei_from_space0(struct snd_pcm_substream *substream, +static int __maybe_unused writei_from_space0(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) { @@ -77,7 +77,7 @@ static int writei_from_space1(struct snd_pcm_substream *substream, return 0; }
-static int writen_from_space0(struct snd_pcm_substream *substream, +static int __maybe_unused writen_from_space0(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) { @@ -138,7 +138,7 @@ static int writen_from_space1(struct snd_pcm_substream *substream, return 0; }
-static int readi_to_space0(struct snd_pcm_substream *substream, +static int __maybe_unused readi_to_space0(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) { @@ -169,7 +169,7 @@ static int readi_to_space1(struct snd_pcm_substream *substream, return 0; }
-static int readn_to_space0(struct snd_pcm_substream *substream, +static int __maybe_unused readn_to_space0(struct snd_pcm_substream *substream, unsigned int hwoff, unsigned long data, unsigned int off, snd_pcm_uframes_t count) { @@ -287,19 +287,23 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, return;
if (substream->ops->copy_frames) { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) return; - copy_frames = substream->ops->copy_frames; + else + copy_frames = substream->ops->copy_frames; } else { if (runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) copy_frames = writei_from_space0; else copy_frames = writei_from_space1; } else { - if (rnutime->client-space == 0) - copy_frames = writen_from_space0; + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) + copy_frames = writen_from_space1; else copy_frames = writen_from_space1; } @@ -2304,11 +2308,14 @@ snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream, return -EINVAL;
if (substream->ops->copy_frames) { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) return -ENXIO; - copy_frames = substream->ops->copy_frames; + else + copy_frames = substream->ops->copy_frames; } else { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) copy_frames = writei_from_space0; else copy_frames = writei_from_space1; @@ -2338,12 +2345,14 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream, return -EINVAL;
if (substream->ops->copy_frames) { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) return -ENXIO; else copy_frames = substream->ops->copy_frames; } else { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) copy_frames = writen_from_space0; else copy_frames = writen_from_space1; @@ -2481,11 +2490,14 @@ snd_pcm_sframes_t snd_pcm_lib_read(struct snd_pcm_substream *substream, return -EINVAL;
if (substream->ops->copy_frames) { - if (runtime->client_space == 0) - return -ENXIO; - copy_frames = substream->ops->copy_frames; + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) + return -ENXIO; + else + copy_frames = substream->ops->copy_frames; } else { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) copy_frames = readi_to_space0; else copy_frames = readi_to_space1; @@ -2517,11 +2529,14 @@ snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream, return -EINVAL;
if (substream->ops->copy_frames) { - if (runtime->client_space == 0) - return -ENXIO; - copy_frames = substream->ops->copy_frames; + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) + return -ENXIO; + else + copy_frames = substream->ops->copy_frames; } else { - if (runtime->client_space == 0) + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT) && + runtime->client_space == 0) copy_frames = readn_to_space0; else copy_frames = readn_to_space1; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 3428583ac0d7..c6f310c17657 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -600,12 +600,14 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, pm_qos_add_request(&substream->latency_pm_qos_req, PM_QOS_CPU_DMA_LATENCY, usecs);
- /* - * Usual client puts PCM frames on user space, on the other - * hand PCM proxy drivers puts on kernel space. This is a - * switch handlers for PCM frames in different spaces. - */ - runtime->client_space = 1; + if (IS_ENABLED(CONFIG_SND_PCM_PROXY_DRIVER_SUPPORT)) { + /* + * Usual client puts PCM frames on user space, on the other + * hand PCM proxy drivers puts on kernel space. This is a + * switch handlers for PCM frames in different spaces. + */ + runtime->client_space = 1; + }
return 0; _error:
On Wed, 24 May 2017 02:52:44 +0200, Takashi Sakamoto wrote:
Hi,
This RFC patchset is my concept work of another solution against a series of issues in ALSA PCM core, for which Iwai Takashi work in his hasty proposals[2][3]. Included changes are also available in my personal repository on github.com[4].
The aim of this work is to declare existent issues and to propose solutions as moderated as possible. Code refactoring is done to assist it.
Patch 01-05: code refactoring phase, with an alias of function prototype. Patch 06-09: driver API changing phase. The .copy_frames is introduced. Patch 10-11: integration phase for proxy drivers, to get new configuration.
I need to do this work with a little time (half a day), thus changes are as minimal as possible, especially two drivers are just modified even if drivers in below list should be changed:
- drivers/media/pci/solo6x10/solo6x10-g723.c
- sound/drivers/dummy.c
- sound/isa/gus/gus_pcm.c
- sound/isa/sb/emu8000_pcm.c
- sound/pci/es1938.c
- sound/pci/korg1212/korg1212.c
- sound/pci/nm256/nm256.c
- sound/pci/rme32.c
- sound/pci/rme96.c
- sound/pci/rme9652/hdsp.c
- sound/pci/rme9652/rme9652.c
- sound/sh/sh_dac_audio.c
- sound/soc/blackfin/bf5xx-ac97-pcm.c
- sound/soc/blackfin/bf5xx-i2s-pcm.c
Furthermore, I apologize that this is untested. I wish you to check the concept.
OK, I now understand your idea. Actually the word "proxy" was misleading. "proxy" implies the action, but PCM core can't know the action the caller intended. From PCM core POV, it's merely an in-kernel buffer copy. We don't need to invent new terms here (who is the client?).
Apart from that, basically your idea is: - Keep in-kernel buffer copy flag into substream->runtime. - The ops handles both copy and silence. - Move the whole transfer action to each driver instead of looping in PCM core.
I find the first point is acceptable, from the current usage pattern, as there is little chance to mix up both user-space buffer and kernel-space buffer.
Though, another side of coin is that, by embedding in runtime, you can easily overlook the in-kernel copy case, in comparison with passing via the argument. So it's not always plus.
The second point is as same as mine.
The potential problem is the third point. It'd require a larger change, and it's error-prone, because you'll have to translate the passed pointer in each driver to morph it to different values, either the linear buffer pointer or the vector, in addition to the user-space and kernel-space check.
In the PCM core code, we did that in that way, but it's OK since it's done only there thus it's manageable. However, forcing the complex cast in each driver implementation is better to avoid.
How to make each driver implementing less error-prone codes -- that's what we need to reconsider further.
thanks,
Takashi
[1] [alsa-devel] [PATCH RFC 00/26] Kill set_fs() in ALSA codes http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120542 [2] [alsa-devel] [PATCH 00/16] ALSA: Convert to new copy_silence PCM ops http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120895.html [3] [alsa-devel] [PATCH 0/4] ALSA: Extend PCM buffer-copy helpers http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120957.html [4] takaswie/sound https://github.com/takaswie/sound/tree/topic/add-pcm-frame-ops
Takashi Sakamoto (11): ALSA: pcm: introduce an alias of prototype to copy PCM frames ALSA: pcm: use new function alias instead of local one ALSA: pcm: refactoring silence operation ALSA: pcm: add alternative calls ALSA: core: remove duplicated codes to handle PCM frames ALSA: pcm: add new operation; copy_frames ALSA: rme9652: a sample for drivers which support interleaved buffer ALSA: gus: a sample for drivers which support non-interleaved buffer ALSA: pcm: remove copy and silence callbacks ALSA: pcm: add client_space parameter to runtime of PCM substream for PCM proxy drivers ALSA: pcm: add new configuration for PCM proxy drivers
drivers/usb/gadget/Kconfig | 3 +- include/sound/pcm.h | 13 +- sound/core/Kconfig | 15 +- sound/core/pcm_lib.c | 448 +++++++++++++++++++++++++++++---------------- sound/core/pcm_native.c | 10 + sound/isa/gus/gus_pcm.c | 100 +++++----- sound/pci/rme9652/hdsp.c | 59 +++--- 7 files changed, 398 insertions(+), 250 deletions(-)
-- 2.11.0
On Wed, 24 May 2017 08:29:28 +0200, Takashi Iwai wrote:
On Wed, 24 May 2017 02:52:44 +0200, Takashi Sakamoto wrote:
Hi,
This RFC patchset is my concept work of another solution against a series of issues in ALSA PCM core, for which Iwai Takashi work in his hasty proposals[2][3]. Included changes are also available in my personal repository on github.com[4].
The aim of this work is to declare existent issues and to propose solutions as moderated as possible. Code refactoring is done to assist it.
Patch 01-05: code refactoring phase, with an alias of function prototype. Patch 06-09: driver API changing phase. The .copy_frames is introduced. Patch 10-11: integration phase for proxy drivers, to get new configuration.
I need to do this work with a little time (half a day), thus changes are as minimal as possible, especially two drivers are just modified even if drivers in below list should be changed:
- drivers/media/pci/solo6x10/solo6x10-g723.c
- sound/drivers/dummy.c
- sound/isa/gus/gus_pcm.c
- sound/isa/sb/emu8000_pcm.c
- sound/pci/es1938.c
- sound/pci/korg1212/korg1212.c
- sound/pci/nm256/nm256.c
- sound/pci/rme32.c
- sound/pci/rme96.c
- sound/pci/rme9652/hdsp.c
- sound/pci/rme9652/rme9652.c
- sound/sh/sh_dac_audio.c
- sound/soc/blackfin/bf5xx-ac97-pcm.c
- sound/soc/blackfin/bf5xx-i2s-pcm.c
Furthermore, I apologize that this is untested. I wish you to check the concept.
OK, I now understand your idea. Actually the word "proxy" was misleading. "proxy" implies the action, but PCM core can't know the action the caller intended. From PCM core POV, it's merely an in-kernel buffer copy. We don't need to invent new terms here (who is the client?).
Apart from that, basically your idea is:
- Keep in-kernel buffer copy flag into substream->runtime.
- The ops handles both copy and silence.
- Move the whole transfer action to each driver instead of looping in PCM core.
I find the first point is acceptable, from the current usage pattern, as there is little chance to mix up both user-space buffer and kernel-space buffer.
Actually it's even dynamically switched in PCM OSS emulation at each read/write. Having the flag in runtime field would still work because the OSS read/write takes the mutex, but you see that it can be more volatile than one thinks.
Though, another side of coin is that, by embedding in runtime, you can easily overlook the in-kernel copy case, in comparison with passing via the argument. So it's not always plus.
The second point is as same as mine.
The potential problem is the third point. It'd require a larger change, and it's error-prone, because you'll have to translate the passed pointer in each driver to morph it to different values, either the linear buffer pointer or the vector, in addition to the user-space and kernel-space check.
In the PCM core code, we did that in that way, but it's OK since it's done only there thus it's manageable. However, forcing the complex cast in each driver implementation is better to avoid.
How to make each driver implementing less error-prone codes -- that's what we need to reconsider further.
... and now reading back your patches, I still don't figure out how you achieve the in-kernel data transfer via your new copy_frames ops. For example, with your patches gus_pcm.c, how OSS or UAC1 gadget would work?
What am I missing?
thanks,
Takashi
Hi,
On May 24 2017 15:29, Takashi Iwai wrote:
OK, I now understand your idea. Actually the word "proxy" was misleading. "proxy" implies the action, but PCM core can't know the action the caller intended. From PCM core POV, it's merely an in-kernel buffer copy. We don't need to invent new terms here (who is the client?).
Currently, I have no idea for the better term.
Data transmission is a kind of communication, thus there's a originator. I use the 'client' as the originator. Usually, userspace applications are the client, however in addressed issue in-kernel OSS/UAC1 drivers are the client. They perform as a proxy for their direct clients.
Apart from that, basically your idea is:
- Keep in-kernel buffer copy flag into substream->runtime.
- The ops handles both copy and silence.
- Move the whole transfer action to each driver instead of looping in PCM core.
I find the first point is acceptable, from the current usage pattern, as there is little chance to mix up both user-space buffer and kernel-space buffer.
Practically, it's rarely for clients to request copy operation for source or destination on several memory spaces.
Though, another side of coin is that, by embedding in runtime, you can easily overlook the in-kernel copy case, in comparison with passing via the argument. So it's not always plus.
However, in current place, drivers with kernel/kernel copying is quite rare. At least, they're not major ones. Features for them should be selectable by configurations. In this point, such argument-oriented implementation is exaggerated for the features. It's not worth to change existent kernel APIs.
The second point is as same as mine.
The potential problem is the third point. It'd require a larger change, and it's error-prone, because you'll have to translate the passed pointer in each driver to morph it to different values, either the linear buffer pointer or the vector, in addition to the user-space and kernel-space check.
In the PCM core code, we did that in that way, but it's OK since it's done only there thus it's manageable. However, forcing the complex cast in each driver implementation is better to avoid.
How to make each driver implementing less error-prone codes -- that's what we need to reconsider further.
I have no objection for your view. It's natural idea for software developers.
Essentially, it's not better idea to produce one callback function for two types of buffer access. Type casting is enough dangerous and make it hard to trace arguments, but current PCM core implementation heavily utilizes it. I can assume to add two callback operations to driver programming API; for interleaved buffer and non-interleaved buffer.
...but I'd like to postpone this discussion enough later (next week), because we have several patches worth to be merged[1][2][3]. Furthermore, patch 03-05 in this patchset are valuable as a refactoring (of course need to be revised), independent of the discussion. I suggest to put our priority to merging/reviewing them in this week, if you don't mind. After arranging code bases with better shape, we can continue the discussion with a calm mind.
There's no need to jump.
[1] [alsa-devel] [PATCH 0/7] ALSA: Fix/improve PCM ack callback http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120886.html [2] [alsa-devel] [PATCH] ALSA: gus: remove unused local flag http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120967.html [3] [alsa-devel] [PATCH] ALSA: sb: remove needless evaluation in implementation for copy callback http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120968.html
Thanks
Takashi Sakamoto
On Wed, 24 May 2017 16:19:06 +0200, Takashi Sakamoto wrote:
Hi,
On May 24 2017 15:29, Takashi Iwai wrote:
OK, I now understand your idea. Actually the word "proxy" was misleading. "proxy" implies the action, but PCM core can't know the action the caller intended. From PCM core POV, it's merely an in-kernel buffer copy. We don't need to invent new terms here (who is the client?).
Currently, I have no idea for the better term.
Data transmission is a kind of communication, thus there's a originator. I use the 'client' as the originator. Usually, userspace applications are the client, however in addressed issue in-kernel OSS/UAC1 drivers are the client. They perform as a proxy for their direct clients.
A good way is to just look through the kernel code and see what other people use for the similar aim.
Apart from that, basically your idea is:
- Keep in-kernel buffer copy flag into substream->runtime.
- The ops handles both copy and silence.
- Move the whole transfer action to each driver instead of looping in PCM core.
I find the first point is acceptable, from the current usage pattern, as there is little chance to mix up both user-space buffer and kernel-space buffer.
Practically, it's rarely for clients to request copy operation for source or destination on several memory spaces.
Though, another side of coin is that, by embedding in runtime, you can easily overlook the in-kernel copy case, in comparison with passing via the argument. So it's not always plus.
However, in current place, drivers with kernel/kernel copying is quite rare. At least, they're not major ones. Features for them should be selectable by configurations. In this point, such argument-oriented implementation is exaggerated for the features. It's not worth to change existent kernel APIs.
The additional kconfig is also both merit and demerit. More kconfigs are more mess, in general, and we're trying really hard not to add a new kconfig if it doesn't give a very clear benefit. And, this case is in a gray zone.
Also, the rarity in the code isn't always a good judgment point, either. Even though it's used only in few places of the code, it's the code most of distros enable. So practically seen, it's almost always enabled.
The second point is as same as mine.
The potential problem is the third point. It'd require a larger change, and it's error-prone, because you'll have to translate the passed pointer in each driver to morph it to different values, either the linear buffer pointer or the vector, in addition to the user-space and kernel-space check.
In the PCM core code, we did that in that way, but it's OK since it's done only there thus it's manageable. However, forcing the complex cast in each driver implementation is better to avoid.
How to make each driver implementing less error-prone codes -- that's what we need to reconsider further.
I have no objection for your view. It's natural idea for software developers.
Essentially, it's not better idea to produce one callback function for two types of buffer access. Type casting is enough dangerous and make it hard to trace arguments, but current PCM core implementation heavily utilizes it. I can assume to add two callback operations to driver programming API; for interleaved buffer and non-interleaved buffer.
Well, the original copy/silence ops aren't bad in that perspective; their purposes are simple, just copy or fill silence on the given position from the given pointer, no matter which mode it is. So I think we should keep this semantic simplicity.
I really appreciate your patches, but I pushed back just by a simple reason: I already experimented that approach in the past, thus I know it doesn't fly. By moving the complexity from PCM core to the driver ops (e.g. looping over channels inside callback) makes the PCM code simpler, but at the same time, it makes the callback more complex than now. Keeping the driver side implementation simpler is much more benefit overall.
And, taking a look back, I believe that merging both copy and silence operations into a single ops was a bad idea, too. Although it helped reducing the code -- which is a good sign in one side -- it turned out to make the code flow in the callback more complicated. You'll have to do ugly NULL-check in each place.
Then, now we're dealing with another conditional for in-kernel copy... It makes the callback even more complicated.
So, for now, I'm inclined to go back and just to add a new ops for in-kernel copying. Then the __user pointer cast isn't needed, and the purpose of each ops is clear. The drawback is, of course, a slightly more code than the merged ops, but it's the cost for clarity.
But one thing I'm experimenting now, while we're at it, is to change the arguments passed to copy/silence ops. So far, they take frames. It's good in most cases, but for copy/silence, it's often rather a burden than a benefit. Instead, passing bytes fit better in many callback implementations, and above all, it makes the meaning of position and size consistent in both interleaved and non-interleaved modes. This allowed me to unify the transfer codes in PCM core.
I'm going to submit a demo-patchset shortly later.
thanks,
Takashi
participants (2)
-
Takashi Iwai -
Takashi Sakamoto