[PATCH] ALSA: compress: fix drain calls blocking other compress functions

[PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

The drain and drain_notify callback were blocked by low level driver untill the
draining was complete. Due to this being invoked with big fat mutex held, others
ops like reading timestamp, calling pause, drop were blocked.

So to fix this we add a new snd_compr_drain_notify() API. This would be required
to be invoked by low level driver when drain or partial drain has been completed
by the DSP. Thus we make the drain and partial_drain callback as non blocking
and driver returns immediately after notifying DSP.  The waiting is done while
relasing the lock so that other ops can go ahead.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
CC: stable@vger.kernel.org
---
 include/sound/compress_driver.h |   12 ++++++++++
 sound/core/compress_offload.c   |   43 +++++++++++++++++++++++++++++++++++---
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 9031a26..eff5f84 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -48,6 +48,8 @@ struct snd_compr_ops;
  *	the ring buffer
  * @total_bytes_transferred: cumulative bytes transferred by offload DSP
  * @sleep: poll sleep
+ * @wait: drain wait queuq
+ * @draining: draining wait condition
  */
 struct snd_compr_runtime {
 	snd_pcm_state_t state;
@@ -59,6 +61,8 @@ struct snd_compr_runtime {
 	u64 total_bytes_available;
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
+	wait_queue_head_t wait;
+	unsigned int draining;
 	void *private_data;
 };
 
@@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
+{
+	snd_BUG_ON(!stream);
+
+	stream->runtime->draining = 1;
+	wake_up(&stream->runtime->wait);
+}
+
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index bea523a..3582f9f 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
 	}
 	runtime->state = SNDRV_PCM_STATE_OPEN;
 	init_waitqueue_head(&runtime->sleep);
+	init_waitqueue_head(&runtime->wait);
 	data->stream.runtime = runtime;
 	f->private_data = (void *)data;
 	mutex_lock(&compr->lock);
@@ -682,12 +683,36 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 		wake_up(&stream->runtime->sleep);
+		stream->runtime->draining = 1;
+		wake_up(&stream->runtime->wait);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
 	}
 	return retval;
 }
 
+static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
+{
+	int retval = 0;
+
+	/*
+	 * we are called with lock held. So drop the lock while we wait for
+	 * drain complete notfication from the driver
+	 */
+	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	mutex_unlock(&stream->device->lock);
+
+	retval = wait_event_interruptible(stream->runtime->wait, stream->runtime->draining);
+	if (retval)
+		pr_err("Wait for drain failed %d\n", retval);
+
+	wake_up(&stream->runtime->sleep);
+	mutex_lock(&stream->device->lock);
+	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+
+	return retval;
+}
+
 static int snd_compr_drain(struct snd_compr_stream *stream)
 {
 	int retval;
@@ -695,12 +720,16 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
 			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
 		return -EPERM;
+
+	stream->runtime->draining = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
-	if (!retval) {
-		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	if (retval) {
+		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
 		wake_up(&stream->runtime->sleep);
+		return retval;
 	}
-	return retval;
+
+	return snd_compress_wait_for_drain(stream);
 }
 
 static int snd_compr_next_track(struct snd_compr_stream *stream)
@@ -735,10 +764,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	if (stream->next_track == false)
 		return -EPERM;
 
+	stream->runtime->draining = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+	if (retval) {
+		pr_err("Partial drain returned failure\n");
+		wake_up(&stream->runtime->sleep);
+		return retval;
+	}
 
 	stream->next_track = false;
-	return retval;
+	return snd_compress_wait_for_drain(stream);
 }
 
 static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
-- 
1.7.0.4

[PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

The drain and drain_notify callback were blocked by low level driver untill the
draining was complete. Due to this being invoked with big fat mutex held, others
ops like reading timestamp, calling pause, drop were blocked.

So to fix this we add a new snd_compr_drain_notify() API. This would be required
to be invoked by low level driver when drain or partial drain has been completed
by the DSP. Thus we make the drain and partial_drain callback as non blocking
and driver returns immediately after notifying DSP.
The waiting is done while relasing the lock so that other ops can go ahead.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
CC: stable@vger.kernel.org
---
v2:
 fix the 80 line warn
 move the state change to compress_drain()

 include/sound/compress_driver.h |   12 ++++++++++
 sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 9031a26..eff5f84 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -48,6 +48,8 @@ struct snd_compr_ops;
  *	the ring buffer
  * @total_bytes_transferred: cumulative bytes transferred by offload DSP
  * @sleep: poll sleep
+ * @wait: drain wait queuq
+ * @draining: draining wait condition
  */
 struct snd_compr_runtime {
 	snd_pcm_state_t state;
@@ -59,6 +61,8 @@ struct snd_compr_runtime {
 	u64 total_bytes_available;
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
+	wait_queue_head_t wait;
+	unsigned int draining;
 	void *private_data;
 };
 
@@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
+{
+	snd_BUG_ON(!stream);
+
+	stream->runtime->draining = 1;
+	wake_up(&stream->runtime->wait);
+}
+
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index bea523a..c030365 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
 	}
 	runtime->state = SNDRV_PCM_STATE_OPEN;
 	init_waitqueue_head(&runtime->sleep);
+	init_waitqueue_head(&runtime->wait);
 	data->stream.runtime = runtime;
 	f->private_data = (void *)data;
 	mutex_lock(&compr->lock);
@@ -682,12 +683,36 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 		wake_up(&stream->runtime->sleep);
+		stream->runtime->draining = 1;
+		wake_up(&stream->runtime->wait);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
 	}
 	return retval;
 }
 
+static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
+{
+	int retval = 0;
+
+	/*
+	 * we are called with lock held. So drop the lock while we wait for
+	 * drain complete notfication from the driver
+	 */
+	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	mutex_unlock(&stream->device->lock);
+
+	retval = wait_event_interruptible(stream->runtime->wait,
+			stream->runtime->draining);
+	if (retval)
+		pr_err("Wait for drain failed %d\n", retval);
+
+	wake_up(&stream->runtime->sleep);
+	mutex_lock(&stream->device->lock);
+
+	return retval;
+}
+
 static int snd_compr_drain(struct snd_compr_stream *stream)
 {
 	int retval;
@@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
 			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
 		return -EPERM;
+
+	stream->runtime->draining = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
-	if (!retval) {
-		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	if (retval) {
+		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
 		wake_up(&stream->runtime->sleep);
+		return retval;
 	}
+
+	retval = snd_compress_wait_for_drain(stream);
+	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 	return retval;
 }
 
@@ -735,10 +766,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	if (stream->next_track == false)
 		return -EPERM;
 
+	stream->runtime->draining = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+	if (retval) {
+		pr_err("Partial drain returned failure\n");
+		wake_up(&stream->runtime->sleep);
+		return retval;
+	}
 
 	stream->next_track = false;
-	return retval;
+	return snd_compress_wait_for_drain(stream);
 }
 
 static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
-- 
1.7.0.4

Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Thu, 24 Oct 2013 13:05:41 +0530,
Vinod Koul wrote:
> 
> The drain and drain_notify callback were blocked by low level driver untill the
> draining was complete. Due to this being invoked with big fat mutex held, others
> ops like reading timestamp, calling pause, drop were blocked.
> 
> So to fix this we add a new snd_compr_drain_notify() API. This would be required
> to be invoked by low level driver when drain or partial drain has been completed
> by the DSP. Thus we make the drain and partial_drain callback as non blocking
> and driver returns immediately after notifying DSP.
> The waiting is done while relasing the lock so that other ops can go ahead.
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> CC: stable@vger.kernel.org
> ---
> v2:
>  fix the 80 line warn
>  move the state change to compress_drain()
> 
>  include/sound/compress_driver.h |   12 ++++++++++
>  sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 9031a26..eff5f84 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -48,6 +48,8 @@ struct snd_compr_ops;
>   *	the ring buffer
>   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>   * @sleep: poll sleep
> + * @wait: drain wait queuq
> + * @draining: draining wait condition

What does this mean exactly?  I thought draining = 1 meaning that the
stream is draining (i.e. waiting for the finish notification), but
it looks opposite in your code below...


