[alsa-devel] [PATCH] ALSA: compress: fix drain calls blocking other compress functions
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)
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)
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);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
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);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
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);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
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!
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);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
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
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)
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);}
+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
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);}
+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
--
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);}
+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
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
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
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
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
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)
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);}
+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);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
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
participants (2)
-
Takashi Iwai -
Vinod Koul