[alsa-devel] [PATCH alsa-lib 00/12] pcm: hw: optimization for v2.0.14 of PCM protocol/interface
Hi,
This patchset is an optimization for version 2.0.14 of ALSA PCM protocol/interface.
In this version, for some devices/drivers, applications are disallowed to map control data of runtime of PCM substream. On the other hand, mapping status data is still available.
In current implementation of alsa-lib, execution of ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR is still done without enough care of the change. For example, even if status data is successfully mapped, the ioctl is executed to query the status data. This is inefficient.
This patchset is to arrange the execution.
Takashi Sakamoto (12): pcm: hw: fix to initialize function local variable pcm: hw: add a helper function just to query status/control data of PCM substream pcm: hw: use heler function to query status/control data after reading/writing PCM frames pcm: hw: use helper function to query status/control data after HW_PARAMS call pcm: hw: use helper function to query status/control data after PREPARE/RESET call pcm: hw: use helper function to query status/control data after REWIND/FORWARD call pcm: hw: use helper function to query status/control data for calculation of available space on PCM buffer pcm: hw: add a helper function to request hwsync without side-effects pcm: hw: add a helper function to issue appl_ptr without sub-effects pcm: hw: add a helper function to issue avail_min without sub-effects pcm: hw: remove superfluous code to call of SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_hw_forward() pcm: hw: minor refactoring for initialization of control data
src/pcm/pcm_hw.c | 109 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 75 insertions(+), 34 deletions(-)
This commit is to fix below warning.
pcm_hw.c: In function ‘snd1_pcm_hw_open_fd’: pcm_hw.c:955:33: warning: ‘mmap_control’ may be used uninitialized in this function [-Wmaybe-uninitialized] if (mmap_control == MAP_FAILED || mmap_control == NULL) { ^ pcm_hw.c:946:31: note: ‘mmap_control’ was declared here struct snd_pcm_mmap_control *mmap_control; ^~~~~~~~~~~~
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 64188b22..e0931577 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -904,6 +904,7 @@ static bool map_control_data(snd_pcm_hw_t *hw, struct snd_pcm_mmap_control *mmap_control; bool 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,
When failing to map status data, applications can query status of runtime of PCM substream by executing ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR. This operation is available for several purposes; e.g. to update control data in kernel space.
When querying status, the applications don't need to update control data in kernel space. This commit adds a helper function to query status/control data without issuing the control just in fallback from failure of status mmap.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index e0931577..2b5f08be 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -151,6 +151,20 @@ static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) return 0; }
+static int query_state(snd_pcm_hw_t *hw) +{ + if (!hw->mmap_status_fallbacked) + return 0; + + /* + * Query both of control/status data to avoid unexpected change of + * control data in kernel space. + */ + return sync_ptr1(hw, + 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) { @@ -542,7 +556,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 = query_state(hw); if (err < 0) return err; return (snd_pcm_state_t) hw->mmap_status->state;
When executing ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I], applications request ALSA PCM core to handle PCM frames for buffer given as an argument. This operation moves appl_ptr of runtime of PCM substream in kernel space. Additionally, for some cases, the operation shifts state of the runtime.
When alsa-lib applications run with fallback mode from failure of page mapping of status/control data, the data in user space should be updated. Current implementation of alsa-lib follows this principle, however for this purpose, an added helper function can be available.
This commit replaces with the function. In this timing, no need to change avail_min of runtime, thus this commit also suppresses it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 2b5f08be..50619fe9 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -819,8 +819,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_state(hw); #ifdef DEBUG_RW fprintf(stderr, "hw_writei: frames = %li, xferi.result = %li, err = %i\n", size, xferi.result, err); #endif @@ -838,8 +840,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_state(hw); #ifdef DEBUG_RW fprintf(stderr, "hw_writen: frames = %li, result = %li, err = %i\n", size, xfern.result, err); #endif @@ -857,8 +861,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_state(hw); #ifdef DEBUG_RW fprintf(stderr, "hw_readi: frames = %li, result = %li, err = %i\n", size, xferi.result, err); #endif @@ -876,8 +882,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_state(hw); #ifdef DEBUG_RW fprintf(stderr, "hw_readn: frames = %li, result = %li, err = %i\n", size, xfern.result, err); #endif
When executing ioctl(2) with SNDRV_PCM_IOCTL_HW_PARAMS, applications request ALSA PCM core to decide configurations of runtime of PCM substream. This operation shifts state of the runtime after the decision. Additionally, avail_min of the runtime is reset to the size of one period of PCM buffer.
When alsa-lib applications run with fallback mode from failure of page mapping of status/control data, the data in user space should be updated. On the other hand, they don't need to change/update control data. Current implementation of alsa-lib satisfies this principle with sub-effect to the control data. For this purpose, an added helper function can be available.
This commit replaces with the function.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 50619fe9..aa548529 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -349,7 +349,7 @@ static int snd_pcm_hw_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params) params->info &= ~0xf0000000; if (pcm->tstamp_type != SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY) params->info |= SND_PCM_INFO_MONOTONIC; - return sync_ptr(hw, 0); + return query_state(hw); }
static void snd_pcm_hw_close_timer(snd_pcm_hw_t *hw)
In ALSA PCM core, calls of ioctl(2) with SNDRV_PCM_IOCTL_PREPARE and SNDRV_PCM_IOCTL_RESET have effects to initialize appl_ptr to current hw_ptr and shift state of runtime. On the other hand, avail_min of the runtime is kept as is.
In a fallback mode of status data mapping, after executing the call, applications should execute ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR to query appl_ptr and the state, while don't need to issue avail_min.
This commit applies a minor optimization to the call.
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 aa548529..6f20cbff 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -615,7 +615,7 @@ static int snd_pcm_hw_prepare(snd_pcm_t *pcm) SYSMSG("SNDRV_PCM_IOCTL_PREPARE failed (%i)", err); return err; } - return sync_ptr(hw, SNDRV_PCM_SYNC_PTR_APPL); + return query_state(hw); }
static int snd_pcm_hw_reset(snd_pcm_t *pcm) @@ -627,7 +627,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_state(hw); }
static int snd_pcm_hw_start(snd_pcm_t *pcm)
When operating rewind/forward, appl_ptr is recalculated by ALSA PCM core in kernel space. Therefore, after the operations, applications should query appl_ptr.
This commit utilizes a helper function for this purpose. The value of avail_min is relevant to this operation, thus just queried.
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 6f20cbff..59ba33b5 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -702,7 +702,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_state(hw); if (err < 0) return err; return frames; @@ -723,7 +723,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_state(hw); if (err < 0) return err; return frames;
When calculating available space on PCM buffer, positions of hw_ptr and appl_ptr are required. When applications map staus and control data successfully, they can calculate the space with architecture's support for cache coherency. When failed, they should execute ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR command to query these positions to kernel space.
This commit uses an added helper function for this purpose. For this call, no need to update appl_ptr and avail_min in kernel space.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 59ba33b5..c6abf170 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -1066,7 +1066,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); + query_state(hw); avail = snd_pcm_mmap_avail(pcm); switch (FAST_PCM_STATE(hw)) { case SNDRV_PCM_STATE_RUNNING:
SNDRV_PCM_IOCTL_SYNC_PTR command for ioctl(2) with SNDRV_PCM_SYNC_PTR_HWSYNC flag has an effect to update hw_ptr. This is an alternative of SNDRV_PCM_IOCTL_HWSYNC but caller can get current status simultaneously.
This commit adds a helper function just to issue hwsync. To avoid side-effect to change appl_ptr and avail_min, this commit uses SNDRV_PCM_SYNC_PTR_APPL and SNDRV_PCM_SYNC_PTR_AVAIL_MIN flags.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index c6abf170..43c37b83 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -151,6 +151,21 @@ static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) return 0; }
+static int request_hwsync(snd_pcm_hw_t *hw) +{ + if (!hw->mmap_status_fallbacked) + return 0; + + /* + * Query both of control/status data to avoid unexpected change of + * control data in kernel space. + */ + return sync_ptr1(hw, + SNDRV_PCM_SYNC_PTR_HWSYNC | + SNDRV_PCM_SYNC_PTR_APPL | + SNDRV_PCM_SYNC_PTR_AVAIL_MIN); +} + static int query_state(snd_pcm_hw_t *hw) { if (!hw->mmap_status_fallbacked) @@ -579,8 +594,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, SNDRV_PCM_SYNC_PTR_HWSYNC); + if (hw->mmap_status_fallbacked) { + err = request_hwsync(hw); if (err < 0) return err; } else {
After starting, PCM substream shift its state to running and applications can move appl_ptr by several ways. When status and control data of runtime of the PCM substream is not mapped, the applications should issue appl_ptr to kernel land. In this case, when any PCM frames is handled by mmap operation, the applications should issue appl_ptr to update.
This commit adds a helper function for this purpose. To avoid unexpected change of avail_min, this commit uses a flag just to update appl_ptr.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index 43c37b83..aacfa669 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -151,6 +151,15 @@ static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) return 0; }
+static int issue_applptr(snd_pcm_hw_t *hw) +{ + if (!hw->mmap_control_fallbacked) + return 0; + + /* Avoid unexpected change of avail_min in kernel space. */ + return sync_ptr1(hw, SNDRV_PCM_SYNC_PTR_AVAIL_MIN); +} + static int request_hwsync(snd_pcm_hw_t *hw) { if (!hw->mmap_status_fallbacked) @@ -653,7 +662,7 @@ static int snd_pcm_hw_start(snd_pcm_t *pcm) assert(pcm->stream != SND_PCM_STREAM_PLAYBACK || snd_pcm_mmap_playback_hw_avail(pcm) > 0); #endif - sync_ptr(hw, 0); + issue_applptr(hw); if (ioctl(hw->fd, SNDRV_PCM_IOCTL_START) < 0) { err = -errno; SYSMSG("SNDRV_PCM_IOCTL_START failed (%i)", err); @@ -1069,7 +1078,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); - sync_ptr(hw, 0); + 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
At present, applications can change avail_min parameter of PCM substream by two ways; via mapped control data, and by executing ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR. The former is available in a case that the applications map the data successfully.
When utilizing alsa-lib API, the above is done by a call of 'snd_pcm_sw_params()' to hw PCM plugin. In current implementation, this call has an sub-effect to issue appl_ptr unexpectedly.
This commit adds a helper function to issue avail_min without the sub-effect.
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 aacfa669..c8fef64d 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -151,6 +151,15 @@ static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) return 0; }
+static int issue_avail_min(snd_pcm_hw_t *hw) +{ + if (!hw->mmap_control_fallbacked) + return 0; + + /* Avoid unexpected change of applptr in kernel space. */ + return sync_ptr1(hw, SNDRV_PCM_SYNC_PTR_APPL); +} + static int issue_applptr(snd_pcm_hw_t *hw) { if (!hw->mmap_control_fallbacked) @@ -492,7 +501,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_avail_min(hw); } if (params->tstamp_type == SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW && hw->version < SNDRV_PROTOCOL_VERSION(2, 0, 12)) {
SNDRV_PCM_IOCTL_SYNC_PTR command was introduced to PCM protocol/interface in its version 2.0.7, however this command is used in a branch for the newer version protocol/interface in snd_pcm_hw_forward().
This commit removes the superfluous code as a part of work for code refactoring.
Fixes: eafb4925124b ("- added SYNC_PTR ioctl support for pcm_hw plugin") Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 13 ------------- 1 file changed, 13 deletions(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index c8fef64d..a849c644 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -144,13 +144,6 @@ static int sync_ptr1(snd_pcm_hw_t *hw, unsigned int flags) return 0; }
-static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags) -{ - if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked) - return sync_ptr1(hw, flags); - return 0; -} - static int issue_avail_min(snd_pcm_hw_t *hw) { if (!hw->mmap_control_fallbacked) @@ -763,9 +756,6 @@ 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); - if (err < 0) - return err; switch (FAST_PCM_STATE(hw)) { case SNDRV_PCM_STATE_RUNNING: case SNDRV_PCM_STATE_DRAINING: @@ -783,9 +773,6 @@ 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); - if (err < 0) - return err; return frames; } }
At failure of control data mapping, alsa-lib goes to fallback mode. In this mode, it keeps a buffer in user space and executes ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR to issue/query control data as necessary. The effect of this operation can be managed by passing corresponding flags. When 0 is passing as the flag, all members in the control data are updated in kernel space. This is used when control data is initialized.
This commit adds a minor code refactoring for the initialization.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- src/pcm/pcm_hw.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index a849c644..9608e89d 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -999,11 +999,9 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback) snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL); 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); + err = sync_ptr1(hw, 0); + if (err < 0) return err; - } }
return 0;
Hi,
I realized a part of this patchset brings regressions due to missing to handle appl_ptr correctly for driver with SNDRV_PCM_INFO_SYNC_APPLPTR. Below patches are invalid.
* pcm: hw: use heler function to query status/control data after reading/writing PCM frames * pcm: hw: use helper function to query status/control data after PREPARE/RESET call * pcm: hw: use helper function to query status/control data after REWIND/FORWARD call
On these patches, a helper function, query_status() is used to execute ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR, however it is done in a case that status data mapping is failed. However, for the drivers, ALSA PCM core allows to map status data and the ioctl is not executed against my expectation. Although these operations changes appl_ptr in kernel space, appl_ptr in user space is not synchronized to the one in kernel space.
This regression brings some operation failures. For example, as I confirmed, pulseaudio gets unexpected appl_ptr when it operates REWIND. In the program, this is detected underrun and PCM frames are sent doubly as a recovery. Users hear sound with short repetition after the operation.
Please abandon this patchset. Later, I'll send second version of this patchset.
On Jun 30 2017 08:58, Takashi Sakamoto wrote:
Hi,
This patchset is an optimization for version 2.0.14 of ALSA PCM protocol/interface.
In this version, for some devices/drivers, applications are disallowed to map control data of runtime of PCM substream. On the other hand, mapping status data is still available.
In current implementation of alsa-lib, execution of ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR is still done without enough care of the change. For example, even if status data is successfully mapped, the ioctl is executed to query the status data. This is inefficient.
This patchset is to arrange the execution.
Takashi Sakamoto (12): pcm: hw: fix to initialize function local variable pcm: hw: add a helper function just to query status/control data of PCM substream pcm: hw: use heler function to query status/control data after reading/writing PCM frames pcm: hw: use helper function to query status/control data after HW_PARAMS call pcm: hw: use helper function to query status/control data after PREPARE/RESET call pcm: hw: use helper function to query status/control data after REWIND/FORWARD call pcm: hw: use helper function to query status/control data for calculation of available space on PCM buffer pcm: hw: add a helper function to request hwsync without side-effects pcm: hw: add a helper function to issue appl_ptr without sub-effects pcm: hw: add a helper function to issue avail_min without sub-effects pcm: hw: remove superfluous code to call of SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_hw_forward() pcm: hw: minor refactoring for initialization of control data
src/pcm/pcm_hw.c | 109 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 75 insertions(+), 34 deletions(-)
Regards
Takashi Sakamoto
participants (1)
-
Takashi Sakamoto