>   */
>  struct snd_compr_runtime {
>  	snd_pcm_state_t state;
> @@ -59,6 +61,8 @@ struct snd_compr_runtime {
>  	u64 total_bytes_available;
>  	u64 total_bytes_transferred;
>  	wait_queue_head_t sleep;
> +	wait_queue_head_t wait;
> +	unsigned int draining;
>  	void *private_data;
>  };
>  
> @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
>  	wake_up(&stream->runtime->sleep);
>  }
>  
> +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> +{
> +	snd_BUG_ON(!stream);
> +
> +	stream->runtime->draining = 1;
> +	wake_up(&stream->runtime->wait);
> +}
> +
>  #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index bea523a..c030365 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
>  	}
>  	runtime->state = SNDRV_PCM_STATE_OPEN;
>  	init_waitqueue_head(&runtime->sleep);
> +	init_waitqueue_head(&runtime->wait);
>  	data->stream.runtime = runtime;
>  	f->private_data = (void *)data;
>  	mutex_lock(&compr->lock);
> @@ -682,12 +683,36 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>  	if (!retval) {
>  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
>  		wake_up(&stream->runtime->sleep);
> +		stream->runtime->draining = 1;
> +		wake_up(&stream->runtime->wait);

Not using snd_compr_drain_notify()?


>  		stream->runtime->total_bytes_available = 0;
>  		stream->runtime->total_bytes_transferred = 0;
>  	}
>  	return retval;
>  }
>  
> +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> +{
> +	int retval = 0;
> +
> +	/*
> +	 * we are called with lock held. So drop the lock while we wait for
> +	 * drain complete notfication from the driver
> +	 */
> +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +	mutex_unlock(&stream->device->lock);
> +
> +	retval = wait_event_interruptible(stream->runtime->wait,
> +			stream->runtime->draining);
> +	if (retval)
> +		pr_err("Wait for drain failed %d\n", retval);

If you make it interruptible, it's no actual kernel error when user
interrupts the operation, so no reason to put the kernel error message
there.  At least, better to check whether it's a user interruption or
a system error.


> +
> +	wake_up(&stream->runtime->sleep);
> +	mutex_lock(&stream->device->lock);
> +
> +	return retval;
> +}
> +
>  static int snd_compr_drain(struct snd_compr_stream *stream)
>  {
>  	int retval;
> @@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
>  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
>  		return -EPERM;
> +
> +	stream->runtime->draining = 0;
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> -	if (!retval) {
> -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +	if (retval) {
> +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
>  		wake_up(&stream->runtime->sleep);
> +		return retval;
>  	}
> +
> +	retval = snd_compress_wait_for_drain(stream);
> +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;

So, the runtime state goes to SETUP even if wait_for_drain() returns
an error?  In other words, compress core expects that the driver takes
care of the proper state change in the error handling?


thanks,

Takashi

>  	return retval;
>  }
>  
> @@ -735,10 +766,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	if (stream->next_track == false)
>  		return -EPERM;
>  
> +	stream->runtime->draining = 0;
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> +	if (retval) {
> +		pr_err("Partial drain returned failure\n");
> +		wake_up(&stream->runtime->sleep);
> +		return retval;
> +	}
>  
>  	stream->next_track = false;
> -	return retval;
> +	return snd_compress_wait_for_drain(stream);
>  }
>  
>  static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> -- 
> 1.7.0.4
> 

Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Thu, Oct 24, 2013 at 11:25:35AM +0200, Takashi Iwai wrote:
> At Thu, 24 Oct 2013 13:05:41 +0530,
> Vinod Koul wrote:
> > 
> > The drain and drain_notify callback were blocked by low level driver untill the
> > draining was complete. Due to this being invoked with big fat mutex held, others
> > ops like reading timestamp, calling pause, drop were blocked.
> > 
> > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > to be invoked by low level driver when drain or partial drain has been completed
> > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > and driver returns immediately after notifying DSP.
> > The waiting is done while relasing the lock so that other ops can go ahead.
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > CC: stable@vger.kernel.org
> > ---
> > v2:
> >  fix the 80 line warn
> >  move the state change to compress_drain()
> > 
> >  include/sound/compress_driver.h |   12 ++++++++++
> >  sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 52 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > index 9031a26..eff5f84 100644
> > --- a/include/sound/compress_driver.h
> > +++ b/include/sound/compress_driver.h
> > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> >   *	the ring buffer
> >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> >   * @sleep: poll sleep
> > + * @wait: drain wait queuq
> > + * @draining: draining wait condition
> 
> What does this mean exactly?  I thought draining = 1 meaning that the
> stream is draining (i.e. waiting for the finish notification), but
> it looks opposite in your code below...
What i meant was the draining wait conditions which would be used for waiting.
Before wait the caller will set to 0 and on completion the callback will set to
1 and wake up.

> >   */
> >  struct snd_compr_runtime {
> >  	snd_pcm_state_t state;
> > @@ -59,6 +61,8 @@ struct snd_compr_runtime {
> >  	u64 total_bytes_available;
> >  	u64 total_bytes_transferred;
> >  	wait_queue_head_t sleep;
> > +	wait_queue_head_t wait;
> > +	unsigned int draining;
> >  	void *private_data;
> >  };
> >  
> > @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
> >  	wake_up(&stream->runtime->sleep);
> >  }
> >  
> > +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> > +{
> > +	snd_BUG_ON(!stream);
> > +
> > +	stream->runtime->draining = 1;
> > +	wake_up(&stream->runtime->wait);
> > +}
> > +
> >  #endif
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index bea523a..c030365 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
> >  	}
> >  	runtime->state = SNDRV_PCM_STATE_OPEN;
> >  	init_waitqueue_head(&runtime->sleep);
> > +	init_waitqueue_head(&runtime->wait);
> >  	data->stream.runtime = runtime;
> >  	f->private_data = (void *)data;
> >  	mutex_lock(&compr->lock);
> > @@ -682,12 +683,36 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> >  	if (!retval) {
> >  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> >  		wake_up(&stream->runtime->sleep);
> > +		stream->runtime->draining = 1;
> > +		wake_up(&stream->runtime->wait);
> 
> Not using snd_compr_drain_notify()?
Yes that should be added, thanks for pointing!

> >  		stream->runtime->total_bytes_available = 0;
> >  		stream->runtime->total_bytes_transferred = 0;
> >  	}
> >  	return retval;
> >  }
> >  
> > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > +{
> > +	int retval = 0;
> > +
> > +	/*
> > +	 * we are called with lock held. So drop the lock while we wait for
> > +	 * drain complete notfication from the driver
> > +	 */
> > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > +	mutex_unlock(&stream->device->lock);
> > +
> > +	retval = wait_event_interruptible(stream->runtime->wait,
> > +			stream->runtime->draining);
> > +	if (retval)
> > +		pr_err("Wait for drain failed %d\n", retval);
> 
> If you make it interruptible, it's no actual kernel error when user
> interrupts the operation, so no reason to put the kernel error message
> there.  At least, better to check whether it's a user interruption or
> a system error.
Agreed. I think I dont even need to make it interruptible. If user is
terminating the stop will be invoked and then with above fix it should work. We
can do away with interrutiple part :)

