diff mbox

[2/6,v2] alsa-lib: Fix for sync issue on xrun recover

Message ID 1483622955-1899-1-git-send-email-sutar.mounesh@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

sutar.mounesh@gmail.com Jan. 5, 2017, 1:29 p.m. UTC
From: Andreas Pape <apape@de.adit-jv.com>

If using very short periods, DSHARE/DSNOOP/DMIX may report underruns while in
status 'prepared'. This prohibits correct recovery. Now slave xrun conditions
for DSHARE/DSNOOP/DMIX are being handled properly.

Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
Signed-off-by: Mounesh Sutar <mounesh_sutar@mentor.com>

Comments

Takashi Iwai Jan. 5, 2017, 2:21 p.m. UTC | #1
On Thu, 05 Jan 2017 14:29:15 +0100,
sutar.mounesh@gmail.com wrote:
> 
> From: Andreas Pape <apape@de.adit-jv.com>
> 
> If using very short periods, DSHARE/DSNOOP/DMIX may report underruns while in
> status 'prepared'. This prohibits correct recovery. Now slave xrun conditions
> for DSHARE/DSNOOP/DMIX are being handled properly.
> 
> Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
> Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
> Signed-off-by: Mounesh Sutar <mounesh_sutar@mentor.com>

The codes look mostly good, but minor coding style issues:

