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);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!