diff mbox

[2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

Message ID s5hinkx6lfl.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai May 19, 2017, 1:11 p.m. UTC 
On Thu, 18 May 2017 10:09:23 +0200,
Takashi Iwai wrote:
> 
> On Thu, 18 May 2017 08:18:21 +0200,
> Subhransu S. Prusty wrote:
> > 
> > On Tue, May 16, 2017 at 01:06:57PM +0530, Subhransu S. Prusty wrote:
> > > On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> > > > On Tue, 16 May 2017 03:01:57 +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 is extended with new attribute argument, to
> > > > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > > process the application ptr update in the driver like in the skylake
> > > > > driver which can use this to inform position of appl pointer to host DMA
> > > > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > > submitted with a separate patch set.
> > > > > 
> > > > > 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: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > 
> > > > It might be me who suggested the extension of the ack ops, but now
> > > > looking at the result, I reconsider whether it'd be a better choice if
> > > > we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> > > > rewrite the patch in that way for comparison?
> > > 
> > > Here is the version using update_appl_ptr.
> > 
> > Hi Takashi,
> > 
> > Did you get a chance to look at the update_appl_ptr changes?
> > Please let us know which one will be preferable, will submit the patches
> > accordingly.
> 
> Now I have a mixed feeling.  Using ack() is basically "the right
> thing".  The update of appl_ptr in forward/rewind and sync_ptr should
> be notified to ack() in general.  It's the purpose of ack(), after
> all.  In that sense, we may call ack() without any argument from any
> places.
> 
> The only problem is that the rewind is broken on some drivers, and
> calling ack() may lead to unexpected results.
> 
> That is, we should look at these existing drivers and handle the
> rewind case (negative appl_ptr diff) appropriately -- or maybe we
> should add a flag to disallow the rewind on such drivers.
> After that, ack() can be called safely from all places that update
> appl_ptr.
> 
> ... this is one way.  Another way is to allow a quick hack and doubly
> call a new callback.
> 
> I prefer the former, but obviously it'll take longer.  So it depends
> on urgency.

Now I'm thinking of changes like the following: namely, it allows
return an error from ack (it should be!), and does the proper error
handling in forward / rewind / sync_ptr ioctls.  In addition, the
indirect PCM helper checks the negative appl_ptr movement, and returns
an error appropriately.

The total change is attached below.  It'll be split to patches, but
you get an idea.

The merit by this approach is that we "fix" ack callback and its
usages properly without changing its API.  Whenever appl_ptr is moved,
it should have been called for tracking the update.  Now it behaves
so.  And, it also fixes the forgotten error for rewinds, too.

Comments?


thanks,

Takashi

---
diff mbox

Patch

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index e8cf0b97bf02..3637ddf909a4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -353,9 +353,8 @@  static int snd_bcm2835_pcm_ack(struct snd_pcm_substream *substream)
 	struct snd_pcm_indirect *pcm_indirect = &alsa_stream->pcm_indirect;
 
 	pcm_indirect->hw_queue_size = runtime->hw.buffer_bytes_max;
-	snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
-					   snd_bcm2835_pcm_transfer);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
+						  snd_bcm2835_pcm_transfer);
 }
 
 /* trigger callback */
diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
index 1df7acaaa535..7ade285328cf 100644
--- a/include/sound/pcm-indirect.h
+++ b/include/sound/pcm-indirect.h
@@ -43,7 +43,7 @@  typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
 /*
  * helper function for playback ack callback
  */
-static inline void
+static inline int
 snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 				   struct snd_pcm_indirect *rec,
 				   snd_pcm_indirect_copy_t copy)
@@ -56,6 +56,8 @@  snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if (diff < 0)
+			return -EINVAL;
 		rec->sw_ready += (int)frames_to_bytes(runtime, diff);
 		rec->appl_ptr = appl_ptr;
 	}
@@ -82,6 +84,7 @@  snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 		rec->hw_ready += bytes;
 		rec->sw_ready -= bytes;
 	}
