[2/7] ALSA: core: add .update_appl_ptr callback for pcm ops
diff mbox

Message ID 1475239410-16548-3-git-send-email-subhransu.s.prusty@intel.com
State New
Headers show

Commit Message

Subhransu S. Prusty Sept. 30, 2016, 12:43 p.m. UTC
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

When appl_ptr is updated let low-level driver know, e.g.  to let the
low-level driver/hardware pre-fetch data opportunistically.

The existing .ack callback could be used but it would need to be
extended with new arguments, resulting in multiple changes in legacy
code.

Instead a new .appl_ptr_update callback is added.  The difference
between .ack and .appl_ptr_update is that .ack is only called on read or
write. .appl_ptr_update is called on read, write, rewind, forward or
when updating the appl_ptr from userspace.

In the ALSA core, this capability is independent from the NO_REWIND
hardware flag. The low-level driver may however tie both options and
only use the updated appl_ptr when rewinds are disabled due to hardware
limitations.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Babu, Ramesh <ramesh.babu@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 include/sound/pcm.h     |  1 +
 sound/core/pcm_lib.c    |  6 ++++++
 sound/core/pcm_native.c | 24 +++++++++++++++++++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

Comments

Takashi Iwai Sept. 30, 2016, 1:24 p.m. UTC | #1
On Fri, 30 Sep 2016 14:43:25 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> When appl_ptr is updated let low-level driver know, e.g.  to let the
> low-level driver/hardware pre-fetch data opportunistically.
> 
> The existing .ack callback could be used but it would need to be
> extended with new arguments, resulting in multiple changes in legacy
> code.

I wouldn't mind changing these callers.  They aren't so many, after
all. 


Takashi

> Instead a new .appl_ptr_update callback is added.  The difference
> between .ack and .appl_ptr_update is that .ack is only called on read or
> write. .appl_ptr_update is called on read, write, rewind, forward or
> when updating the appl_ptr from userspace.
> 
> In the ALSA core, this capability is independent from the NO_REWIND
> hardware flag. The low-level driver may however tie both options and
> only use the updated appl_ptr when rewinds are disabled due to hardware
> limitations.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Babu, Ramesh <ramesh.babu@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  include/sound/pcm.h     |  1 +
>  sound/core/pcm_lib.c    |  6 ++++++
>  sound/core/pcm_native.c | 24 +++++++++++++++++++++++-
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 5344c16..1accb8b 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -87,6 +87,7 @@ 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 (*appl_ptr_update)(struct snd_pcm_substream *substream);
>  };
>  
>  /*
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index bb12615..1656ca9 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2090,6 +2090,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  		if (substream->ops->ack)
>  			substream->ops->ack(substream);
>  
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +
>  		offset += frames;
>  		size -= frames;
>  		xfer += frames;
> @@ -2322,6 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  		if (substream->ops->ack)
>  			substream->ops->ack(substream);
>  
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +
>  		offset += frames;
>  		size -= frames;
>  		xfer += frames;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index be8617b..c56d4ed 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2475,6 +2475,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2526,6 +2530,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2575,6 +2583,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 (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2624,6 +2636,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 (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2723,8 +2739,14 @@ 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)) {
> +		/* boundary wrap-around is assumed to be handled in userspace */
>  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> +
> +		/* let low-level driver know about appl_ptr change */
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +	}
>  	else
>  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> -- 
> 1.9.1
>
Vinod Koul Sept. 30, 2016, 5:20 p.m. UTC | #2
On Fri, Sep 30, 2016 at 03:24:59PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 14:43:25 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > low-level driver/hardware pre-fetch data opportunistically.
> > 
> > The existing .ack callback could be used but it would need to be
> > extended with new arguments, resulting in multiple changes in legacy
> > code.
> 
> I wouldn't mind changing these callers.  They aren't so many, after
> all. 

Yes this was one of the discussions we had in the past. I don't recall the
conclusion so had recommened to keep as is and discuss here.

Do you think it's better to do that or use a new one :)
Takashi Iwai Sept. 30, 2016, 6:38 p.m. UTC | #3
On Fri, 30 Sep 2016 19:20:10 +0200,
Vinod Koul wrote:
> 
> On Fri, Sep 30, 2016 at 03:24:59PM +0200, Takashi Iwai wrote:
> > On Fri, 30 Sep 2016 14:43:25 +0200,
> > Subhransu S. Prusty wrote:
> > > 
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > low-level driver/hardware pre-fetch data opportunistically.
> > > 
> > > The existing .ack callback could be used but it would need to be
> > > extended with new arguments, resulting in multiple changes in legacy
> > > code.
> > 
> > I wouldn't mind changing these callers.  They aren't so many, after
> > all. 
> 
> Yes this was one of the discussions we had in the past. I don't recall the
> conclusion so had recommened to keep as is and discuss here.
> 
> Do you think it's better to do that or use a new one :)

