Regardless of success/failure mapping of control/status data for runtime of PCM substream, appl_ptr/avail_min parameters are initialized. In current implementation, they are initialized in case-dependent, different places. It's possible to collect them to one place.

This commit unifies relevant code in a place after all of trials for the mappings are finished. --- src/pcm/pcm_hw.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index abf4afe0..c60a521b 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -867,10 +867,8 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u return xfern.result; }

-static int map_status_data(snd_pcm_t *pcm) +static int map_status_data(snd_pcm_hw_t *hw) { - snd_pcm_hw_t *hw = pcm->private_data; - struct snd_pcm_sync_ptr sync_ptr; void *ptr; int err; ptr = MAP_FAILED; @@ -879,15 +877,6 @@ static int map_status_data(snd_pcm_t *pcm) 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; @@ -897,13 +886,12 @@ static int map_status_data(snd_pcm_t *pcm) } else { hw->mmap_status = ptr; } - 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; }

-static int map_control_data(snd_pcm_t *pcm) +static int map_control_data(snd_pcm_hw_t *hw) { - snd_pcm_hw_t *hw = pcm->private_data; void *ptr; int err; if (hw->sync_ptr == NULL) { @@ -916,10 +904,8 @@ static int map_control_data(snd_pcm_t *pcm) return err; } hw->mmap_control = ptr; - } else { - hw->mmap_control->avail_min = 1; } - snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL); + return 0; }

@@ -930,14 +916,30 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)

hw->sync_ptr_ioctl = (int)force_fallback;

- err = map_status_data(pcm); + err = map_status_data(hw); if (err < 0) return err;

- err = map_control_data(pcm); + err = map_control_data(hw); if (err < 0) return err;

+ /* Initialize the data. */ + hw->mmap_control->appl_ptr = 0; + hw->mmap_control->avail_min = 1; + 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)); + snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd, + SNDRV_PCM_MMAP_OFFSET_CONTROL); + if (hw->sync_ptr_ioctl) { + if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, hw->sync_ptr) < 0) { + err = -errno; + SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err); + return err; + } + } + return 0; }

When allowing failure of map operation for both of status/control data for runtime of PCM substream, applications need to use fallback buffer for an alternative ioctl. However, in current implementation, status mapping is dominant to the allocation.

This commit moves code for the allocation outside of the mapping operation for status data. --- src/pcm/pcm_hw.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 1d34956c..a3d1f137 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -867,21 +867,18 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u return xfern.result; }

-static int map_status_data(snd_pcm_hw_t *hw) +static int map_status_data(snd_pcm_hw_t *hw, 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) { - 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->mmap_status = &sync_ptr->s.status; + hw->mmap_control = &sync_ptr->c.control; hw->sync_ptr_ioctl = 1; } else { hw->mmap_status = ptr; @@ -894,7 +891,7 @@ static int map_control_data(snd_pcm_hw_t *hw) { void *ptr; int err; - if (hw->sync_ptr == NULL) { + if (hw->sync_ptr_ioctl == 0) { ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_control)), PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED, hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL); @@ -912,11 +909,18 @@ static int map_control_data(snd_pcm_hw_t *hw) static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback) { snd_pcm_hw_t *hw = pcm->private_data; + struct snd_pcm_sync_ptr *sync_ptr; int err;

+ /* Preparation for fallback to failure of mmap(2). */ + sync_ptr = malloc(sizeof(*sync_ptr)); + if (sync_ptr == NULL) + return -ENOMEM; + memset(sync_ptr, 0, sizeof(*sync_ptr)); + hw->sync_ptr_ioctl = (int)force_fallback;

- err = map_status_data(hw); + err = map_status_data(hw, sync_ptr); if (err < 0) return err;

@@ -924,6 +928,14 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback) if (err < 0) return err;

+ /* Any fallback mode needs to keep the buffer. */ + if (hw->sync_ptr_ioctl == 0) { + hw->sync_ptr = sync_ptr; + } else { + free(sync_ptr); + hw->sync_ptr = NULL; + } + /* Initialize the data. */ hw->mmap_control->appl_ptr = 0; hw->mmap_control->avail_min = 1;

Currently, failures of status/control data mapping are handled dependently. However, it's not sure that one of the operations is failed when another is failed.