> > +
> > +	wake_up(&stream->runtime->sleep);
> > +	mutex_lock(&stream->device->lock);
> > +
> > +	return retval;
> > +}
> > +
> >  static int snd_compr_drain(struct snd_compr_stream *stream)
> >  {
> >  	int retval;
> > @@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> >  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> >  		return -EPERM;
> > +
> > +	stream->runtime->draining = 0;
> >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > -	if (!retval) {
> > -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > +	if (retval) {
> > +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> >  		wake_up(&stream->runtime->sleep);
> > +		return retval;
> >  	}
> > +
> > +	retval = snd_compress_wait_for_drain(stream);
> > +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> 
> So, the runtime state goes to SETUP even if wait_for_drain() returns
> an error?  In other words, compress core expects that the driver takes
> care of the proper state change in the error handling?
I guess it would make sense to communicate error. I havent added notion of
status on drain complete. By adding another status flag in the callback we cna
let driver tell us if error occured. Then pass the error in draining value and
do the state change accordingly.

--
~Vinod

Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Thu, 24 Oct 2013 14:57:24 +0530,
Vinod Koul wrote:
> 
> On Thu, Oct 24, 2013 at 11:25:35AM +0200, Takashi Iwai wrote:
> > At Thu, 24 Oct 2013 13:05:41 +0530,
> > Vinod Koul wrote:
> > > 
> > > The drain and drain_notify callback were blocked by low level driver untill the
> > > draining was complete. Due to this being invoked with big fat mutex held, others
> > > ops like reading timestamp, calling pause, drop were blocked.
> > > 
> > > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > > to be invoked by low level driver when drain or partial drain has been completed
> > > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > > and driver returns immediately after notifying DSP.
> > > The waiting is done while relasing the lock so that other ops can go ahead.
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > CC: stable@vger.kernel.org
> > > ---
> > > v2:
> > >  fix the 80 line warn
> > >  move the state change to compress_drain()
> > > 
> > >  include/sound/compress_driver.h |   12 ++++++++++
> > >  sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 52 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > > index 9031a26..eff5f84 100644
> > > --- a/include/sound/compress_driver.h
> > > +++ b/include/sound/compress_driver.h
> > > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> > >   *	the ring buffer
> > >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> > >   * @sleep: poll sleep
> > > + * @wait: drain wait queuq
> > > + * @draining: draining wait condition
> > 
> > What does this mean exactly?  I thought draining = 1 meaning that the
> > stream is draining (i.e. waiting for the finish notification), but
> > it looks opposite in your code below...
> What i meant was the draining wait conditions which would be used for waiting.
> Before wait the caller will set to 0 and on completion the callback will set to
> 1 and wake up.

Well, it's not so intuitive as the flag being named "draining".
Please give a more detailed comment (or name it a bit less-confusing
one :)

> > >   */
> > >  struct snd_compr_runtime {
> > >  	snd_pcm_state_t state;
> > > @@ -59,6 +61,8 @@ struct snd_compr_runtime {
> > >  	u64 total_bytes_available;
> > >  	u64 total_bytes_transferred;
> > >  	wait_queue_head_t sleep;
> > > +	wait_queue_head_t wait;
> > > +	unsigned int draining;
> > >  	void *private_data;
> > >  };
> > >  
> > > @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
> > >  	wake_up(&stream->runtime->sleep);
> > >  }
> > >  
> > > +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> > > +{
> > > +	snd_BUG_ON(!stream);
> > > +
> > > +	stream->runtime->draining = 1;
> > > +	wake_up(&stream->runtime->wait);
> > > +}
> > > +
> > >  #endif
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index bea523a..c030365 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
> > >  	}
> > >  	runtime->state = SNDRV_PCM_STATE_OPEN;
> > >  	init_waitqueue_head(&runtime->sleep);
> > > +	init_waitqueue_head(&runtime->wait);
> > >  	data->stream.runtime = runtime;
> > >  	f->private_data = (void *)data;
> > >  	mutex_lock(&compr->lock);
> > > @@ -682,12 +683,36 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > >  	if (!retval) {
> > >  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > >  		wake_up(&stream->runtime->sleep);
> > > +		stream->runtime->draining = 1;
> > > +		wake_up(&stream->runtime->wait);
> > 
> > Not using snd_compr_drain_notify()?
> Yes that should be added, thanks for pointing!
> 
> > >  		stream->runtime->total_bytes_available = 0;
> > >  		stream->runtime->total_bytes_transferred = 0;
> > >  	}
> > >  	return retval;
> > >  }
> > >  
> > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > +{
> > > +	int retval = 0;
> > > +
> > > +	/*
> > > +	 * we are called with lock held. So drop the lock while we wait for
> > > +	 * drain complete notfication from the driver
> > > +	 */
> > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > +	mutex_unlock(&stream->device->lock);
> > > +
> > > +	retval = wait_event_interruptible(stream->runtime->wait,
> > > +			stream->runtime->draining);
> > > +	if (retval)
> > > +		pr_err("Wait for drain failed %d\n", retval);
> > 
> > If you make it interruptible, it's no actual kernel error when user
> > interrupts the operation, so no reason to put the kernel error message
> > there.  At least, better to check whether it's a user interruption or
> > a system error.
> Agreed. I think I dont even need to make it interruptible. If user is
> terminating the stop will be invoked and then with above fix it should work. We
> can do away with interrutiple part :)
> 
> > > +
> > > +	wake_up(&stream->runtime->sleep);
> > > +	mutex_lock(&stream->device->lock);
> > > +
> > > +	return retval;
> > > +}
> > > +
> > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > >  {
> > >  	int retval;
> > > @@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > >  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > >  		return -EPERM;
> > > +
> > > +	stream->runtime->draining = 0;
> > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > -	if (!retval) {
> > > -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > +	if (retval) {
> > > +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> > >  		wake_up(&stream->runtime->sleep);
> > > +		return retval;
> > >  	}
> > > +
> > > +	retval = snd_compress_wait_for_drain(stream);
> > > +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > 
> > So, the runtime state goes to SETUP even if wait_for_drain() returns
> > an error?  In other words, compress core expects that the driver takes
> > care of the proper state change in the error handling?
> I guess it would make sense to communicate error. I havent added notion of
> status on drain complete. By adding another status flag in the callback we cna
> let driver tell us if error occured. Then pass the error in draining value and
> do the state change accordingly.

I think it's fine as in the current patch, I just wanted to make sure
that this runtime state change is the designed behavior.  If so, it'd
be better to document / comment clearly.


thanks,

Takashi

Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Thu, Oct 24, 2013 at 12:59:18PM +0200, Takashi Iwai wrote:
> At Thu, 24 Oct 2013 14:57:24 +0530,
> Vinod Koul wrote:
> > 
> > On Thu, Oct 24, 2013 at 11:25:35AM +0200, Takashi Iwai wrote:
> > > At Thu, 24 Oct 2013 13:05:41 +0530,
> > > Vinod Koul wrote:
> > > > 
> > > > The drain and drain_notify callback were blocked by low level driver untill the
> > > > draining was complete. Due to this being invoked with big fat mutex held, others
> > > > ops like reading timestamp, calling pause, drop were blocked.
> > > > 
> > > > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > > > to be invoked by low level driver when drain or partial drain has been completed
> > > > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > > > and driver returns immediately after notifying DSP.
> > > > The waiting is done while relasing the lock so that other ops can go ahead.
> > > > 
> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > CC: stable@vger.kernel.org
> > > > ---
> > > > v2:
> > > >  fix the 80 line warn
> > > >  move the state change to compress_drain()
> > > > 
> > > >  include/sound/compress_driver.h |   12 ++++++++++
> > > >  sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
> > > >  2 files changed, 52 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > > > index 9031a26..eff5f84 100644
> > > > --- a/include/sound/compress_driver.h
> > > > +++ b/include/sound/compress_driver.h
> > > > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> > > >   *	the ring buffer
> > > >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> > > >   * @sleep: poll sleep
> > > > + * @wait: drain wait queuq
> > > > + * @draining: draining wait condition
> > > 
> > > What does this mean exactly?  I thought draining = 1 meaning that the
> > > stream is draining (i.e. waiting for the finish notification), but
> > > it looks opposite in your code below...
> > What i meant was the draining wait conditions which would be used for waiting.
> > Before wait the caller will set to 0 and on completion the callback will set to
> > 1 and wake up.
> 
> Well, it's not so intuitive as the flag being named "draining".
> Please give a more detailed comment (or name it a bit less-confusing
> one :)
hmmm, yes it needs a better name :)
perhaps drain_condition or drain_wake would be apt!
 
> > > > +
> > > > +	wake_up(&stream->runtime->sleep);
> > > > +	mutex_lock(&stream->device->lock);
> > > > +
> > > > +	return retval;
> > > > +}
> > > > +
> > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > >  {
> > > >  	int retval;
> > > > @@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > >  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > >  		return -EPERM;
> > > > +
> > > > +	stream->runtime->draining = 0;
> > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > > -	if (!retval) {
> > > > -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > +	if (retval) {
> > > > +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> > > >  		wake_up(&stream->runtime->sleep);
> > > > +		return retval;
> > > >  	}
> > > > +
> > > > +	retval = snd_compress_wait_for_drain(stream);
> > > > +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > > 
> > > So, the runtime state goes to SETUP even if wait_for_drain() returns
> > > an error?  In other words, compress core expects that the driver takes
> > > care of the proper state change in the error handling?
> > I guess it would make sense to communicate error. I havent added notion of
> > status on drain complete. By adding another status flag in the callback we cna
> > let driver tell us if error occured. Then pass the error in draining value and
> > do the state change accordingly.
> 
> I think it's fine as in the current patch, I just wanted to make sure
> that this runtime state change is the designed behavior.  If so, it'd
> be better to document / comment clearly.
Sure, lets add that explictly!

-- 
~Vinod

Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Thu, 24 Oct 2013 16:05:05 +0530,
Vinod Koul wrote:
> 
> On Thu, Oct 24, 2013 at 12:59:18PM +0200, Takashi Iwai wrote:
> > At Thu, 24 Oct 2013 14:57:24 +0530,
> > Vinod Koul wrote:
> > > 
> > > On Thu, Oct 24, 2013 at 11:25:35AM +0200, Takashi Iwai wrote:
> > > > At Thu, 24 Oct 2013 13:05:41 +0530,
> > > > Vinod Koul wrote:
> > > > > 
> > > > > The drain and drain_notify callback were blocked by low level driver untill the
> > > > > draining was complete. Due to this being invoked with big fat mutex held, others
> > > > > ops like reading timestamp, calling pause, drop were blocked.
> > > > > 
> > > > > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > > > > to be invoked by low level driver when drain or partial drain has been completed
> > > > > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > > > > and driver returns immediately after notifying DSP.
> > > > > The waiting is done while relasing the lock so that other ops can go ahead.
> > > > > 
> > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > > CC: stable@vger.kernel.org
> > > > > ---
> > > > > v2:
> > > > >  fix the 80 line warn
> > > > >  move the state change to compress_drain()
> > > > > 
> > > > >  include/sound/compress_driver.h |   12 ++++++++++
> > > > >  sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
> > > > >  2 files changed, 52 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > > > > index 9031a26..eff5f84 100644
> > > > > --- a/include/sound/compress_driver.h
> > > > > +++ b/include/sound/compress_driver.h
> > > > > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> > > > >   *	the ring buffer
> > > > >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> > > > >   * @sleep: poll sleep
> > > > > + * @wait: drain wait queuq
> > > > > + * @draining: draining wait condition
> > > > 
> > > > What does this mean exactly?  I thought draining = 1 meaning that the
> > > > stream is draining (i.e. waiting for the finish notification), but
> > > > it looks opposite in your code below...
> > > What i meant was the draining wait conditions which would be used for waiting.
> > > Before wait the caller will set to 0 and on completion the callback will set to
> > > 1 and wake up.
> > 
> > Well, it's not so intuitive as the flag being named "draining".
> > Please give a more detailed comment (or name it a bit less-confusing
> > one :)
> hmmm, yes it needs a better name :)
> perhaps drain_condition or drain_wake would be apt!

Yes, drain_wake would be more self-explanatory.

> > > > > +
> > > > > +	wake_up(&stream->runtime->sleep);
> > > > > +	mutex_lock(&stream->device->lock);
> > > > > +
> > > > > +	return retval;
> > > > > +}
> > > > > +
> > > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > > >  {
> > > > >  	int retval;
> > > > > @@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> > > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > > >  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > > >  		return -EPERM;
> > > > > +
> > > > > +	stream->runtime->draining = 0;
> > > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > > > -	if (!retval) {
> > > > > -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > > +	if (retval) {
> > > > > +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> > > > >  		wake_up(&stream->runtime->sleep);
> > > > > +		return retval;
> > > > >  	}
> > > > > +
> > > > > +	retval = snd_compress_wait_for_drain(stream);
> > > > > +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > > > 
> > > > So, the runtime state goes to SETUP even if wait_for_drain() returns
> > > > an error?  In other words, compress core expects that the driver takes
> > > > care of the proper state change in the error handling?
> > > I guess it would make sense to communicate error. I havent added notion of
> > > status on drain complete. By adding another status flag in the callback we cna
> > > let driver tell us if error occured. Then pass the error in draining value and
> > > do the state change accordingly.
> > 
> > I think it's fine as in the current patch, I just wanted to make sure
> > that this runtime state change is the designed behavior.  If so, it'd
> > be better to document / comment clearly.
> Sure, lets add that explictly!

OK, thanks.


Takashi

Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Thu, Oct 24, 2013 at 02:00:39PM +0200, Takashi Iwai wrote:
> > hmmm, yes it needs a better name :)
> > perhaps drain_condition or drain_wake would be apt!
> 
> Yes, drain_wake would be more self-explanatory.
Ok sent v3 with this a few moment ago :)

