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

From: Takashi Iwai <tiwai@suse.de>
To: sutar.mounesh@gmail.com
Cc: alsa-devel@alsa-project.org,
	Joshua Frkuska <joshua_frkuska@mentor.com>,
	Andreas Pape <apape@de.adit-jv.com>,
	mounesh_sutar@mentor.com
Subject: Re: [PATCH 2/6 v2] alsa-lib: Fix for sync issue on xrun recover
Date: Thu, 05 Jan 2017 15:21:33 +0100	[thread overview]
Message-ID: <s5hinpt37v6.wl-tiwai@suse.de> (raw)
In-Reply-To: <1483622955-1899-1-git-send-email-sutar.mounesh@gmail.com>

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