[alsa-lib][RFC][PATCH 0/9] pcm: handle status/control mapping independently

[alsa-lib][RFC][PATCH 0/9] pcm: handle status/control mapping independently

[Takashi Sakamoto]

Hi,

This RFC patchset is for recent discussion about new version of PCM
protocol/interface (2.0.14). This is accumulated on the below patchset.
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122066.html

In the discussion, we found an advantage to handle page mapping for
status/control data of runtime of PCM substream independently. Aim of
this patchset is a proof-of-concept for alsa-lib.

I tested this patchset with two machines based on x86/armv7a architectures.
This looks work well without any regression. But I've never tested a case
in which mapping is failed for one of the status/control data.

For your information.

Takashi Sakamoto (9):
  pcm: obsolete 'mmap_emulation' parameter of snd_pcm_hw_open_fd()
  pcm: minor code refactoring for ioctl call
  pcm: handle status/control mapping independently
  pcm: add a helper function to query status of PCM substream
  pcm: add a helper function to query hwptr
  pcm: add a helper function to query applptr
  pcm: add a helper function to issue avail_min
  pcm: add a helper function to issue applptr
  pcm: code refactoring to use helper functions

 src/pcm/pcm_direct.c |   2 +-
 src/pcm/pcm_hw.c     | 324 +++++++++++++++++++++++++++++++++------------------
 src/pcm/pcm_local.h  |   3 +-
 3 files changed, 214 insertions(+), 115 deletions(-)

-- 
2.11.0

[alsa-lib][RFC][PATCH 1/9] pcm: obsolete 'mmap_emulation' parameter of snd_pcm_hw_open_fd()

[Takashi Sakamoto]

A function, snd_pcm_hw_open_fd(), is just for internal use. This function
has an obsoleted parameter and we can remove it without any compatibility
issue.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_direct.c | 2 +-
 src/pcm/pcm_hw.c     | 6 ++----
 src/pcm/pcm_local.h  | 3 ++-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 43702601..9fd376d8 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -1523,7 +1523,7 @@ int snd_pcm_direct_open_secondary_client(snd_pcm_t **spcmp, snd_pcm_direct_t *dm
 	int ret;
 	snd_pcm_t *spcm;
 
-	ret = snd_pcm_hw_open_fd(spcmp, client_name, dmix->hw_fd, 0, 0);
+	ret = snd_pcm_hw_open_fd(spcmp, client_name, dmix->hw_fd, 0);
 	if (ret < 0) {
 		SNDERR("unable to open hardware");
 		return ret;
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 66658599..f74bd0d7 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -1417,15 +1417,13 @@ static const snd_pcm_fast_ops_t snd_pcm_hw_fast_ops_timer = {
  * \param pcmp Returns created PCM handle
  * \param name Name of PCM
  * \param fd File descriptor
- * \param mmap_emulation Obsoleted parameter
  * \param sync_ptr_ioctl Boolean flag for sync_ptr ioctl
  * \retval zero on success otherwise a negative error code
  * \warning Using of this function might be dangerous in the sense
  *          of compatibility reasons. The prototype might be freely
  *          changed in future.
  */
-int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
-		       int fd, int mmap_emulation ATTRIBUTE_UNUSED,
+int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd,
 		       int sync_ptr_ioctl)
 {
 	int ver, mode;
@@ -1632,7 +1630,7 @@ int snd_pcm_hw_open(snd_pcm_t **pcmp, const char *name,
 		}
 	}
 	snd_ctl_close(ctl);
-	return snd_pcm_hw_open_fd(pcmp, name, fd, 0, sync_ptr_ioctl);
+	return snd_pcm_hw_open_fd(pcmp, name, fd, sync_ptr_ioctl);
        _err:
 	snd_ctl_close(ctl);
 	return ret;
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index e4f65218..7600daa3 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -935,7 +935,8 @@ snd_pcm_open_slave(snd_pcm_t **pcmp, snd_config_t *root,
 
 #define snd_pcm_conf_generic_id(id) _snd_conf_generic_id(id)
 
-int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd, int mmap_emulation, int sync_ptr_ioctl);
+int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd,
+		       int sync_ptr_ioctl);
 int __snd_pcm_mmap_emul_open(snd_pcm_t **pcmp, const char *name,
 			     snd_pcm_t *slave, int close_slave);
 
-- 
2.11.0

[alsa-lib][RFC][PATCH 2/9] pcm: minor code refactoring for ioctl call

[Takashi Sakamoto]

When error occurs, return value from ioctl(2) is -1 and error code can
be got thread local variable, errno. It's OK just to check the return
value without any assignment.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index f74bd0d7..cf143f2f 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -135,12 +135,12 @@ static int sync_ptr1(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *ptr,
 	int err;
 
 	ptr->flags = flags;
-	err = ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, ptr);
-	if (err < 0) {
+	if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, ptr) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
 		return err;
 	}