--
~Vinod

[PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

The drain and drain_notify callback were blocked by low level driver untill the
draining was complete. Due to this being invoked with big fat mutex held, others
ops like reading timestamp, calling pause, drop were blocked.

So to fix this we add a new snd_compr_drain_notify() API. This would be required
to be invoked by low level driver when drain or partial drain has been completed
by the DSP. Thus we make the drain and partial_drain callback as non blocking
and driver returns immediately after notifying DSP.
The waiting is done while relasing the lock so that other ops can go ahead.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
CC: stable@vger.kernel.org
---
v3:
 call snd_compr_drain_notify from compress_stop()
 rename draining -> drain_wake
 add some comments on state transistion after drain

v2:
 fix the 80 line warn
 move the state change to compress_drain()


 include/sound/compress_driver.h |   12 +++++++++++
 sound/core/compress_offload.c   |   41 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 9031a26..175ab32 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -48,6 +48,8 @@ struct snd_compr_ops;
  *	the ring buffer
  * @total_bytes_transferred: cumulative bytes transferred by offload DSP
  * @sleep: poll sleep
+ * @wait: drain wait queue
+ * @drain_wake: condition for drain wake
  */
 struct snd_compr_runtime {
 	snd_pcm_state_t state;
@@ -59,6 +61,8 @@ struct snd_compr_runtime {
 	u64 total_bytes_available;
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
+	wait_queue_head_t wait;
+	unsigned int drain_wake;
 	void *private_data;
 };
 
@@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
+{
+	snd_BUG_ON(!stream);
+
+	stream->runtime->drain_wake = 1;
+	wake_up(&stream->runtime->wait);
+}
+
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index bea523a..3eb47d0 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
 	}
 	runtime->state = SNDRV_PCM_STATE_OPEN;
 	init_waitqueue_head(&runtime->sleep);
+	init_waitqueue_head(&runtime->wait);
 	data->stream.runtime = runtime;
 	f->private_data = (void *)data;
 	mutex_lock(&compr->lock);
@@ -682,12 +683,34 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 		wake_up(&stream->runtime->sleep);
+		snd_compr_drain_notify(stream);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
 	}
 	return retval;
 }
 
+static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
+{
+	/*
+	 * We are called with lock held. So drop the lock while we wait for
+	 * drain complete notfication from the driver
+	 *
+	 * It is expected that driver will notify the drain completion and then
+	 * stream will be moved to SETUP state, even if draining resulted in an
+	 * error. We can trigger next track after this.
+	 */
+	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	mutex_unlock(&stream->device->lock);
+
+	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
+
+	wake_up(&stream->runtime->sleep);
+	mutex_lock(&stream->device->lock);
+
+	return 0;
+}
+
 static int snd_compr_drain(struct snd_compr_stream *stream)
 {
 	int retval;
@@ -695,11 +718,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
 			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
 		return -EPERM;
+
+	stream->runtime->drain_wake = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
-	if (!retval) {
-		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	if (retval) {
+		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
 		wake_up(&stream->runtime->sleep);
+		return retval;
 	}
+
+	retval = snd_compress_wait_for_drain(stream);
+	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 	return retval;
 }
 
@@ -735,10 +764,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	if (stream->next_track == false)
 		return -EPERM;
 
+	stream->runtime->drain_wake = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+	if (retval) {
+		pr_err("Partial drain returned failure\n");
+		wake_up(&stream->runtime->sleep);
+		return retval;
+	}
 
 	stream->next_track = false;
-	return retval;
+	return snd_compress_wait_for_drain(stream);
 }
 
 static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