It's OK to change ack callback, and actually it'll be cleaner.
But then it'll be a problem in the next patch, I suppose :)


Takashi
Vinod Koul Oct. 3, 2016, 4:36 a.m. UTC | #4
On Fri, Sep 30, 2016 at 08:38:38PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 19:20:10 +0200,
> Vinod Koul wrote:
> > 
> > On Fri, Sep 30, 2016 at 03:24:59PM +0200, Takashi Iwai wrote:
> > > On Fri, 30 Sep 2016 14:43:25 +0200,
> > > Subhransu S. Prusty wrote:
> > > > 
> > > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > 
> > > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > low-level driver/hardware pre-fetch data opportunistically.
> > > > 
> > > > The existing .ack callback could be used but it would need to be
> > > > extended with new arguments, resulting in multiple changes in legacy
> > > > code.
> > > 
> > > I wouldn't mind changing these callers.  They aren't so many, after
> > > all. 
> > 
> > Yes this was one of the discussions we had in the past. I don't recall the
> > conclusion so had recommened to keep as is and discuss here.
> > 
> > Do you think it's better to do that or use a new one :)
> 
> It's OK to change ack callback, and actually it'll be cleaner.
> But then it'll be a problem in the next patch, I suppose :)

Yes and one of the reason is that we are using one flag to advertise two
capabilities, one is no rewind and second is appl_ptr update.

We feel we should deal with them by using two flags so that code can be
made cleaner.

Thanks
Pierre-Louis Bossart Oct. 3, 2016, 2:49 p.m. UTC | #5
>>>> When appl_ptr is updated let low-level driver know, e.g.  to let the
>>>> low-level driver/hardware pre-fetch data opportunistically.
>>>>
>>>> The existing .ack callback could be used but it would need to be
>>>> extended with new arguments, resulting in multiple changes in legacy
>>>> code.
>>>
>>> I wouldn't mind changing these callers.  They aren't so many, after
>>> all.
>>
>> Yes this was one of the discussions we had in the past. I don't recall the
>> conclusion so had recommened to keep as is and discuss here.
>>
>> Do you think it's better to do that or use a new one :)
>
> It's OK to change ack callback, and actually it'll be cleaner.
> But then it'll be a problem in the next patch, I suppose :)

The main problem is test/coverage of legacy v. alignment of 
functionality. If we change .ack() then others will have to test for the 
changes and there's a risk of a regression on a specific platform that 
wasn't tested - Murphy's law applies.

Patch
diff mbox

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 5344c16..1accb8b 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -87,6 +87,7 @@  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 (*appl_ptr_update)(struct snd_pcm_substream *substream);
 };
 
 /*
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index bb12615..1656ca9 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2090,6 +2090,9 @@  static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
 		if (substream->ops->ack)
 			substream->ops->ack(substream);
 
+		if (substream->ops->appl_ptr_update)
+			substream->ops->appl_ptr_update(substream);
+
 		offset += frames;
 		size -= frames;
 		xfer += frames;
@@ -2322,6 +2325,9 @@  static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
 		if (substream->ops->ack)
 			substream->ops->ack(substream);
 
+		if (substream->ops->appl_ptr_update)
+			substream->ops->appl_ptr_update(substream);
+
 		offset += frames;
 		size -= frames;
 		xfer += frames;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index be8617b..c56d4ed 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2475,6 +2475,10 @@  static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 		appl_ptr += runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2526,6 +2530,10 @@  static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 		appl_ptr += runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2575,6 +2583,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 (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2624,6 +2636,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 (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2723,8 +2739,14 @@  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)) {
+		/* boundary wrap-around is assumed to be handled in userspace */
 		control->appl_ptr = sync_ptr.c.control.appl_ptr;
+
+		/* let low-level driver know about appl_ptr change */
+		if (substream->ops->appl_ptr_update)
+			substream->ops->appl_ptr_update(substream);
+	}
 	else
 		sync_ptr.c.control.appl_ptr = control->appl_ptr;
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))