+
 	return 0;
 }
 
-- 
2.11.0

[alsa-lib][RFC][PATCH 3/9] pcm: handle status/control mapping independently

[Takashi Sakamoto]

In ALSA PCM interface, status/control data of PCM substream can be mapped
into process' VMA of application. This has an advantage to share
information of the PCM substream. This operation has limitation depending
on running architecture and handled device. In this case, for convenience
to applications, SNDRV_PCM_IOCTL_SYNC_PTR command is available as a
fallback.

In current implementation, mapping operation for status/control data of
PCM substream depends each other. If the operation for one side results
in failure, operation for the another side is cancelled, thus no mapping
is available. However, mapping for one side has advantages because nature
of these two types of data is essentially different.

This commit handles these two mapping operations independently.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 201 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 126 insertions(+), 75 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index cf143f2f..7da2db5b 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -90,11 +90,14 @@ typedef struct {
 	int version;
 	int fd;
 	int card, device, subdevice;
-	int sync_ptr_ioctl;
-	int sync_applptr;
-	volatile struct snd_pcm_mmap_status * mmap_status;
-	struct snd_pcm_mmap_control *mmap_control;
+
 	struct snd_pcm_sync_ptr *sync_ptr;
+	volatile struct snd_pcm_mmap_status *mmap_status;
+	struct snd_pcm_mmap_control *mmap_control;
+	int mmap_status_fallbacked;
+	int mmap_control_fallbacked;
+	int sync_applptr;
+
 	int period_event;
 	snd_timer_t *period_timer;
 	struct pollfd period_timer_pfd;
@@ -878,96 +881,149 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
 	return xfern.result;
 }
 
-static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
+static int map_status(snd_pcm_t *pcm, struct snd_pcm_sync_ptr *sync_ptr,
+		      int force_fallback)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
-	struct snd_pcm_sync_ptr sync_ptr;
-	void *ptr;
-	int err;
-	ptr = MAP_FAILED;
-	if (hw->sync_ptr_ioctl == 0)
-		ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_status)),
-			   PROT_READ, MAP_FILE|MAP_SHARED, 
-			   hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
-	if (ptr == MAP_FAILED || ptr == NULL) {
-		memset(&sync_ptr, 0, sizeof(sync_ptr));
-		sync_ptr.c.control.appl_ptr = 0;
-		sync_ptr.c.control.avail_min = 1;
-		err = ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, &sync_ptr);
-		if (err < 0) {
-			err = -errno;
-			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
-			return err;
-		}
-		hw->sync_ptr = calloc(1, sizeof(struct snd_pcm_sync_ptr));
-		if (hw->sync_ptr == NULL)
-			return -ENOMEM;
-		hw->mmap_status = &hw->sync_ptr->s.status;
-		hw->mmap_control = &hw->sync_ptr->c.control;
-		hw->sync_ptr_ioctl = 1;
+	struct snd_pcm_mmap_status *mmap_status;
+	int fallbacked;
+
+	mmap_status = MAP_FAILED;
+	if (!force_fallback) {
+		mmap_status = mmap(NULL, page_align(sizeof(*mmap_status)),
+				   PROT_READ, MAP_FILE|MAP_SHARED,
+				   hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
+	}
+
+	/* Switch to fallback mode. */
+	if (mmap_status == MAP_FAILED || mmap_status == NULL) {
+		mmap_status = &sync_ptr->s.status;
+		fallbacked = 1;
 	} else {
-		hw->mmap_status = ptr;
+		fallbacked = 0;
 	}
-	snd_pcm_set_hw_ptr(pcm, &hw->mmap_status->hw_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS + offsetof(struct snd_pcm_mmap_status, hw_ptr));
-	return 0;
+
+	/* Initialization. */
+	snd_pcm_set_hw_ptr(pcm, &mmap_status->hw_ptr, hw->fd,
+			   SNDRV_PCM_MMAP_OFFSET_STATUS +
+			   offsetof(struct snd_pcm_mmap_status, hw_ptr));
+
+	hw->mmap_status = mmap_status;
+	return fallbacked;
 }
 