-- 
1.7.0.4

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Thu, 24 Oct 2013 16:37:31 +0530,
Vinod Koul wrote:
> 
> The drain and drain_notify callback were blocked by low level driver untill the
> draining was complete. Due to this being invoked with big fat mutex held, others
> ops like reading timestamp, calling pause, drop were blocked.
> 
> So to fix this we add a new snd_compr_drain_notify() API. This would be required
> to be invoked by low level driver when drain or partial drain has been completed
> by the DSP. Thus we make the drain and partial_drain callback as non blocking
> and driver returns immediately after notifying DSP.
> The waiting is done while relasing the lock so that other ops can go ahead.
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> CC: stable@vger.kernel.org
> ---
> v3:
>  call snd_compr_drain_notify from compress_stop()
>  rename draining -> drain_wake
>  add some comments on state transistion after drain
> 
> v2:
>  fix the 80 line warn
>  move the state change to compress_drain()
> 
> 
>  include/sound/compress_driver.h |   12 +++++++++++
>  sound/core/compress_offload.c   |   41 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 9031a26..175ab32 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -48,6 +48,8 @@ struct snd_compr_ops;
>   *	the ring buffer
>   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>   * @sleep: poll sleep
> + * @wait: drain wait queue
> + * @drain_wake: condition for drain wake
>   */
>  struct snd_compr_runtime {
>  	snd_pcm_state_t state;
> @@ -59,6 +61,8 @@ struct snd_compr_runtime {
>  	u64 total_bytes_available;
>  	u64 total_bytes_transferred;
>  	wait_queue_head_t sleep;
> +	wait_queue_head_t wait;
> +	unsigned int drain_wake;
>  	void *private_data;
>  };
>  
> @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
>  	wake_up(&stream->runtime->sleep);
>  }
>  
> +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> +{
> +	snd_BUG_ON(!stream);

Should return, otherwise you'll get Oops:

	if (snd_BUG_ON(!stream))
		return;

> +
> +	stream->runtime->drain_wake = 1;
> +	wake_up(&stream->runtime->wait);
> +}
> +
>  #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index bea523a..3eb47d0 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
>  	}
>  	runtime->state = SNDRV_PCM_STATE_OPEN;
>  	init_waitqueue_head(&runtime->sleep);
> +	init_waitqueue_head(&runtime->wait);
>  	data->stream.runtime = runtime;
>  	f->private_data = (void *)data;
>  	mutex_lock(&compr->lock);
> @@ -682,12 +683,34 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>  	if (!retval) {
>  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
>  		wake_up(&stream->runtime->sleep);
> +		snd_compr_drain_notify(stream);
>  		stream->runtime->total_bytes_available = 0;
>  		stream->runtime->total_bytes_transferred = 0;
>  	}
>  	return retval;
>  }
>  
> +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> +{
> +	/*
> +	 * We are called with lock held. So drop the lock while we wait for
> +	 * drain complete notfication from the driver
> +	 *
> +	 * It is expected that driver will notify the drain completion and then
> +	 * stream will be moved to SETUP state, even if draining resulted in an
> +	 * error. We can trigger next track after this.
> +	 */
> +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +	mutex_unlock(&stream->device->lock);
> +
> +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);

Don't you really want the interruption?
Or use wait_event_interruptible() and return error appropriately.

> +
> +	wake_up(&stream->runtime->sleep);
> +	mutex_lock(&stream->device->lock);
> +
> +	return 0;
> +}
> +
>  static int snd_compr_drain(struct snd_compr_stream *stream)
>  {
>  	int retval;
> @@ -695,11 +718,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
>  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
>  		return -EPERM;
> +
> +	stream->runtime->drain_wake = 0;
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> -	if (!retval) {
> -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +	if (retval) {
> +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);

I don't think spamming such an error is good.  The trigger error can
happen often repeatedly once when it happens.  Such a message is at
most a debug message, so use pr_debug(), if you still want to keep it
in the core code.

>  		wake_up(&stream->runtime->sleep);
> +		return retval;
>  	}
> +
> +	retval = snd_compress_wait_for_drain(stream);
> +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
>  	return retval;
>  }
>  
> @@ -735,10 +764,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	if (stream->next_track == false)
>  		return -EPERM;
>  
> +	stream->runtime->drain_wake = 0;
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> +	if (retval) {
> +		pr_err("Partial drain returned failure\n");

Ditto.

> +		wake_up(&stream->runtime->sleep);
> +		return retval;
> +	}
>  
>  	stream->next_track = false;
> -	return retval;
> +	return snd_compress_wait_for_drain(stream);
>  }
>  
>  static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> -- 
> 1.7.0.4
> 


Takashi

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Wed, Oct 30, 2013 at 03:47:45PM +0100, Takashi Iwai wrote:
> At Thu, 24 Oct 2013 16:37:31 +0530,
> Vinod Koul wrote:
> > 
> > The drain and drain_notify callback were blocked by low level driver untill the
> > draining was complete. Due to this being invoked with big fat mutex held, others
> > ops like reading timestamp, calling pause, drop were blocked.
> > 
> > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > to be invoked by low level driver when drain or partial drain has been completed
> > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > and driver returns immediately after notifying DSP.
> > The waiting is done while relasing the lock so that other ops can go ahead.
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > CC: stable@vger.kernel.org
> > ---
> > v3:
> >  call snd_compr_drain_notify from compress_stop()
> >  rename draining -> drain_wake
> >  add some comments on state transistion after drain
> > 
> > v2:
> >  fix the 80 line warn
> >  move the state change to compress_drain()
> > 
> > 
> >  include/sound/compress_driver.h |   12 +++++++++++
> >  sound/core/compress_offload.c   |   41 ++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 50 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > index 9031a26..175ab32 100644
> > --- a/include/sound/compress_driver.h
> > +++ b/include/sound/compress_driver.h
> > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> >   *	the ring buffer
> >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> >   * @sleep: poll sleep
> > + * @wait: drain wait queue
> > + * @drain_wake: condition for drain wake
> >   */
> >  struct snd_compr_runtime {
> >  	snd_pcm_state_t state;
> > @@ -59,6 +61,8 @@ struct snd_compr_runtime {
> >  	u64 total_bytes_available;
> >  	u64 total_bytes_transferred;
> >  	wait_queue_head_t sleep;
> > +	wait_queue_head_t wait;
> > +	unsigned int drain_wake;
> >  	void *private_data;
> >  };
> >  
> > @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
> >  	wake_up(&stream->runtime->sleep);
> >  }
> >  
> > +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> > +{
> > +	snd_BUG_ON(!stream);
> 
> Should return, otherwise you'll get Oops:
> 
> 	if (snd_BUG_ON(!stream))
> 		return;
ah, missed that!

> > +
> > +	stream->runtime->drain_wake = 1;
> > +	wake_up(&stream->runtime->wait);
> > +}
> > +
> >  #endif
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index bea523a..3eb47d0 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
> >  	}
> >  	runtime->state = SNDRV_PCM_STATE_OPEN;
> >  	init_waitqueue_head(&runtime->sleep);
> > +	init_waitqueue_head(&runtime->wait);
> >  	data->stream.runtime = runtime;
> >  	f->private_data = (void *)data;
> >  	mutex_lock(&compr->lock);
> > @@ -682,12 +683,34 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> >  	if (!retval) {
> >  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> >  		wake_up(&stream->runtime->sleep);
> > +		snd_compr_drain_notify(stream);
> >  		stream->runtime->total_bytes_available = 0;
> >  		stream->runtime->total_bytes_transferred = 0;
> >  	}
> >  	return retval;
> >  }
> >  
> > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > +{
> > +	/*
> > +	 * We are called with lock held. So drop the lock while we wait for
> > +	 * drain complete notfication from the driver
> > +	 *
> > +	 * It is expected that driver will notify the drain completion and then
> > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > +	 * error. We can trigger next track after this.
> > +	 */
> > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > +	mutex_unlock(&stream->device->lock);
> > +
> > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> 
> Don't you really want the interruption?
> Or use wait_event_interruptible() and return error appropriately.
any interruption from usermode should trigger the compress_stop/compress_free
which will unblock this. I dont see the need to have interruptible here

> > +
> > +	wake_up(&stream->runtime->sleep);
> > +	mutex_lock(&stream->device->lock);
> > +
> > +	return 0;
> > +}
> > +
> >  static int snd_compr_drain(struct snd_compr_stream *stream)
> >  {
> >  	int retval;
> > @@ -695,11 +718,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> >  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> >  		return -EPERM;
> > +
> > +	stream->runtime->drain_wake = 0;
> >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > -	if (!retval) {
> > -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > +	if (retval) {
> > +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> 
> I don't think spamming such an error is good.  The trigger error can
> happen often repeatedly once when it happens.  Such a message is at
> most a debug message, so use pr_debug(), if you still want to keep it
> in the core code.
Sure...

Thanks for the quick review, will send v4 before I hit the bed today!

--
~Vinod
> >  		wake_up(&stream->runtime->sleep);
> > +		return retval;
> >  	}
> > +
> > +	retval = snd_compress_wait_for_drain(stream);
> > +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> >  	return retval;
> >  }
> >  
> > @@ -735,10 +764,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> >  	if (stream->next_track == false)
> >  		return -EPERM;
> >  
> > +	stream->runtime->drain_wake = 0;
> >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> > +	if (retval) {
> > +		pr_err("Partial drain returned failure\n");
> 
> Ditto.
> 
> > +		wake_up(&stream->runtime->sleep);
> > +		return retval;
> > +	}
> >  
> >  	stream->next_track = false;
> > -	return retval;
> > +	return snd_compress_wait_for_drain(stream);
> >  }
> >  
> >  static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> > -- 
> > 1.7.0.4
> > 
> 
> 
> Takashi

