[RFC,2/4] ALSA: core: add .notify callback for pcm ops
diff mbox

Message ID 1436350236-17509-3-git-send-email-pierre-louis.bossart@linux.intel.com
State New
Headers show

Commit Message

Pierre-Louis Bossart July 8, 2015, 10:10 a.m. UTC
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>
---
 include/sound/pcm.h     |  2 ++
 sound/core/pcm_native.c | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Comments

Takashi Iwai July 8, 2015, 2:27 p.m. UTC | #1
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
>
Pierre-Louis Bossart July 8, 2015, 5:10 p.m. UTC | #2
>> 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.

Nope, this is explicitly not supported, so unfortunately if we want to 
optimize for power and let hardware fetch data when it's most 
appropriate rewinds need to be disabled.

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

It's fine to remove the check. I added this based on internal review 
comments but I agree with your point.

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

The first open I had was to know if we could use .ack for this? if a 
different callback is needed, we can use 'appl_ptr_update' instead of 
'notify'

>
>
> 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
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Raymond Yau July 9, 2015, 7:25 a.m. UTC | #3
> >
> > 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.
>
>

If driver specify no rewind flag, should alsa lib

1) return error when application call snd_pcm_rewind() and
snd_pcm_forward() ?
2) return zero when call snd_pcm_rewindable() and snd_pcm_forwardable()

How can the application recover when hw_ptr is behind appl_ptr when stop
threshold is set to boundary ?

Do you mean compressed audio stream don't support rewind and forward ?
Pierre-Louis Bossart July 9, 2015, 7:35 a.m. UTC | #4
On 7/9/15 9:25 AM, Raymond Yau 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.
>>
>>
>
> If driver specify no rewind flag, should alsa lib
>
> 1) return error when application call snd_pcm_rewind() and
> snd_pcm_forward() ?

no, it would return 0 on rewind and the number of frames on forward. In 
other words the value returned is consistent with the function prototype 
which has no scope for errors.

> 2) return zero when call snd_pcm_rewindable() and snd_pcm_forwardable()

again zero for rewind and the actual number for forward.

> How can the application recover when hw_ptr is behind appl_ptr when stop
> threshold is set to boundary ?

Don't understand the question, there is no change here.

> Do you mean compressed audio stream don't support rewind and forward ?

You can't rewind in a compressed bitstream in general without losing 
sync or missing state information when there are dependencies between 
frames (linear prediction or filter with history). Forward is the same.
Some encoders allow for skips to well identified markers but random 
access is not possible or desired in general.

> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Takashi Iwai July 9, 2015, 2:44 p.m. UTC | #5
On Wed, 08 Jul 2015 19:10:32 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >> 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.
> 
> Nope, this is explicitly not supported, so unfortunately if we want to 
> optimize for power and let hardware fetch data when it's most 
> appropriate rewinds need to be disabled.
> 
> > In other words, I see no reason to strict notify callback only for
> > no_rewinds.  This is an optional ops in anyway.
> 
> It's fine to remove the check. I added this based on internal review 
> comments but I agree with your point.

OK, then let's treat the NO_REWIND and new ops individually.

> > 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.
> 
> The first open I had was to know if we could use .ack for this? if a 
> different callback is needed, we can use 'appl_ptr_update' instead of 
> 'notify'

As there are no many users of ack callback, I don't mind to reuse it.
But then we need to extend the function to receive a new argument
indicating the type of ack, right?


thanks,

Takashi

Patch
diff mbox

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