[PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[Takashi Sakamoto]

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(-)

-- 
2.11.0

[PATCH RFC 01/11] ALSA: pcm: introduce an alias of prototype to copy PCM frames

[Takashi Sakamoto]

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);
-- 
2.11.0

[PATCH RFC 02/11] ALSA: pcm: use new function alias instead of local one

[Takashi Sakamoto]

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);
 
 /*
-- 
2.11.0

[PATCH RFC 03/11] ALSA: pcm: refactoring silence operation

[Takashi Sakamoto]

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;
-- 
2.11.0

[PATCH RFC 04/11] ALSA: pcm: add alternative calls

[Takashi Sakamoto]

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);
-- 
2.11.0

[PATCH RFC 05/11] ALSA: core: remove duplicated codes to handle PCM frames

[Takashi Sakamoto]

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;
 }
  
-- 
2.11.0

[PATCH RFC 06/11] ALSA: pcm: add new operation; copy_frames

[Takashi Sakamoto]

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;
-- 
2.11.0

[PATCH RFC 07/11] ALSA: rme9652: an exsample changes for drivers which support interleaved buffer

[Takashi Sakamoto]

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)
-- 
2.11.0

[PATCH RFC 08/11] ALSA: gus: an examples change for drivers which support non-interleaved buffer

[Takashi Sakamoto]

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 = {
-- 
2.11.0

[PATCH RFC 09/11] ALSA: pcm: remove copy and silence callbacks

[Takashi Sakamoto]

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,
 };
 
-- 
2.11.0

[PATCH RFC 10/11] ALSA: pcm: add client_space parameter to runtime of PCM substream for PCM proxy drivers

[Takashi Sakamoto]

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,
-- 
2.11.0

[PATCH RFC 11/11] ALSA: pcm: add new configuration for PCM proxy drivers

[Takashi Sakamoto]

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:
-- 
2.11.0

Re: [PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[Takashi Iwai]

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
> 

Re: [PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[Takashi Iwai]

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

Re: [PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[Takashi Sakamoto]

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

Re: [PATCH RFC 00/11] ALSA: pcm: introduce copy_frame operation and SND_PCM_PROXY_DRIVER_SUPPORT

[Takashi Iwai]

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