-static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
+static int map_control(snd_pcm_t *pcm, struct snd_pcm_sync_ptr *sync_ptr,
+		       int force_fallback)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
-	void *ptr;
-	int err;
-	if (hw->sync_ptr == NULL) {
-		ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_control)),
-			   PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED, 
+	struct snd_pcm_mmap_control *mmap_control;
+	int fallbacked;
+
+	mmap_control = MAP_FAILED;
+	if (!force_fallback) {
+		mmap_control = mmap(NULL, page_align(sizeof(*mmap_control)),
+			   PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED,
 			   hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
-		if (ptr == MAP_FAILED || ptr == NULL) {
-			err = -errno;
-			SYSMSG("control mmap failed (%i)", err);
-			return err;
-		}
-		hw->mmap_control = ptr;
+	}
+
+	/* Switch to fallback mode. */
+	if (mmap_control == MAP_FAILED || mmap_control == NULL) {
+		mmap_control = &sync_ptr->c.control;
+		fallbacked = 1;
 	} else {
-		hw->mmap_control->avail_min = 1;
+		fallbacked = 0;
 	}
-	snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
-	return 0;
+
+	/* Initialization. */
+	mmap_control->appl_ptr = 0;
+	mmap_control->avail_min = 1;
+	snd_pcm_set_appl_ptr(pcm, &mmap_control->appl_ptr, hw->fd,
+			     SNDRV_PCM_MMAP_OFFSET_CONTROL);
+
+	hw->mmap_control = mmap_control;
+	return fallbacked;
 }
 
-static int snd_pcm_hw_munmap_status(snd_pcm_t *pcm)
+static int map_status_and_control_data(snd_pcm_t *pcm, int force_fallback)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
-	int err;
-	if (hw->sync_ptr_ioctl) {
-		free(hw->sync_ptr);
-		hw->sync_ptr = NULL;
+	struct snd_pcm_sync_ptr *sync_ptr;
+
+	/* Preparation for fallback from failure of mmap(2). */
+	sync_ptr = malloc(sizeof(*sync_ptr));
+	if (sync_ptr == NULL)
+		return -ENOMEM;
+	memset(sync_ptr, 0, sizeof(*sync_ptr));
+
+	hw->mmap_status_fallbacked = map_status(pcm, sync_ptr, force_fallback);
+	hw->mmap_control_fallbacked = map_control(pcm, sync_ptr, force_fallback);
+
+	/* Any fallback mode needs to keep the buffer. */
+	if (hw->mmap_status_fallbacked > 0 || hw->mmap_control_fallbacked > 0) {
+		hw->sync_ptr = sync_ptr;
 	} else {
-		if (munmap((void*)hw->mmap_status, page_align(sizeof(*hw->mmap_status))) < 0) {
-			err = -errno;
-			SYSMSG("status munmap failed (%i)", err);
-			return err;
+		free(sync_ptr);
+		hw->sync_ptr = NULL;
+	}
+
+	/* Initialize when control mapping is fallbacked. */
+	if (hw->mmap_control_fallbacked > 0) {
+		int err = ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, &hw->sync_ptr);
+		if (err < 0) {
+			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", -errno);
+			return -errno;
 		}
 	}
+
 	return 0;
 }
 