This commit adds a member into private data structure to maintain fallback mode for control data mapping, independently of status data mapping. As a result, we have four cases to handle status/control data:

1. both of status/control data are mapped. Nothing changed. A structure with alias of 'snd_pcm_hw_t' already has two members to point the mapped area and in application runtime they're used to refer/set status/control data. No need to call ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR to issue/query the data.

2. both of status/control data are unmapped. The two members point to allocated memory for fallback buffer. In application runtime, the buffer is given as an argument for ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR to issue/query the data.

3. status data is mapped only. One of the two members is used to point the mapped area. Another points to allocated memory for fallback buffer. In application runtime, the buffer is used as an argument to execute ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR for the latter data, but the former data is already synchronized.

4. control data is mapped only. The same as the above.

In design of ALSA PCM interface, userspace applications are not expected to map the status data as writable. On the other hand, expected to map the control data as writable. In a focus on the differences, we could achieve to reduce calls of the ioctl(2) in a case that one of the status/control data is successfully mapped and another is failed (case 3 and 4). Especially, in current alsa-lib implementation, application runtime queries state of runtime of PCM substream so often. --- src/pcm/pcm_hw.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 78857941..5573fce2 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -95,6 +95,7 @@ typedef struct { volatile struct snd_pcm_mmap_status * mmap_status; struct snd_pcm_mmap_control *mmap_control; bool mmap_status_fallbacked; + bool mmap_control_fallbacked; struct snd_pcm_sync_ptr *sync_ptr;

int period_event; @@ -143,9 +144,11 @@ static int sync_ptr1(snd_pcm_hw_t *hw, unsigned int flags) return 0; }

-static inline int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) +static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) { - return hw->sync_ptr ? sync_ptr1(hw, flags) : 0; + if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked) + return sync_ptr1(hw, flags); + return 0; }

static int snd_pcm_hw_clear_timer_queue(snd_pcm_hw_t *hw) @@ -899,24 +902,24 @@ static bool map_control_data(snd_pcm_hw_t *hw, bool force_fallback) { struct snd_pcm_mmap_control *mmap_control; - int err; + bool fallbacked;

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 (mmap_control == MAP_FAILED || mmap_control == NULL) { - err = -errno; - SYSMSG("control mmap failed (%i)", err); - return err; - } - } else { + } + + if (mmap_control == MAP_FAILED || mmap_control == NULL) { mmap_control = &sync_ptr->c.control; + fallbacked = true; + } else { + fallbacked = false; }

hw->mmap_control = mmap_control;

- return 0; + return fallbacked; }

static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback) @@ -933,12 +936,11 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)

hw->mmap_status_fallbacked = map_status_data(hw, sync_ptr, force_fallback); - err = map_control_data(hw, sync_ptr, hw->mmap_status_fallbacked); - if (err < 0) - return err; + hw->mmap_control_fallbacked = + map_control_data(hw, sync_ptr, force_fallback);

/* Any fallback mode needs to keep the buffer. */ - if (hw->mmap_status_fallbacked == 0) { + if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked) { hw->sync_ptr = sync_ptr; } else { free(sync_ptr); @@ -953,7 +955,7 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback) offsetof(struct snd_pcm_mmap_status, hw_ptr)); snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL); - if (hw->mmap_status_fallbacked) { + if (hw->mmap_control_fallbacked) { if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, hw->sync_ptr) < 0) { err = -errno; SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err); @@ -975,7 +977,7 @@ static void unmap_status_data(snd_pcm_hw_t *hw)

static void unmap_control_data(snd_pcm_hw_t *hw) { - if (!hw->mmap_status_fallbacked) { + if (!hw->mmap_control_fallbacked) { if (munmap((void *)hw->mmap_control, page_align(sizeof(*hw->mmap_control))) < 0) SYSMSG("control munmap failed (%u)", errno); @@ -987,12 +989,13 @@ static void unmap_status_and_control_data(snd_pcm_hw_t *hw) unmap_status_data(hw); unmap_control_data(hw);

- if (hw->mmap_status_fallbacked) + if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked) free(hw->sync_ptr);

hw->mmap_status = NULL; hw->mmap_control = NULL; hw->mmap_status_fallbacked = false; + hw->mmap_control_fallbacked = false; hw->sync_ptr = NULL; }