Re: [RFC PATCH 2/4] ALSA: core: add .notify callback for pcm ops

From: Takashi Iwai <tiwai@suse.de>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org
Subject: Re: [RFC PATCH 2/4] ALSA: core: add .notify callback for	pcm ops
Date: Wed, 08 Jul 2015 16:27:09 +0200	[thread overview]
Message-ID: <s5hlheqhghu.wl-tiwai@suse.de> (raw)
In-Reply-To: <1436350236-17509-3-git-send-email-pierre-louis.bossart@linux.intel.com>

On Wed, 08 Jul 2015 12:10:34 +0200,
Pierre-Louis Bossart wrote:
> 
> When appl_ptr is updated let low-level driver know.
> 
> This is only enabled when the NO_REWIND hardware flag is used,
> so that the low-level driver/hardware to opportunistically pre-fetch
> data.
> 
> FIXME: should we rely on .ack for this?
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Hmm, OK, so the forward is allowed but with workarounds...
But then why rewind won't work in a similar way?  DSP might be able to
cancel some of inflight data.

In other words, I see no reason to strict notify callback only for
no_rewinds.  This is an optional ops in anyway.

Also, I find the name "notify" a bit too ambiguous.  In this case,
it's notifying the applptr change.  So, a name related with the
function would be more understandable.

thanks,

Takashi

> ---
>  include/sound/pcm.h     |  2 ++
>  sound/core/pcm_native.c | 18 +++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 25310b7..d5eff03 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -87,6 +87,8 @@ struct snd_pcm_ops {
>  			     unsigned long offset);
>  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
>  	int (*ack)(struct snd_pcm_substream *substream);
> +	int (*notify)(struct snd_pcm_substream *substream);
> +	/* FIXME: what's the difference between ack and notify ? */
>  };
>  
>  /*
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index a70e52d..dd519b8 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2565,6 +2565,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (runtime->no_rewinds && substream->ops->notify)
> +		substream->ops->notify(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2614,6 +2618,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (runtime->no_rewinds && substream->ops->notify)
> +		substream->ops->notify(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2713,8 +2721,16 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>  			return err;
>  	}
>  	snd_pcm_stream_lock_irq(substream);
> -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> +		/* FIXME: this code is used by mmap_commit, should it handle boundary
> +		 *   wrap-around as done for read/write in pcm_lib.c
> +		 */
> +
>  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> +		/* if supported, let low-level driver know about appl_ptr change */
> +		if (runtime->no_rewinds && substream->ops->notify)
> +			substream->ops->notify(substream);
> +	}
>  	else
>  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 