-static int snd_pcm_hw_munmap_control(snd_pcm_t *pcm)
+static int unmap_status(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
-	int err;
-	if (hw->sync_ptr_ioctl) {
-		free(hw->sync_ptr);
-		hw->sync_ptr = NULL;
-	} else {
-		if (munmap(hw->mmap_control, page_align(sizeof(*hw->mmap_control))) < 0) {
-			err = -errno;
-			SYSMSG("control munmap failed (%i)", err);
-			return err;
+
+	if (!hw->mmap_status_fallbacked) {
+		if (munmap((void*)hw->mmap_status,
+			   page_align(sizeof(*hw->mmap_status))) < 0) {
+			SYSMSG("status munmap failed (%i)", -errno);
+			return -errno;
 		}
 	}
+
+	hw->mmap_status = NULL;
+	hw->mmap_status_fallbacked = 0;
+
 	return 0;
 }
 
+static int unmap_control(snd_pcm_t *pcm)
+{
+	snd_pcm_hw_t *hw = pcm->private_data;
+
+	if (!hw->mmap_control_fallbacked) {
+		if (munmap((void *)hw->mmap_control,
+			   page_align(sizeof(*hw->mmap_control))) < 0) {
+			SYSMSG("control munmap failed (%i)", -errno);
+			return -errno;
+		}
+	}
+
+	hw->mmap_control = NULL;
+	hw->mmap_control_fallbacked = 0;
+
+	return 0;
+}
+
+static void unmap_status_and_control_data(snd_pcm_t *pcm)
+{
+	snd_pcm_hw_t *hw = pcm->private_data;
+
+	unmap_status(pcm);
+	unmap_control(pcm);
+	if (hw->sync_ptr)
+		free(hw->sync_ptr);
+}
+
 static int snd_pcm_hw_mmap(snd_pcm_t *pcm ATTRIBUTE_UNUSED)
 {
 	return 0;
@@ -986,8 +1042,9 @@ static int snd_pcm_hw_close(snd_pcm_t *pcm)
 		err = -errno;
 		SYSMSG("close failed (%i)\n", err);
 	}
-	snd_pcm_hw_munmap_status(pcm);
-	snd_pcm_hw_munmap_control(pcm);
+
+	unmap_status_and_control_data(pcm);
+
 	free(hw);
 	return err;
 }
@@ -1509,7 +1566,6 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd,
 	hw->device = info.device;
 	hw->subdevice = info.subdevice;
 	hw->fd = fd;
-	hw->sync_ptr_ioctl = sync_ptr_ioctl;
 	/* no restriction */
 	hw->format = SND_PCM_FORMAT_UNKNOWN;
 	hw->rate = 0;
@@ -1533,12 +1589,7 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd,
 #endif
 	pcm->own_state_check = 1; /* skip the common state check */
 
-	ret = snd_pcm_hw_mmap_status(pcm);
-	if (ret < 0) {
-		snd_pcm_close(pcm);
-		return ret;
-	}
-	ret = snd_pcm_hw_mmap_control(pcm);
+	ret = map_status_and_control_data(pcm, sync_ptr_ioctl);
 	if (ret < 0) {
 		snd_pcm_close(pcm);
 		return ret;
-- 
2.11.0

[alsa-lib][RFC][PATCH 4/9] pcm: add a helper function to query status of PCM substream

[Takashi Sakamoto]

When applications requests queueing/dequeueing some PCM frames by PCM
read/write operations, ALSA PCM core handles the request and moves
application-side position on PCM buffer. Usually, this can be visible
from user space because page frame for the position can be mapped into
process' VMA.

However, when mapping operation results in failure, applications need
to execute ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR to get current position.
This is important task for the applications to maintain runtime of PCM
substream.

This commit adds a helper function to unify relevant the codes for
readability. Original implementation has a side effect to issue the
size of avail_min. This commit add a flag to purge the effect.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 7da2db5b..23211cba 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -160,6 +160,16 @@ static int sync_applptr(snd_pcm_hw_t *hw)
 	return sync_ptr1(hw, &ptr, 0);
 }
 
+static int query_status(snd_pcm_hw_t *hw)
+{
+	if (!hw->mmap_status_fallbacked)
+		return 0;
+
+	return sync_ptr1(hw, hw->sync_ptr,
+			 SNDRV_PCM_SYNC_PTR_APPL |
+			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+}
+
 static int snd_pcm_hw_clear_timer_queue(snd_pcm_hw_t *hw)
 {
 	if (hw->period_timer_need_poll) {
@@ -814,8 +824,10 @@ static snd_pcm_sframes_t snd_pcm_hw_writei(snd_pcm_t *pcm, const void *buffer, s
 	xferi.buf = (char*) buffer;
 	xferi.frames = size;
 	xferi.result = 0; /* make valgrind happy */
-	err = ioctl(fd, SNDRV_PCM_IOCTL_WRITEI_FRAMES, &xferi);
-	err = err >= 0 ? sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL) : -errno;
+	if (ioctl(fd, SNDRV_PCM_IOCTL_WRITEI_FRAMES, &xferi) < 0)
+		err = -errno;
+	else
+		err = query_status(hw);
 #ifdef DEBUG_RW
 	fprintf(stderr, "hw_writei: frames = %li, xferi.result = %li, err = %i\n", size, xferi.result, err);
 #endif
@@ -833,8 +845,10 @@ static snd_pcm_sframes_t snd_pcm_hw_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_
 	memset(&xfern, 0, sizeof(xfern)); /* make valgrind happy */
 	xfern.bufs = bufs;
 	xfern.frames = size;
-	err = ioctl(fd, SNDRV_PCM_IOCTL_WRITEN_FRAMES, &xfern);
-	err = err >= 0 ? sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL) : -errno;
+	if (ioctl(fd, SNDRV_PCM_IOCTL_WRITEN_FRAMES, &xfern) < 0)
+		err = -errno;
+	else
+		err = query_status(hw);
 #ifdef DEBUG_RW
 	fprintf(stderr, "hw_writen: frames = %li, result = %li, err = %i\n", size, xfern.result, err);
 #endif
@@ -852,8 +866,10 @@ static snd_pcm_sframes_t snd_pcm_hw_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_
 	xferi.buf = buffer;
 	xferi.frames = size;
 	xferi.result = 0; /* make valgrind happy */
-	err = ioctl(fd, SNDRV_PCM_IOCTL_READI_FRAMES, &xferi);
-	err = err >= 0 ? sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL) : -errno;
+	if (ioctl(fd, SNDRV_PCM_IOCTL_READI_FRAMES, &xferi) < 0)
+		err = -errno;
+	else
+		err = query_status(hw);
 #ifdef DEBUG_RW
 	fprintf(stderr, "hw_readi: frames = %li, result = %li, err = %i\n", size, xferi.result, err);
 #endif
@@ -871,8 +887,10 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
 	memset(&xfern, 0, sizeof(xfern)); /* make valgrind happy */
 	xfern.bufs = bufs;
 	xfern.frames = size;
-	err = ioctl(fd, SNDRV_PCM_IOCTL_READN_FRAMES, &xfern);
-	err = err >= 0 ? sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL) : -errno;
+	if (ioctl(fd, SNDRV_PCM_IOCTL_READN_FRAMES, &xfern) < 0)
+		err = -errno;
+	else
+		err = query_status(hw);
 #ifdef DEBUG_RW
 	fprintf(stderr, "hw_readn: frames = %li, result = %li, err = %i\n", size, xfern.result, err);
 #endif
-- 
2.11.0

[alsa-lib][RFC][PATCH 5/9] pcm: add a helper function to query hwptr

[Takashi Sakamoto]

Usual PCM drivers in kernel land can respond applications' request about
recent hardware-side position of data transmission. In most cases, the
request is issued by ioctl(2) with SNDRV_PCM_IOCTL_HWSYNC, then the
position is shared via mapped page frame. However, when failing the
mapping, applications use SNDRV_PCM_IOCTL_SYNC_PTR command to do the
same thing as a fallback.

This commit adds a helper function to unify the relevant codes for
readability. Original implementation has a side effect to issue current
application-side position and the size of avail_min, thus has a side
effect. This commit adds some flags to purge the effect.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 23211cba..e3a4ee7c 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -160,6 +160,17 @@ static int sync_applptr(snd_pcm_hw_t *hw)
 	return sync_ptr1(hw, &ptr, 0);
 }
 
