On Thu, 05 Jan 2017 14:29:15 +0100,
sutar.mounesh@xxxxxxxxx wrote:
>
> From: Andreas Pape <apape@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Joshua Frkuska <joshua_frkuska@xxxxxxxxxx>
> Signed-off-by: Mounesh Sutar <mounesh_sutar@xxxxxxxxxx>
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