-- 

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Wed, 30 Oct 2013 20:28:11 +0530,
Vinod Koul wrote:
> 
> On Wed, Oct 30, 2013 at 03:47:45PM +0100, Takashi Iwai wrote:
> > At Thu, 24 Oct 2013 16:37:31 +0530,
> > Vinod Koul wrote:
> > > 
> > > The drain and drain_notify callback were blocked by low level driver untill the
> > > draining was complete. Due to this being invoked with big fat mutex held, others
> > > ops like reading timestamp, calling pause, drop were blocked.
> > > 
> > > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > > to be invoked by low level driver when drain or partial drain has been completed
> > > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > > and driver returns immediately after notifying DSP.
> > > The waiting is done while relasing the lock so that other ops can go ahead.
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > CC: stable@vger.kernel.org
> > > ---
> > > v3:
> > >  call snd_compr_drain_notify from compress_stop()
> > >  rename draining -> drain_wake
> > >  add some comments on state transistion after drain
> > > 
> > > v2:
> > >  fix the 80 line warn
> > >  move the state change to compress_drain()
> > > 
> > > 
> > >  include/sound/compress_driver.h |   12 +++++++++++
> > >  sound/core/compress_offload.c   |   41 ++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 50 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > > index 9031a26..175ab32 100644
> > > --- a/include/sound/compress_driver.h
> > > +++ b/include/sound/compress_driver.h
> > > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> > >   *	the ring buffer
> > >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> > >   * @sleep: poll sleep
> > > + * @wait: drain wait queue
> > > + * @drain_wake: condition for drain wake
> > >   */
> > >  struct snd_compr_runtime {
> > >  	snd_pcm_state_t state;
> > > @@ -59,6 +61,8 @@ struct snd_compr_runtime {
> > >  	u64 total_bytes_available;
> > >  	u64 total_bytes_transferred;
> > >  	wait_queue_head_t sleep;
> > > +	wait_queue_head_t wait;
> > > +	unsigned int drain_wake;
> > >  	void *private_data;
> > >  };
> > >  
> > > @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
> > >  	wake_up(&stream->runtime->sleep);
> > >  }
> > >  
> > > +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> > > +{
> > > +	snd_BUG_ON(!stream);
> > 
> > Should return, otherwise you'll get Oops:
> > 
> > 	if (snd_BUG_ON(!stream))
> > 		return;
> ah, missed that!
> 
> > > +
> > > +	stream->runtime->drain_wake = 1;
> > > +	wake_up(&stream->runtime->wait);
> > > +}
> > > +
> > >  #endif
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index bea523a..3eb47d0 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
> > >  	}
> > >  	runtime->state = SNDRV_PCM_STATE_OPEN;
> > >  	init_waitqueue_head(&runtime->sleep);
> > > +	init_waitqueue_head(&runtime->wait);
> > >  	data->stream.runtime = runtime;
> > >  	f->private_data = (void *)data;
> > >  	mutex_lock(&compr->lock);
> > > @@ -682,12 +683,34 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > >  	if (!retval) {
> > >  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > >  		wake_up(&stream->runtime->sleep);
> > > +		snd_compr_drain_notify(stream);
> > >  		stream->runtime->total_bytes_available = 0;
> > >  		stream->runtime->total_bytes_transferred = 0;
> > >  	}
> > >  	return retval;
> > >  }
> > >  
> > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > +{
> > > +	/*
> > > +	 * We are called with lock held. So drop the lock while we wait for
> > > +	 * drain complete notfication from the driver
> > > +	 *
> > > +	 * It is expected that driver will notify the drain completion and then
> > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > +	 * error. We can trigger next track after this.
> > > +	 */
> > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > +	mutex_unlock(&stream->device->lock);
> > > +
> > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > 
> > Don't you really want the interruption?
> > Or use wait_event_interruptible() and return error appropriately.
> any interruption from usermode should trigger the compress_stop/compress_free

... but how?

> which will unblock this. I dont see the need to have interruptible here

Suppose you're running a single thread program.  Then who triggers the
stop?


Takashi

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Wed, Oct 30, 2013 at 05:01:38PM +0100, Takashi Iwai wrote:
 
> > > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > > +{
> > > > +	/*
> > > > +	 * We are called with lock held. So drop the lock while we wait for
> > > > +	 * drain complete notfication from the driver
> > > > +	 *
> > > > +	 * It is expected that driver will notify the drain completion and then
> > > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > > +	 * error. We can trigger next track after this.
> > > > +	 */
> > > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > +	mutex_unlock(&stream->device->lock);
> > > > +
> > > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > > 
> > > Don't you really want the interruption?
> > > Or use wait_event_interruptible() and return error appropriately.
> > any interruption from usermode should trigger the compress_stop/compress_free
> 
> ... but how?
> 
> > which will unblock this. I dont see the need to have interruptible here
> 
> Suppose you're running a single thread program.  Then who triggers the
> stop?
Your signal catcher which should be registered for signals intrested and then
calls stop or free. Assuming this should be considered single threaded. IIRC,
this is how you do in aplay, right?

Btw am okay to make it interruptible, but since can be done in usermode and
closed which unblocks this I would prefer this way. Pls do let me know :)
--
~Vinod

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Wed, 30 Oct 2013 20:48:40 +0530,
Vinod Koul wrote:
> 
> On Wed, Oct 30, 2013 at 05:01:38PM +0100, Takashi Iwai wrote:
>  
> > > > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > > > +{
> > > > > +	/*
> > > > > +	 * We are called with lock held. So drop the lock while we wait for
> > > > > +	 * drain complete notfication from the driver
> > > > > +	 *
> > > > > +	 * It is expected that driver will notify the drain completion and then
> > > > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > > > +	 * error. We can trigger next track after this.
> > > > > +	 */
> > > > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > > +	mutex_unlock(&stream->device->lock);
> > > > > +
> > > > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > > > 
> > > > Don't you really want the interruption?
> > > > Or use wait_event_interruptible() and return error appropriately.
> > > any interruption from usermode should trigger the compress_stop/compress_free
> > 
> > ... but how?
> > 
> > > which will unblock this. I dont see the need to have interruptible here
> > 
> > Suppose you're running a single thread program.  Then who triggers the
> > stop?
> Your signal catcher which should be registered for signals intrested and then
> calls stop or free. Assuming this should be considered single threaded. IIRC,
> this is how you do in aplay, right?

No, issuing an ioctl in a signal isn't guaranteed to work at all.
You shouldn't code the kernel driver relying on that userspace
behavior.


Takashi

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Wed, Oct 30, 2013 at 05:24:12PM +0100, Takashi Iwai wrote:
> At Wed, 30 Oct 2013 20:48:40 +0530,
> Vinod Koul wrote:
> > 
> > On Wed, Oct 30, 2013 at 05:01:38PM +0100, Takashi Iwai wrote:
> >  
> > > > > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > > > > +{
> > > > > > +	/*
> > > > > > +	 * We are called with lock held. So drop the lock while we wait for
> > > > > > +	 * drain complete notfication from the driver
> > > > > > +	 *
> > > > > > +	 * It is expected that driver will notify the drain completion and then
> > > > > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > > > > +	 * error. We can trigger next track after this.
> > > > > > +	 */
> > > > > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > > > +	mutex_unlock(&stream->device->lock);
> > > > > > +
> > > > > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > > > > 
> > > > > Don't you really want the interruption?
> > > > > Or use wait_event_interruptible() and return error appropriately.
> > > > any interruption from usermode should trigger the compress_stop/compress_free
> > > 
> > > ... but how?
> > > 
> > > > which will unblock this. I dont see the need to have interruptible here
> > > 
> > > Suppose you're running a single thread program.  Then who triggers the
> > > stop?
> > Your signal catcher which should be registered for signals intrested and then
> > calls stop or free. Assuming this should be considered single threaded. IIRC,
> > this is how you do in aplay, right?
> 
> No, issuing an ioctl in a signal isn't guaranteed to work at all.
Okay thanks for pointing out, do you know why we have that limitation. Btw
closing the fd should have same effect as free would trigger the stop internally

> You shouldn't code the kernel driver relying on that userspace
> behavior.
Ok, will update it

--
~Vinod

Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Wed, 30 Oct 2013 21:03:54 +0530,
Vinod Koul wrote:
> 
> On Wed, Oct 30, 2013 at 05:24:12PM +0100, Takashi Iwai wrote:
> > At Wed, 30 Oct 2013 20:48:40 +0530,
> > Vinod Koul wrote:
> > > 
> > > On Wed, Oct 30, 2013 at 05:01:38PM +0100, Takashi Iwai wrote:
> > >  
> > > > > > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > > > > > +{
> > > > > > > +	/*
> > > > > > > +	 * We are called with lock held. So drop the lock while we wait for
> > > > > > > +	 * drain complete notfication from the driver
> > > > > > > +	 *
> > > > > > > +	 * It is expected that driver will notify the drain completion and then
> > > > > > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > > > > > +	 * error. We can trigger next track after this.
> > > > > > > +	 */
> > > > > > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > > > > +	mutex_unlock(&stream->device->lock);
> > > > > > > +
> > > > > > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > > > > > 
> > > > > > Don't you really want the interruption?
> > > > > > Or use wait_event_interruptible() and return error appropriately.
> > > > > any interruption from usermode should trigger the compress_stop/compress_free
> > > > 
> > > > ... but how?
> > > > 
> > > > > which will unblock this. I dont see the need to have interruptible here
> > > > 
> > > > Suppose you're running a single thread program.  Then who triggers the
> > > > stop?
> > > Your signal catcher which should be registered for signals intrested and then
> > > calls stop or free. Assuming this should be considered single threaded. IIRC,
> > > this is how you do in aplay, right?
> > 
> > No, issuing an ioctl in a signal isn't guaranteed to work at all.
> Okay thanks for pointing out, do you know why we have that
> limitation.

It's a long-standing limitation of POSIX.
Basically a signal handler can accept only very few functions that are
asynchronous-safe.  That's the reason why signal is practically
useless in most use cases.

>  Btw
> closing the fd should have same effect as free would trigger the stop internally

Well, better to test it before jumping into the conclusion :)

> > You shouldn't code the kernel driver relying on that userspace
> > behavior.
> Ok, will update it

OK.


thanks,

Takashi

[PATCH v4] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

The drain and drain_notify callback were blocked by low level driver untill the
draining was complete. Due to this being invoked with big fat mutex held, others
ops like reading timestamp, calling pause, drop were blocked.

So to fix this we add a new snd_compr_drain_notify() API. This would be required
to be invoked by low level driver when drain or partial drain has been completed
by the DSP. Thus we make the drain and partial_drain callback as non blocking
and driver returns immediately after notifying DSP.
The waiting is done while relasing the lock so that other ops can go ahead.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
CC: stable@vger.kernel.org
---
v4:
 move pr_err -> pr_debug to avoid spamming kernel log
 make wait in drain interruptible
v3:
 call snd_compr_drain_notify from compress_stop()
 rename draining -> drain_wake
 add some comments on state transistion after drain
v2:
 fix the 80 line warn
 move the state change to compress_drain()


 include/sound/compress_driver.h |   13 +++++++++
 sound/core/compress_offload.c   |   55 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 9031a26..e723935 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -48,6 +48,8 @@ struct snd_compr_ops;
  *	the ring buffer
  * @total_bytes_transferred: cumulative bytes transferred by offload DSP
  * @sleep: poll sleep
+ * @wait: drain wait queue
+ * @drain_wake: condition for drain wake
  */
 struct snd_compr_runtime {
 	snd_pcm_state_t state;
@@ -59,6 +61,8 @@ struct snd_compr_runtime {
 	u64 total_bytes_available;
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
+	wait_queue_head_t wait;
+	unsigned int drain_wake;
 	void *private_data;
 };
 
@@ -171,4 +175,13 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
+{
+	if (snd_BUG_ON(!stream))
+		return;
+
+	stream->runtime->drain_wake = 1;
+	wake_up(&stream->runtime->wait);
+}
+
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index bea523a..645cec5 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
 	}
 	runtime->state = SNDRV_PCM_STATE_OPEN;
 	init_waitqueue_head(&runtime->sleep);
+	init_waitqueue_head(&runtime->wait);
 	data->stream.runtime = runtime;
 	f->private_data = (void *)data;
 	mutex_lock(&compr->lock);
@@ -682,12 +683,48 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 		wake_up(&stream->runtime->sleep);
+		snd_compr_drain_notify(stream);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
 	}
 	return retval;
 }
 
+static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
+{
+	int ret;
+
+	/*
+	 * We are called with lock held. So drop the lock while we wait for
+	 * drain complete notfication from the driver
+	 *
+	 * It is expected that driver will notify the drain completion and then
+	 * stream will be moved to SETUP state, even if draining resulted in an
+	 * error. We can trigger next track after this.
+	 */
+	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	mutex_unlock(&stream->device->lock);
+
+	/* we wait for drain to complete here, drain can return when
+	 * interruption occurred, wait returned error or success.
+	 * For the first two cases we don't do anything different here and
+	 * return after waking up
+	 */
+
+	ret = wait_event_interruptible(stream->runtime->wait,
+			stream->runtime->drain_wake);
+	if (ret == -ERESTARTSYS)
+		pr_debug("wait aborted by a signal");
+	else if (ret)
+		pr_debug("wait for drain failed with %d\n", ret);
+
+
+	wake_up(&stream->runtime->sleep);
+	mutex_lock(&stream->device->lock);
+
+	return ret;
+}
+
 static int snd_compr_drain(struct snd_compr_stream *stream)
 {
 	int retval;
@@ -695,11 +732,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
 			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
 		return -EPERM;
+
+	stream->runtime->drain_wake = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
-	if (!retval) {
-		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+	if (retval) {
+		pr_debug("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
 		wake_up(&stream->runtime->sleep);
+		return retval;
 	}
+
+	retval = snd_compress_wait_for_drain(stream);
+	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 	return retval;
 }
 
@@ -735,10 +778,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	if (stream->next_track == false)
 		return -EPERM;
 
+	stream->runtime->drain_wake = 0;
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+	if (retval) {
+		pr_debug("Partial drain returned failure\n");
+		wake_up(&stream->runtime->sleep);
+		return retval;
+	}
 
 	stream->next_track = false;
-	return retval;
+	return snd_compress_wait_for_drain(stream);
 }
 
 static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
-- 
1.7.0.4

Re: [PATCH v4] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Wed, 30 Oct 2013 21:43:27 +0530,
Vinod Koul wrote:
> 
> The drain and drain_notify callback were blocked by low level driver untill the
> draining was complete. Due to this being invoked with big fat mutex held, others
> ops like reading timestamp, calling pause, drop were blocked.
> 
> So to fix this we add a new snd_compr_drain_notify() API. This would be required
> to be invoked by low level driver when drain or partial drain has been completed
> by the DSP. Thus we make the drain and partial_drain callback as non blocking
> and driver returns immediately after notifying DSP.
> The waiting is done while relasing the lock so that other ops can go ahead.
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> CC: stable@vger.kernel.org
> ---
> v4:
>  move pr_err -> pr_debug to avoid spamming kernel log
>  make wait in drain interruptible
> v3:
>  call snd_compr_drain_notify from compress_stop()
>  rename draining -> drain_wake
>  add some comments on state transistion after drain
> v2:
>  fix the 80 line warn
>  move the state change to compress_drain()
> 
> 
>  include/sound/compress_driver.h |   13 +++++++++
>  sound/core/compress_offload.c   |   55 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 65 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 9031a26..e723935 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -48,6 +48,8 @@ struct snd_compr_ops;
>   *	the ring buffer
>   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>   * @sleep: poll sleep
> + * @wait: drain wait queue
> + * @drain_wake: condition for drain wake
>   */
>  struct snd_compr_runtime {
>  	snd_pcm_state_t state;
> @@ -59,6 +61,8 @@ struct snd_compr_runtime {
>  	u64 total_bytes_available;
>  	u64 total_bytes_transferred;
>  	wait_queue_head_t sleep;
> +	wait_queue_head_t wait;

I took a look back at the code, and now wonder why you can't use the
same wait queue (sleep) for drain?  PCM code uses the same waitqueue.

> +	unsigned int drain_wake;

Also, drain_wake can be omitted by checking the runtime state
instead, e.g.
	wait_event_interruptible(runtime->sleep,
				 runtime->state != SNDRV_PCM_STATE_DRAINING);

while snd_compr_drain_notify() would be

static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
{
	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
	wake_up(&stream->runtime->sleep);
}

(And, if we still use drain_wake, it can be better bool instead of
unsigned int.)


thanks,

Takashi

>  	void *private_data;
>  };
>  
> @@ -171,4 +175,13 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
>  	wake_up(&stream->runtime->sleep);
>  }
>  
> +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> +{
> +	if (snd_BUG_ON(!stream))
> +		return;
> +
> +	stream->runtime->drain_wake = 1;
> +	wake_up(&stream->runtime->wait);
> +}
> +
>  #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index bea523a..645cec5 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
>  	}
>  	runtime->state = SNDRV_PCM_STATE_OPEN;
>  	init_waitqueue_head(&runtime->sleep);
> +	init_waitqueue_head(&runtime->wait);
>  	data->stream.runtime = runtime;
>  	f->private_data = (void *)data;
>  	mutex_lock(&compr->lock);
> @@ -682,12 +683,48 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>  	if (!retval) {
>  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
>  		wake_up(&stream->runtime->sleep);
> +		snd_compr_drain_notify(stream);
>  		stream->runtime->total_bytes_available = 0;
>  		stream->runtime->total_bytes_transferred = 0;
>  	}
>  	return retval;
>  }
>  
> +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> +{
> +	int ret;
> +
> +	/*
> +	 * We are called with lock held. So drop the lock while we wait for
> +	 * drain complete notfication from the driver
> +	 *
> +	 * It is expected that driver will notify the drain completion and then
> +	 * stream will be moved to SETUP state, even if draining resulted in an
> +	 * error. We can trigger next track after this.
> +	 */
> +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +	mutex_unlock(&stream->device->lock);
> +
> +	/* we wait for drain to complete here, drain can return when
> +	 * interruption occurred, wait returned error or success.
> +	 * For the first two cases we don't do anything different here and
> +	 * return after waking up
> +	 */
> +
> +	ret = wait_event_interruptible(stream->runtime->wait,
> +			stream->runtime->drain_wake);
> +	if (ret == -ERESTARTSYS)
> +		pr_debug("wait aborted by a signal");
> +	else if (ret)
> +		pr_debug("wait for drain failed with %d\n", ret);
> +
> +
> +	wake_up(&stream->runtime->sleep);
> +	mutex_lock(&stream->device->lock);
> +
> +	return ret;
> +}
> +
>  static int snd_compr_drain(struct snd_compr_stream *stream)
>  {
>  	int retval;
> @@ -695,11 +732,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
>  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
>  		return -EPERM;
> +
> +	stream->runtime->drain_wake = 0;
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> -	if (!retval) {
> -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +	if (retval) {
> +		pr_debug("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
>  		wake_up(&stream->runtime->sleep);
> +		return retval;
>  	}
> +
> +	retval = snd_compress_wait_for_drain(stream);
> +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
>  	return retval;
>  }
>  
> @@ -735,10 +778,16 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	if (stream->next_track == false)
>  		return -EPERM;
>  
> +	stream->runtime->drain_wake = 0;
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> +	if (retval) {
> +		pr_debug("Partial drain returned failure\n");
> +		wake_up(&stream->runtime->sleep);
> +		return retval;
> +	}
>  
>  	stream->next_track = false;
> -	return retval;
> +	return snd_compress_wait_for_drain(stream);
>  }
>  
>  static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> -- 
> 1.7.0.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH v4] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul]