> +int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct)
> +{
> +	int ret = 0;

The initialization can be dropped.

> +	if (ret=snd_pcm_direct_semaphore_down(direct, DIRECT_IPC_SEM_CLIENT)) {

Please avoid this style.  I guess gcc -Wall would complain, too.
Simply write like

	ret = xxx();
	if (ret < 0) {
		....

Ditto for all other parts.

> +		SNDERR("SEMDOWN FAILED with err %d", ret);
> +		return ret;
> +	}
> +
> +	if (snd_pcm_state(direct->spcm) != SND_PCM_STATE_XRUN) {
> +		/*ignore... someone else already did recovery*/

Please put a space between "/*" and the text (also to the close, too).


> +		if (ret = snd_pcm_direct_semaphore_up(direct,
> +						      DIRECT_IPC_SEM_CLIENT)) {
> +			SNDERR("SEMUP FAILED with err %d", ret);
> +		}
> +		return ret;
> +	}
> +
> +	ret = snd_pcm_prepare(direct->spcm);
> +	if (ret < 0) {
> +		SNDERR("recover: unable to prepare slave");
> +		if (ret = snd_pcm_direct_semaphore_up(direct,
> +						      DIRECT_IPC_SEM_CLIENT)) {
> +			SNDERR("SEMUP FAILED with err %d", ret);
> +		}
> +		return ret;

This may end up with the return zero if snd_pcm_direct_semaphore_up()
succeeds.  Use another variable.

> +	ret = snd_pcm_start(direct->spcm);
> +	if (ret < 0) {
> +		SNDERR("recover: unable to start slave");
> +		if (ret = snd_pcm_direct_semaphore_up(direct,
> +						      DIRECT_IPC_SEM_CLIENT)) {
> +			SNDERR("SEMUP FAILED with err %d", ret);
> +		}
> +		return ret;

Ditto.


> +/*
> + * enter xrun state, if slave xrun occured
> + * @return: 0 - no xrun >0: xrun happened
> + */
> +int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm)
> +{
> +	if (direct->shmptr->recoveries != direct->recoveries) {
> +		/* no matter how many xruns we missed -
> +		so don't increment but just update to actual counter*/

Please align the comment,
	/*
	 * blah blah
	 * blah blah
	 */
or
	/* blah blah
	 * blah blah
	 */
or
	/* blah blah */



> @@ -572,6 +653,10 @@ int snd_pcm_direct_poll_revents(snd_pcm_t *pcm, struct pollfd *pfds, unsigned in
>  	}
>  	switch (snd_pcm_state(dmix->spcm)) {
>  	case SND_PCM_STATE_XRUN:
> +		/*recover slave and update client state to xrun
> +		before returning POLLERR*/
> +		snd_pcm_direct_slave_recover(dmix);
> +		snd_pcm_direct_client_chk_xrun(dmix, pcm);

Put a comment here like

		/* fallthrough */

to indicate the fall-thru line.

> @@ -841,8 +851,10 @@ static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm,
>  		if ((err = snd_pcm_dmix_start_timer(pcm, dmix)) < 0)
>  			return err;
>  	} else if (dmix->state == SND_PCM_STATE_RUNNING ||
> -		   dmix->state == SND_PCM_STATE_DRAINING)
> -		snd_pcm_dmix_sync_ptr(pcm);
> +		   dmix->state == SND_PCM_STATE_DRAINING) {
> +		if (( err = snd_pcm_dmix_sync_ptr(pcm)) < 0)

Avoid unnecessary space after the parenthesis.

> +			return err;
> +	}
>  	if (dmix->state == SND_PCM_STATE_RUNNING ||
>  	    dmix->state == SND_PCM_STATE_DRAINING) {
>  		/* ok, we commit the changes after the validation of area */
> @@ -858,10 +870,13 @@ static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm,
>  static snd_pcm_sframes_t snd_pcm_dmix_avail_update(snd_pcm_t *pcm)
>  {
>  	snd_pcm_direct_t *dmix = pcm->private_data;
> +	int err;
>  	
>  	if (dmix->state == SND_PCM_STATE_RUNNING ||
> -	    dmix->state == SND_PCM_STATE_DRAINING)
> -		snd_pcm_dmix_sync_ptr(pcm);
> +	    dmix->state == SND_PCM_STATE_DRAINING) {
> +		if (( err = snd_pcm_dmix_sync_ptr(pcm)) < 0)

Ditto.

> diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
> index a1fed5d..ef1e6c1 100644
> --- a/src/pcm/pcm_dshare.c
> +++ b/src/pcm/pcm_dshare.c
> @@ -162,7 +162,6 @@ static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_p
>  	snd_pcm_direct_t *dshare = pcm->private_data;
>  	snd_pcm_uframes_t old_slave_hw_ptr, avail;
>  	snd_pcm_sframes_t diff;
> -	
>  	old_slave_hw_ptr = dshare->slave_hw_ptr;
>  	dshare->slave_hw_ptr = slave_hw_ptr;
>  	diff = slave_hw_ptr - old_slave_hw_ptr;

Don't remove, a blank line after the function declaration is preferred.


thanks,

Takashi
diff mbox

Patch

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 0770abc..1056308 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -550,6 +550,87 @@  int snd_pcm_direct_timer_stop(snd_pcm_direct_t *dmix)
 	return 0;
 }
 
+/*
+ * Recover slave on XRUN.
+ * Even if direct plugins disable xrun detection, there might be an xrun
+ * raised directly by some drivers.
+ * The first client recovers slave pcm.
+ * Each client needs to execute sw xrun handling afterwards
+ */
+int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct)
+{
+	int ret = 0;
+
+	if (ret=snd_pcm_direct_semaphore_down(direct, DIRECT_IPC_SEM_CLIENT)) {
+		SNDERR("SEMDOWN FAILED with err %d", ret);
+		return ret;
+	}
+
+	if (snd_pcm_state(direct->spcm) != SND_PCM_STATE_XRUN) {
+		/*ignore... someone else already did recovery*/
+		if (ret = snd_pcm_direct_semaphore_up(direct,
+						      DIRECT_IPC_SEM_CLIENT)) {
+			SNDERR("SEMUP FAILED with err %d", ret);
+		}
+		return ret;
+	}
+
+	ret = snd_pcm_prepare(direct->spcm);
+	if (ret < 0) {
+		SNDERR("recover: unable to prepare slave");
+		if (ret = snd_pcm_direct_semaphore_up(direct,
+						      DIRECT_IPC_SEM_CLIENT)) {
+			SNDERR("SEMUP FAILED with err %d", ret);
+		}
+		return ret;
+	}
+
+	if (direct->type == SND_PCM_TYPE_DSHARE) {
+		const snd_pcm_channel_area_t *dst_areas;
+		dst_areas = snd_pcm_mmap_areas(direct->spcm);
+		snd_pcm_areas_silence(dst_areas, 0, direct->spcm->channels,
+				      direct->spcm->buffer_size,
+				      direct->spcm->format);
+	}
+
+	ret = snd_pcm_start(direct->spcm);
+	if (ret < 0) {
+		SNDERR("recover: unable to start slave");
+		if (ret = snd_pcm_direct_semaphore_up(direct,
+						      DIRECT_IPC_SEM_CLIENT)) {
+			SNDERR("SEMUP FAILED with err %d", ret);
+		}
+		return ret;
+	}
+	direct->shmptr->recoveries++;
+	if (ret = snd_pcm_direct_semaphore_up(direct, DIRECT_IPC_SEM_CLIENT)) {
+		SNDERR("SEMUP FAILED with err %d", ret);
+	}
+	return ret;
+}
+
+/*
+ * enter xrun state, if slave xrun occured
+ * @return: 0 - no xrun >0: xrun happened
+ */
+int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm)
+{
+	if (direct->shmptr->recoveries != direct->recoveries) {
+		/* no matter how many xruns we missed -
+		so don't increment but just update to actual counter*/
+		direct->recoveries = direct->shmptr->recoveries;
+		pcm->fast_ops->drop(pcm);
+		/*trigger_tstamp update is missing in drop callbacks*/
+		gettimestamp(&direct->trigger_tstamp, pcm->tstamp_type);
+		/*no timer clear:
+		if slave already entered xrun again the event is lost...*/
+		/*snd_pcm_direct_clear_timer_queue(direct);*/
+		direct->state = SND_PCM_STATE_XRUN;
+		return 1;
+	}
+	return 0;
+}
+
 int snd_pcm_direct_poll_revents(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int nfds, unsigned short *revents)
 {
 	snd_pcm_direct_t *dmix = pcm->private_data;
@@ -572,6 +653,10 @@  int snd_pcm_direct_poll_revents(snd_pcm_t *pcm, struct pollfd *pfds, unsigned in
 	}
 	switch (snd_pcm_state(dmix->spcm)) {
 	case SND_PCM_STATE_XRUN:
+		/*recover slave and update client state to xrun
+		before returning POLLERR*/
+		snd_pcm_direct_slave_recover(dmix);
+		snd_pcm_direct_client_chk_xrun(dmix, pcm);
 	case SND_PCM_STATE_SUSPENDED:
 	case SND_PCM_STATE_SETUP:
 		events |= POLLERR;
@@ -1382,6 +1467,7 @@  int snd_pcm_direct_open_secondary_client(snd_pcm_t **spcmp, snd_pcm_direct_t *dm
 	dmix->slave_buffer_size = spcm->buffer_size;
 	dmix->slave_period_size = dmix->shmptr->s.period_size;
 	dmix->slave_boundary = spcm->boundary;
+	dmix->recoveries = dmix->shmptr->recoveries;
 
 	ret = snd_pcm_mmap(spcm);
 	if (ret < 0) {
diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h
index 91e816c..569d2be 100644
--- a/src/pcm/pcm_direct.h
+++ b/src/pcm/pcm_direct.h
@@ -66,6 +66,7 @@  typedef struct {
 	char socket_name[256];			/* name of communication socket */
 	snd_pcm_type_t type;			/* PCM type (currently only hw) */
 	int use_server;
+	unsigned int recoveries;		/* no of executed recoveries on slave*/
 	struct {
 		unsigned int format;
 		snd_interval_t rate;
@@ -157,6 +158,7 @@  struct snd_pcm_direct {
 	int var_periodsize;		/* allow variable period size if max_periods is != -1*/
 	unsigned int channels;		/* client's channels */
 	unsigned int *bindings;
+	unsigned int recoveries;	/* mirror of executed recoveries on slave */
 	union {
 		struct {
 			int shmid_sum;			/* IPC global sum ring buffer memory identification */
@@ -318,7 +320,8 @@  int snd_pcm_direct_open_secondary_client(snd_pcm_t **spcmp, snd_pcm_direct_t *dm
 snd_pcm_chmap_query_t **snd_pcm_direct_query_chmaps(snd_pcm_t *pcm);
 snd_pcm_chmap_t *snd_pcm_direct_get_chmap(snd_pcm_t *pcm);
 int snd_pcm_direct_set_chmap(snd_pcm_t *pcm, const snd_pcm_chmap_t *map);
-
+int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct);
+int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm);
 int snd_timer_async(snd_timer_t *timer, int sig, pid_t pid);
 struct timespec snd_pcm_hw_fast_tstamp(snd_pcm_t *pcm);
 
diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index 0ab7323..b1c846b 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -434,15 +434,21 @@  static int snd_pcm_dmix_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr
 static int snd_pcm_dmix_sync_ptr(snd_pcm_t *pcm)
 {
 	snd_pcm_direct_t *dmix = pcm->private_data;
+	int err;
 
 	switch (snd_pcm_state(dmix->spcm)) {
 	case SND_PCM_STATE_DISCONNECTED:
 		dmix->state = SND_PCM_STATE_DISCONNECTED;
 		return -ENODEV;
+	case SND_PCM_STATE_XRUN:
+		if ((err = snd_pcm_direct_slave_recover(dmix)) <0)
+			return err;
+		break;
 	default:
 		break;
 	}
-
+	if (snd_pcm_direct_client_chk_xrun(dmix, pcm))
+		return -EPIPE;
 	if (dmix->slowptr)
 		snd_pcm_hwsync(dmix->spcm);
 
@@ -828,12 +834,16 @@  static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm,
 
 	switch (snd_pcm_state(dmix->spcm)) {
 	case SND_PCM_STATE_XRUN:
-		return -EPIPE;
+		if ((err = snd_pcm_direct_slave_recover(dmix)) < 0)
+			return err;
+		break;
 	case SND_PCM_STATE_SUSPENDED:
 		return -ESTRPIPE;
 	default:
 		break;
 	}
+	if (snd_pcm_direct_client_chk_xrun(dmix, pcm))
+		return -EPIPE;
 	if (! size)
 		return 0;
 	snd_pcm_mmap_appl_forward(pcm, size);
@@ -841,8 +851,10 @@  static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm,
 		if ((err = snd_pcm_dmix_start_timer(pcm, dmix)) < 0)
 			return err;
 	} else if (dmix->state == SND_PCM_STATE_RUNNING ||
-		   dmix->state == SND_PCM_STATE_DRAINING)
-		snd_pcm_dmix_sync_ptr(pcm);
+		   dmix->state == SND_PCM_STATE_DRAINING) {
+		if (( err = snd_pcm_dmix_sync_ptr(pcm)) < 0)
+			return err;
+	}
 	if (dmix->state == SND_PCM_STATE_RUNNING ||
 	    dmix->state == SND_PCM_STATE_DRAINING) {
 		/* ok, we commit the changes after the validation of area */
@@ -858,10 +870,13 @@  static snd_pcm_sframes_t snd_pcm_dmix_mmap_commit(snd_pcm_t *pcm,
 static snd_pcm_sframes_t snd_pcm_dmix_avail_update(snd_pcm_t *pcm)
 {
 	snd_pcm_direct_t *dmix = pcm->private_data;
+	int err;
 	
 	if (dmix->state == SND_PCM_STATE_RUNNING ||
-	    dmix->state == SND_PCM_STATE_DRAINING)
-		snd_pcm_dmix_sync_ptr(pcm);
+	    dmix->state == SND_PCM_STATE_DRAINING) {
+		if (( err = snd_pcm_dmix_sync_ptr(pcm)) < 0)
+			return err;
+	}
 	return snd_pcm_mmap_playback_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index a1fed5d..ef1e6c1 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -162,7 +162,6 @@  static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_p
 	snd_pcm_direct_t *dshare = pcm->private_data;
 	snd_pcm_uframes_t old_slave_hw_ptr, avail;
 	snd_pcm_sframes_t diff;
-	
 	old_slave_hw_ptr = dshare->slave_hw_ptr;
 	dshare->slave_hw_ptr = slave_hw_ptr;
 	diff = slave_hw_ptr - old_slave_hw_ptr;
@@ -202,15 +201,21 @@  static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_p
 static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
 {
 	snd_pcm_direct_t *dshare = pcm->private_data;
+	int err;
 
 	switch (snd_pcm_state(dshare->spcm)) {
 	case SND_PCM_STATE_DISCONNECTED:
 		dshare->state = SNDRV_PCM_STATE_DISCONNECTED;
 		return -ENODEV;
+	case SND_PCM_STATE_XRUN:
+		if ((err = snd_pcm_direct_slave_recover(dshare)) <0)
+			return err;
+		break;
 	default:
 		break;
 	}
-
+	if (snd_pcm_direct_client_chk_xrun(dshare, pcm))
+		return -EPIPE;
 	if (dshare->slowptr)
 		snd_pcm_hwsync(dshare->spcm);
 
@@ -516,12 +521,16 @@  static snd_pcm_sframes_t snd_pcm_dshare_mmap_commit(snd_pcm_t *pcm,
 
 	switch (snd_pcm_state(dshare->spcm)) {
 	case SND_PCM_STATE_XRUN:
-		return -EPIPE;
+		if ((err = snd_pcm_direct_slave_recover(dshare)) < 0)
+			return err;
+		break;
 	case SND_PCM_STATE_SUSPENDED:
 		return -ESTRPIPE;
 	default:
 		break;
 	}
+	if (snd_pcm_direct_client_chk_xrun(dshare, pcm))
+		return -EPIPE;
 	if (! size)
 		return 0;
 	snd_pcm_mmap_appl_forward(pcm, size);
@@ -529,8 +538,10 @@  static snd_pcm_sframes_t snd_pcm_dshare_mmap_commit(snd_pcm_t *pcm,
 		if ((err = snd_pcm_dshare_start_timer(dshare)) < 0)
 			return err;
 	} else if (dshare->state == SND_PCM_STATE_RUNNING ||
-		   dshare->state == SND_PCM_STATE_DRAINING)
-		snd_pcm_dshare_sync_ptr(pcm);
+		   dshare->state == SND_PCM_STATE_DRAINING) {
+		if ((err = snd_pcm_dshare_sync_ptr(pcm)) < 0)
+			return err;
+	}
 	if (dshare->state == SND_PCM_STATE_RUNNING ||
 	    dshare->state == SND_PCM_STATE_DRAINING) {
 		/* ok, we commit the changes after the validation of area */
@@ -546,10 +557,13 @@  static snd_pcm_sframes_t snd_pcm_dshare_mmap_commit(snd_pcm_t *pcm,
 static snd_pcm_sframes_t snd_pcm_dshare_avail_update(snd_pcm_t *pcm)
 {
 	snd_pcm_direct_t *dshare = pcm->private_data;
+	int err;
 	
 	if (dshare->state == SND_PCM_STATE_RUNNING ||
-	    dshare->state == SND_PCM_STATE_DRAINING)
-		snd_pcm_dshare_sync_ptr(pcm);
+	    dshare->state == SND_PCM_STATE_DRAINING) {
+		if ((err = snd_pcm_dshare_sync_ptr(pcm)) < 0)
+			return err;
+	}
 	return snd_pcm_mmap_playback_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 85f0ff4..6b721ec 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -132,14 +132,21 @@  static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm)
 	snd_pcm_direct_t *dsnoop = pcm->private_data;
 	snd_pcm_uframes_t slave_hw_ptr, old_slave_hw_ptr, avail;
 	snd_pcm_sframes_t diff;
-	
+	int err;
+
 	switch (snd_pcm_state(dsnoop->spcm)) {
 	case SND_PCM_STATE_DISCONNECTED:
 		dsnoop->state = SNDRV_PCM_STATE_DISCONNECTED;
 		return -ENODEV;
+	case SND_PCM_STATE_XRUN:
+		if ((err = snd_pcm_direct_slave_recover(dsnoop)) <0)
+			return err;
+		break;
 	default:
 		break;
 	}
+	if (snd_pcm_direct_client_chk_xrun(dsnoop, pcm))
+		return -EPIPE;
 	if (dsnoop->slowptr)
 		snd_pcm_hwsync(dsnoop->spcm);
 	old_slave_hw_ptr = dsnoop->slave_hw_ptr;
@@ -410,12 +417,16 @@  static snd_pcm_sframes_t snd_pcm_dsnoop_mmap_commit(snd_pcm_t *pcm,
 
 	switch (snd_pcm_state(dsnoop->spcm)) {
 	case SND_PCM_STATE_XRUN:
-		return -EPIPE;
+		if ((err = snd_pcm_direct_slave_recover(dsnoop)) <0)
+			return err;
+		break;
 	case SND_PCM_STATE_SUSPENDED:
 		return -ESTRPIPE;
 	default:
 		break;
 	}
+	if (snd_pcm_direct_client_chk_xrun(dsnoop, pcm))
+		return -EPIPE;
 	if (dsnoop->state == SND_PCM_STATE_RUNNING) {
 		err = snd_pcm_dsnoop_sync_ptr(pcm);
 		if (err < 0)