[PATCH alsa-lib 00/12] pcm: hw: optimization for v2.0.14 of PCM protocol/interface

[PATCH alsa-lib 00/12] pcm: hw: optimization for v2.0.14 of PCM protocol/interface

[Takashi Sakamoto]

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

-- 
2.11.0

[PATCH alsa-lib 01/12] pcm: hw: fix to initialize function local variable

[Takashi Sakamoto]

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH alsa-lib 02/12] pcm: hw: add a helper function just to query status/control data of PCM substream

[Takashi Sakamoto]

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

[PATCH alsa-lib 03/12] pcm: hw: use heler function to query status/control data after reading/writing PCM frames

[Takashi Sakamoto]

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

[PATCH alsa-lib 04/12] pcm: hw: use helper function to query status/control data after HW_PARAMS call

[Takashi Sakamoto]

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

[PATCH alsa-lib 05/12] pcm: hw: use helper function to query status/control data after PREPARE/RESET call

[Takashi Sakamoto]

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

[PATCH alsa-lib 06/12] pcm: hw: use helper function to query status/control data after REWIND/FORWARD call

[Takashi Sakamoto]

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

[PATCH alsa-lib 07/12] pcm: hw: use helper function to query status/control data for calculation of available space on PCM buffer

[Takashi Sakamoto]

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

[PATCH alsa-lib 08/12] pcm: hw: add a helper function to request hwsync without side-effects

[Takashi Sakamoto]

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

[PATCH alsa-lib 09/12] pcm: hw: add a helper function to issue appl_ptr without sub-effects

[Takashi Sakamoto]

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

[PATCH alsa-lib 10/12] pcm: hw: add a helper function to issue avail_min without sub-effects

[Takashi Sakamoto]

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

[PATCH alsa-lib 11/12] pcm: hw: remove superfluous code to call of SNDRV_PCM_IOCTL_SYNC_PTR in snd_pcm_hw_forward()

[Takashi Sakamoto]

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

[PATCH alsa-lib 12/12] pcm: hw: minor refactoring for initialization of control data

[Takashi Sakamoto]

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

Re: [PATCH alsa-lib 00/12] pcm: hw: optimization for v2.0.14 of PCM protocol/interface

[Takashi Sakamoto]

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