On Thu, Oct 31, 2013 at 08:40:14AM +0100, Takashi Iwai wrote:
> At Wed, 30 Oct 2013 21:43:27 +0530,
> Vinod Koul wrote:
> > 
> > The drain and drain_notify callback were blocked by low level driver untill the
> > draining was complete. Due to this being invoked with big fat mutex held, others
> > ops like reading timestamp, calling pause, drop were blocked.
> > 
> > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > to be invoked by low level driver when drain or partial drain has been completed
> > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > and driver returns immediately after notifying DSP.
> > The waiting is done while relasing the lock so that other ops can go ahead.
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > CC: stable@vger.kernel.org
> > ---
> > v4:
> >  move pr_err -> pr_debug to avoid spamming kernel log
> >  make wait in drain interruptible
> > v3:
> >  call snd_compr_drain_notify from compress_stop()
> >  rename draining -> drain_wake
> >  add some comments on state transistion after drain
> > v2:
> >  fix the 80 line warn
> >  move the state change to compress_drain()
> > 
> > 
> >  include/sound/compress_driver.h |   13 +++++++++
> >  sound/core/compress_offload.c   |   55 ++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 65 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > index 9031a26..e723935 100644
> > --- a/include/sound/compress_driver.h
> > +++ b/include/sound/compress_driver.h
> > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> >   *	the ring buffer
> >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> >   * @sleep: poll sleep
> > + * @wait: drain wait queue
> > + * @drain_wake: condition for drain wake
> >   */
> >  struct snd_compr_runtime {
> >  	snd_pcm_state_t state;
> > @@ -59,6 +61,8 @@ struct snd_compr_runtime {
> >  	u64 total_bytes_available;
> >  	u64 total_bytes_transferred;
> >  	wait_queue_head_t sleep;
> > +	wait_queue_head_t wait;
> 
> I took a look back at the code, and now wonder why you can't use the
> same wait queue (sleep) for drain?  PCM code uses the same waitqueue.
> 
> > +	unsigned int drain_wake;
> 
> Also, drain_wake can be omitted by checking the runtime state
> instead, e.g.
> 	wait_event_interruptible(runtime->sleep,
> 				 runtime->state != SNDRV_PCM_STATE_DRAINING);
> 
> while snd_compr_drain_notify() would be
> 
> static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> {
> 	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> 	wake_up(&stream->runtime->sleep);
> }
> 
> (And, if we still use drain_wake, it can be better bool instead of
> unsigned int.)
Hmmm, it started out thinking we can reuse the existing sleep but then went
ahead with a new one. I will test the changes and update this

--
~Vinod