+	return 0;
 }
 
 /*
@@ -109,7 +112,7 @@  snd_pcm_indirect_playback_pointer(struct snd_pcm_substream *substream,
 /*
  * helper function for capture ack callback
  */
-static inline void
+static inline int
 snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 				  struct snd_pcm_indirect *rec,
 				  snd_pcm_indirect_copy_t copy)
@@ -121,6 +124,8 @@  snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if (diff < 0)
+			return -EINVAL;
 		rec->sw_ready -= frames_to_bytes(runtime, diff);
 		rec->appl_ptr = appl_ptr;
 	}
@@ -147,6 +152,7 @@  snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 		rec->hw_ready -= bytes;
 		rec->sw_ready += bytes;
 	}
+	return 0;
 }
 
 /*
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index f3a3580eb44c..a1d1b4540fbb 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2431,13 +2431,68 @@  static int snd_pcm_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int apply_appl_ptr(struct snd_pcm_substream *substream,
+			  snd_pcm_uframes_t appl_ptr)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
+	int ret;
+
+	runtime->control->appl_ptr = appl_ptr;
+	if (substream->ops->ack) {
+		ret = substream->ops->ack(substream);
+		if (ret < 0) {
+			runtime->control->appl_ptr = old_appl_ptr;
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static snd_pcm_sframes_t increase_appl_ptr(struct snd_pcm_substream *substream,
+					   snd_pcm_uframes_t frames,
+					   snd_pcm_sframes_t avail)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_sframes_t appl_ptr;
+	int ret;
+
+	if (avail <= 0)
+		return 0;
+	if (frames > (snd_pcm_uframes_t)avail)
+		frames = avail;
+	appl_ptr = runtime->control->appl_ptr + frames;
+	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
+		appl_ptr -= runtime->boundary;
+	ret = apply_appl_ptr(substream, appl_ptr);
+	return ret < 0 ? ret : frames;
+}
+
+static snd_pcm_sframes_t decrease_appl_ptr(struct snd_pcm_substream *substream,
+					   snd_pcm_uframes_t frames,
+					   snd_pcm_sframes_t avail)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_sframes_t appl_ptr;
+	int ret;
+
+	if (avail <= 0)
+		return 0;
+	if (frames > (snd_pcm_uframes_t)avail)
+		frames = avail;
+	appl_ptr = runtime->control->appl_ptr - frames;
+	if (appl_ptr < 0)
+		appl_ptr += runtime->boundary;
+	runtime->control->appl_ptr = appl_ptr;
+	ret = apply_appl_ptr(substream, appl_ptr);
+	return ret < 0 ? ret : frames;
+}
+
 static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream,
 						 snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t hw_avail;
 
 	if (frames == 0)
 		return 0;
@@ -2462,18 +2517,8 @@  static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 		goto __end;
 	}
 
-	hw_avail = snd_pcm_playback_hw_avail(runtime);
-	if (hw_avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)hw_avail)
-		frames = hw_avail;
-	appl_ptr = runtime->control->appl_ptr - frames;
-	if (appl_ptr < 0)
-		appl_ptr += runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = decrease_appl_ptr(substream, frames,
+				snd_pcm_playback_hw_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2483,9 +2528,7 @@  static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 						snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t hw_avail;
 
 	if (frames == 0)
 		return 0;
@@ -2510,18 +2553,8 @@  static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 		goto __end;
 	}
 
-	hw_avail = snd_pcm_capture_hw_avail(runtime);
-	if (hw_avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)hw_avail)
-		frames = hw_avail;
-	appl_ptr = runtime->control->appl_ptr - frames;
-	if (appl_ptr < 0)
-		appl_ptr += runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = decrease_appl_ptr(substream, frames,
+				snd_pcm_capture_hw_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2531,9 +2564,7 @@  static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
 						  snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t avail;
 
 	if (frames == 0)
 		return 0;
@@ -2559,18 +2590,8 @@  static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
 		goto __end;
 	}
 
-	avail = snd_pcm_playback_avail(runtime);
-	if (avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)avail)
-		frames = avail;
-	appl_ptr = runtime->control->appl_ptr + frames;
-	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
-		appl_ptr -= runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = increase_appl_ptr(substream, frames,
+				snd_pcm_playback_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2580,9 +2601,7 @@  static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
 						 snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t avail;
 
 	if (frames == 0)
 		return 0;
@@ -2608,18 +2627,8 @@  static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
 		goto __end;
 	}
 
-	avail = snd_pcm_capture_avail(runtime);
-	if (avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)avail)
-		frames = avail;
-	appl_ptr = runtime->control->appl_ptr + frames;
-	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
-		appl_ptr -= runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = increase_appl_ptr(substream, frames,
+				snd_pcm_capture_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2721,10 +2730,15 @@  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))
-		control->appl_ptr = sync_ptr.c.control.appl_ptr;
-	else
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
+		if (err < 0) {
+			snd_pcm_stream_unlock_irq(substream);
+			return err;
+		}
+	} else {
 		sync_ptr.c.control.appl_ptr = control->appl_ptr;
+	}
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
 		control->avail_min = sync_ptr.c.control.avail_min;
 	else
diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
index 00fc9241d266..684dc4ddef41 100644
--- a/sound/mips/hal2.c
+++ b/sound/mips/hal2.c
@@ -616,10 +616,9 @@  static int hal2_playback_ack(struct snd_pcm_substream *substream)
 	struct hal2_codec *dac = &hal2->dac;
 
 	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
-	snd_pcm_indirect_playback_transfer(substream,
-					   &dac->pcm_indirect,
-					   hal2_playback_transfer);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream,
+						  &dac->pcm_indirect,
+						  hal2_playback_transfer);
 }
 
 static int hal2_capture_open(struct snd_pcm_substream *substream)
@@ -707,10 +706,9 @@  static int hal2_capture_ack(struct snd_pcm_substream *substream)
 	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
 	struct hal2_codec *adc = &hal2->adc;
 
-	snd_pcm_indirect_capture_transfer(substream,
-					  &adc->pcm_indirect,
-					  hal2_capture_transfer);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream,
+						 &adc->pcm_indirect,
+						 hal2_capture_transfer);
 }
 
 static struct snd_pcm_ops hal2_playback_ops = {
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
index e4cf3187b4dd..429c0fbe3570 100644
--- a/sound/pci/cs46xx/cs46xx_lib.c
+++ b/sound/pci/cs46xx/cs46xx_lib.c
@@ -887,8 +887,8 @@  static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
-	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec,
+						  snd_cs46xx_pb_trans_copy);
 }
 
 static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
@@ -903,8 +903,8 @@  static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
 static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
 {
 	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
-	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec,
+						 snd_cs46xx_cp_trans_copy);
 }
 
 static snd_pcm_uframes_t snd_cs46xx_playback_direct_pointer(struct snd_pcm_substream *substream)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index ef1cf530c929..bdda29f335f6 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -1632,8 +1632,8 @@  static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substr
 	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
 	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
 
-	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec,
+						  fx8010_pb_trans_copy);
 }
 
 static int snd_emu10k1_fx8010_playback_hw_params(struct snd_pcm_substream *substream,
diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
index 96d15db65dfd..f9b424056d0f 100644
--- a/sound/pci/rme32.c
+++ b/sound/pci/rme32.c
@@ -1157,9 +1157,8 @@  static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
 	if (rme32->running & (1 << SNDRV_PCM_STREAM_CAPTURE))
 		rec->hw_queue_size -= cprec->hw_ready;
 	spin_unlock(&rme32->lock);
-	snd_pcm_indirect_playback_transfer(substream, rec,
-					   snd_rme32_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, rec,
+						  snd_rme32_pb_trans_copy);
 }
 
 static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
@@ -1174,9 +1173,8 @@  static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
 static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
 {
 	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
-	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
-					  snd_rme32_cp_trans_copy);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
+						 snd_rme32_cp_trans_copy);
 }
 
 static snd_pcm_uframes_t