[alsa-devel] [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 at intel.com>
> CC: stable at 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
>