[alsa-devel] [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 at intel.com>
> CC: stable at 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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>