+static int query_hwptr(snd_pcm_hw_t *hw)
+{
+	if (!hw->mmap_status_fallbacked)
+		return 0;
+
+	return sync_ptr1(hw, hw->sync_ptr,
+			 SNDRV_PCM_SYNC_PTR_HWSYNC |
+			 SNDRV_PCM_SYNC_PTR_APPL |
+			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+}
+
 static int query_status(snd_pcm_hw_t *hw)
 {
 	if (!hw->mmap_status_fallbacked)
@@ -585,8 +596,8 @@ static int snd_pcm_hw_hwsync(snd_pcm_t *pcm)
 	snd_pcm_hw_t *hw = pcm->private_data;
 	int fd = hw->fd, err;
 	if (SNDRV_PROTOCOL_VERSION(2, 0, 3) <= hw->version) {
-		if (hw->sync_ptr) {
-			err = sync_ptr1(hw, hw->sync_ptr, SNDRV_PCM_SYNC_PTR_HWSYNC);
+		if (hw->mmap_status_fallbacked) {
+			err = query_hwptr(hw);
 			if (err < 0)
 				return err;
 		} else {
@@ -735,7 +746,7 @@ static snd_pcm_sframes_t snd_pcm_hw_forward(snd_pcm_t *pcm, snd_pcm_uframes_t fr
 	} else {
 		snd_pcm_sframes_t avail;
 
-		err = sync_ptr(hw, SNDRV_PCM_SYNC_PTR_HWSYNC);
+		err = query_hwptr(hw);
 		if (err < 0)
 			return err;
 		switch (FAST_PCM_STATE(hw)) {
-- 
2.11.0

[alsa-lib][RFC][PATCH 6/9] pcm: add a helper function to query applptr

[Takashi Sakamoto]

In some operations, application-side position for data transmission is
moved by ALSA PCM core. In this case, applications need to query the
position. Usually, this is done via mapped page frame. However, in
some cases, this is done by ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR.

This commit adds a helper function to unify the relevant codes for
readability. Additionally, to purge side effects, this commit adds
a flag.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index e3a4ee7c..a5cc5e46 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -160,6 +160,16 @@ static int sync_applptr(snd_pcm_hw_t *hw)
 	return sync_ptr1(hw, &ptr, 0);
 }
 
+static int query_applptr(snd_pcm_hw_t *hw)
+{
+	if (!hw->mmap_control_fallbacked)
+		return 0;
+
+	return sync_ptr1(hw, hw->sync_ptr,
+			 SNDRV_PCM_SYNC_PTR_APPL |
+			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+}
+
 static int query_hwptr(snd_pcm_hw_t *hw)
 {
 	if (!hw->mmap_status_fallbacked)
@@ -644,7 +654,7 @@ static int snd_pcm_hw_reset(snd_pcm_t *pcm)
 		SYSMSG("SNDRV_PCM_IOCTL_RESET failed (%i)", err);
 		return err;
 	}
-	return sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL);
+	return query_applptr(hw);
 }
 
 static int snd_pcm_hw_start(snd_pcm_t *pcm)
@@ -718,7 +728,7 @@ static snd_pcm_sframes_t snd_pcm_hw_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t fra
 		SYSMSG("SNDRV_PCM_IOCTL_REWIND failed (%i)", err);
 		return err;
 	}
-	err = sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL);
+	err = query_applptr(hw);
 	if (err < 0)
 		return err;
 	return frames;
@@ -739,7 +749,7 @@ static snd_pcm_sframes_t snd_pcm_hw_forward(snd_pcm_t *pcm, snd_pcm_uframes_t fr
 			SYSMSG("SNDRV_PCM_IOCTL_FORWARD failed (%i)", err);
 			return err;
 		}
-		err = sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL);
+		err = query_applptr(hw);
 		if (err < 0)
 			return err;
 		return frames;
@@ -766,7 +776,7 @@ static snd_pcm_sframes_t snd_pcm_hw_forward(snd_pcm_t *pcm, snd_pcm_uframes_t fr
 		if (frames > (snd_pcm_uframes_t)avail)
 			frames = avail;
 		snd_pcm_mmap_appl_forward(pcm, frames);
-		err = sync_ptr(hw, 0);
+		err = issue_applptr(hw);
 		if (err < 0)
 			return err;
 		return frames;
-- 
2.11.0

[alsa-lib][RFC][PATCH 7/9] pcm: add a helper function to issue avail_min

[Takashi Sakamoto]

Runtime of PCM substream has a parameter, avail_min, as a threshold to
the amount of available PCM frames in PCM buffer. Usually, this value
is shared with user space via mapped page frame. However, when failing
the mapping, applications perform fallback operation with
SNDRV_PCM_IOCTL_SYNC_PTR.

This commit add a helper function so that the above design is explicit.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index a5cc5e46..045e7890 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -160,6 +160,15 @@ static int sync_applptr(snd_pcm_hw_t *hw)
 	return sync_ptr1(hw, &ptr, 0);
 }
 
+static int issue_availmin(snd_pcm_hw_t *hw)
+{
+	if (!hw->mmap_control_fallbacked)
+		return 0;
+
+	return sync_ptr1(hw, hw->sync_ptr,
+			 SNDRV_PCM_SYNC_PTR_APPL);
+}
+
 static int query_applptr(snd_pcm_hw_t *hw)
 {
 	if (!hw->mmap_control_fallbacked)
@@ -495,7 +504,7 @@ static int snd_pcm_hw_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t * params)
 	    params->silence_size == pcm->silence_size &&
 	    old_period_event == hw->period_event) {
 		hw->mmap_control->avail_min = params->avail_min;
-		return sync_ptr(hw, 0);
+		return issue_availmin(hw);
 	}
 	if (params->tstamp_type == SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW &&
 	    hw->version < SNDRV_PROTOCOL_VERSION(2, 0, 12)) {
-- 
2.11.0

[alsa-lib][RFC][PATCH 8/9] pcm: add a helper function to issue applptr

[Takashi Sakamoto]

When handling PCM frames by mmap operation for PCM buffer, in usual
cases, applications have no need to issue application-side position
into stuffs of kernel land. In the cases, specific page frame is
mapped for the position. However, in a case to fail the mapping,
applications can use a fallback method. This is done by ioctl(2)
with SNDRV_PCM_IOCTL_SYNC_PTR.

This commit adds a helper function to unify the relevant code.
Additionally, this commit adds a flag to purge side effect of the
call.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 045e7890..2d68fe12 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -160,6 +160,15 @@ static int sync_applptr(snd_pcm_hw_t *hw)
 	return sync_ptr1(hw, &ptr, 0);
 }
 
+static int issue_applptr(snd_pcm_hw_t *hw)
+{
+	if (!hw->mmap_control_fallbacked)
+		return 0;
+
+	return sync_ptr1(hw, hw->sync_ptr,
+			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+}
+
 static int issue_availmin(snd_pcm_hw_t *hw)
 {
 	if (!hw->mmap_control_fallbacked)
@@ -592,7 +601,7 @@ static int snd_pcm_hw_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 static snd_pcm_state_t snd_pcm_hw_state(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
-	int err = sync_ptr(hw, 0);
+	int err = issue_applptr(hw);
 	if (err < 0)
 		return err;
 	return (snd_pcm_state_t) hw->mmap_status->state;
@@ -1016,7 +1025,7 @@ static int map_status_and_control_data(snd_pcm_t *pcm, int force_fallback)
 
 	/* Initialize when control mapping is fallbacked. */
 	if (hw->mmap_control_fallbacked > 0) {
-		int err = ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, &hw->sync_ptr);
+		int err = issue_applptr(hw);
 		if (err < 0) {
 			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", -errno);
 			return -errno;
@@ -1119,7 +1128,7 @@ static snd_pcm_sframes_t snd_pcm_hw_avail_update(snd_pcm_t *pcm)
 	snd_pcm_hw_t *hw = pcm->private_data;
 	snd_pcm_uframes_t avail;
 
-	sync_ptr(hw, 0);
+	issue_applptr(hw);
 	avail = snd_pcm_mmap_avail(pcm);
 	switch (FAST_PCM_STATE(hw)) {
 	case SNDRV_PCM_STATE_RUNNING:
-- 
2.11.0

[alsa-lib][RFC][PATCH 9/9] pcm: code refactoring to use helper functions

[Takashi Sakamoto]

In former commits, some helper functions were added. This commit applies
refactoring to use them.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 64 +++++++++++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 2d68fe12..4759b5bb 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -132,13 +132,11 @@ struct timespec snd_pcm_hw_fast_tstamp(snd_pcm_t *pcm)
 }
 #endif /* DOC_HIDDEN */
 
-static int sync_ptr1(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *ptr,
-		     unsigned int flags)
+static int do_sync_ptr(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *sync_ptr)
 {
 	int err;
 
-	ptr->flags = flags;
-	if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, ptr) < 0) {
+	if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, sync_ptr) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
 		return err;
@@ -147,26 +145,23 @@ static int sync_ptr1(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *ptr,
 	return 0;
 }
 
-static inline int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags)
+static int issue_applptr(snd_pcm_hw_t *hw)
 {
-	return hw->sync_ptr ? sync_ptr1(hw, hw->sync_ptr, flags) : 0;
-}
+	struct snd_pcm_sync_ptr *sync_ptr, buf = {0};
 
-/* explicit notification of appl_ptr update to kernel */
-static int sync_applptr(snd_pcm_hw_t *hw)
-{
-	struct snd_pcm_sync_ptr ptr;
-	ptr.c.control = *hw->mmap_control;
-	return sync_ptr1(hw, &ptr, 0);
-}
+	if (!hw->mmap_control_fallbacked) {
+		if (!hw->sync_applptr)
+			return 0;
 
-static int issue_applptr(snd_pcm_hw_t *hw)
-{
-	if (!hw->mmap_control_fallbacked)
-		return 0;
 
-	return sync_ptr1(hw, hw->sync_ptr,
-			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+		buf.c.control = *hw->mmap_control;
+		sync_ptr = &buf;
+	} else {
+		sync_ptr = hw->sync_ptr;
+	}
+
+	sync_ptr->flags = SNDRV_PCM_SYNC_PTR_AVAIL_MIN;
+	return do_sync_ptr(hw, sync_ptr);
 }
 
 static int issue_availmin(snd_pcm_hw_t *hw)
@@ -174,8 +169,8 @@ static int issue_availmin(snd_pcm_hw_t *hw)
 	if (!hw->mmap_control_fallbacked)
 		return 0;
 
-	return sync_ptr1(hw, hw->sync_ptr,
-			 SNDRV_PCM_SYNC_PTR_APPL);
+	hw->sync_ptr->flags = SNDRV_PCM_SYNC_PTR_APPL;
+	return do_sync_ptr(hw, hw->sync_ptr);
 }
 
 static int query_applptr(snd_pcm_hw_t *hw)
@@ -183,9 +178,9 @@ static int query_applptr(snd_pcm_hw_t *hw)
 	if (!hw->mmap_control_fallbacked)
 		return 0;
 
-	return sync_ptr1(hw, hw->sync_ptr,
-			 SNDRV_PCM_SYNC_PTR_APPL |
-			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+	hw->sync_ptr->flags = SNDRV_PCM_SYNC_PTR_APPL |
+			      SNDRV_PCM_SYNC_PTR_AVAIL_MIN;
+	return do_sync_ptr(hw, hw->sync_ptr);
 }
 
 static int query_hwptr(snd_pcm_hw_t *hw)
@@ -193,10 +188,10 @@ static int query_hwptr(snd_pcm_hw_t *hw)
 	if (!hw->mmap_status_fallbacked)
 		return 0;
 
-	return sync_ptr1(hw, hw->sync_ptr,
-			 SNDRV_PCM_SYNC_PTR_HWSYNC |
-			 SNDRV_PCM_SYNC_PTR_APPL |
-			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+	hw->sync_ptr->flags = SNDRV_PCM_SYNC_PTR_HWSYNC |
+			      SNDRV_PCM_SYNC_PTR_APPL |
+			      SNDRV_PCM_SYNC_PTR_AVAIL_MIN;
+	return do_sync_ptr(hw, hw->sync_ptr);
 }
 
 static int query_status(snd_pcm_hw_t *hw)
@@ -204,9 +199,9 @@ static int query_status(snd_pcm_hw_t *hw)
 	if (!hw->mmap_status_fallbacked)
 		return 0;
 
-	return sync_ptr1(hw, hw->sync_ptr,
-			 SNDRV_PCM_SYNC_PTR_APPL |
-			 SNDRV_PCM_SYNC_PTR_AVAIL_MIN);
+	hw->sync_ptr->flags = SNDRV_PCM_SYNC_PTR_APPL |
+			      SNDRV_PCM_SYNC_PTR_AVAIL_MIN;
+	return do_sync_ptr(hw, hw->sync_ptr);
 }
 
 static int snd_pcm_hw_clear_timer_queue(snd_pcm_hw_t *hw)
@@ -1113,10 +1108,7 @@ static snd_pcm_sframes_t snd_pcm_hw_mmap_commit(snd_pcm_t *pcm,
 	snd_pcm_hw_t *hw = pcm->private_data;
 
 	snd_pcm_mmap_appl_forward(pcm, size);
-	if (hw->sync_ptr)
-		sync_ptr(hw, 0);
-	else if (hw->sync_applptr)
-		sync_applptr(hw);
+	issue_applptr(hw);
 #ifdef DEBUG_MMAP
 	fprintf(stderr, "appl_forward: hw_ptr = %li, appl_ptr = %li, size = %li\n", *pcm->hw.ptr, *pcm->appl.ptr, size);
 #endif
-- 
2.11.0