[alsa-devel] [PATCH 0/9] ALSA: compress offfload fixes
Hi Takashi,
Here is the patch series which fixes various issues being reported by users (out of tree sadly)
The first three and and last one are marked to stable as would like these to be fixed in older kernels as well. It would be good if you can send them as fixes to linus for 3.11.
Rest can go in the merge window
Fixes: - using lock for all operation was a very bad idead. This is bad as some of the ioctls like drain, partial drain can be time consuming and thus prevent any other operation while these are ongoing like Pause, Stop or timestamp query, so fix this be removing bunch of ioctls not to use device lock. - Now we dont have lock for pointer updates so this maybe racy, so use lock for doing lowest level calculation. - As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this change wont be huge. - Plus few fix like use snprintf, state chacks for pause, write etc..
Vinod Koul (9): ALSA: Compress - dont use lock for all ioctls ALSA: compress: use mutex in drain ASoC: compress: dont aquire lock for draining states ALSA: compress: use snprint instread of sprintf ALSA: compres: wakeup the poll thread on pause ALSA: compress: dont write when stream is paused ALSA: compress: allow write when stream is setup ALSA: compress: call pointer callback and updates under a lock ALSA: compress: use rate values for passing sampling rates
include/sound/compress_driver.h | 2 + include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 140 +++++++++++++++++++++++++------- sound/soc/soc-compress.c | 10 +++ 4 files changed, 122 insertions(+), 32 deletions(-)
Thanks ~Vinod
Some simple ioctls like timsetamp query, capabities query can be done anytime and should not be under the stream lock. Move these to snd_compress_simple_iotcls() which is invoked without lock held
While at it, improve readblity a bit by sprinkling some empty lines
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org --- sound/core/compress_offload.c | 79 ++++++++++++++++++++++++++++++----------- 1 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 99db892..868aefd 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -729,70 +729,107 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) return retval; }
-static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) +static int snd_compress_simple_ioctls(struct file *file, + struct snd_compr_stream *stream, + unsigned int cmd, unsigned long arg) { - struct snd_compr_file *data = f->private_data; - struct snd_compr_stream *stream; int retval = -ENOTTY;
- if (snd_BUG_ON(!data)) - return -EFAULT; - stream = &data->stream; - if (snd_BUG_ON(!stream)) - return -EFAULT; - mutex_lock(&stream->device->lock); switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION): put_user(SNDRV_COMPRESS_VERSION, (int __user *)arg) ? -EFAULT : 0; break; + case _IOC_NR(SNDRV_COMPRESS_GET_CAPS): retval = snd_compr_get_caps(stream, arg); break; + case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS): retval = snd_compr_get_codec_caps(stream, arg); break; + + case _IOC_NR(SNDRV_COMPRESS_TSTAMP): + retval = snd_compr_tstamp(stream, arg); + break; + + case _IOC_NR(SNDRV_COMPRESS_AVAIL): + retval = snd_compr_ioctl_avail(stream, arg); + break; + + /* drain and partial drain need special handling + * we need to drop the locks here as the streams would get blocked on the + * dsp to get drained. The locking would be handled in respective + * function here + */ + case _IOC_NR(SNDRV_COMPRESS_DRAIN): + retval = snd_compr_drain(stream); + break; + + case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN): + retval = snd_compr_partial_drain(stream); + break; + } + + return retval; +} + +static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) +{ + struct snd_compr_file *data = f->private_data; + struct snd_compr_stream *stream; + int retval = -ENOTTY; + + if (snd_BUG_ON(!data)) + return -EFAULT; + stream = &data->stream; + if (snd_BUG_ON(!stream)) + return -EFAULT; + + mutex_lock(&stream->device->lock); + switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS): retval = snd_compr_set_params(stream, arg); break; + case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS): retval = snd_compr_get_params(stream, arg); break; + case _IOC_NR(SNDRV_COMPRESS_SET_METADATA): retval = snd_compr_set_metadata(stream, arg); break; + case _IOC_NR(SNDRV_COMPRESS_GET_METADATA): retval = snd_compr_get_metadata(stream, arg); break; - case _IOC_NR(SNDRV_COMPRESS_TSTAMP): - retval = snd_compr_tstamp(stream, arg); - break; - case _IOC_NR(SNDRV_COMPRESS_AVAIL): - retval = snd_compr_ioctl_avail(stream, arg); - break; + case _IOC_NR(SNDRV_COMPRESS_PAUSE): retval = snd_compr_pause(stream); break; + case _IOC_NR(SNDRV_COMPRESS_RESUME): retval = snd_compr_resume(stream); break; + case _IOC_NR(SNDRV_COMPRESS_START): retval = snd_compr_start(stream); break; + case _IOC_NR(SNDRV_COMPRESS_STOP): retval = snd_compr_stop(stream); break; - case _IOC_NR(SNDRV_COMPRESS_DRAIN): - retval = snd_compr_drain(stream); - break; - case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN): - retval = snd_compr_partial_drain(stream); - break; + case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK): retval = snd_compr_next_track(stream); break;
+ default: + mutex_unlock(&stream->device->lock); + return snd_compress_simple_ioctls(f, stream, cmd, arg); + } + mutex_unlock(&stream->device->lock); return retval; }
Since we dont have lock over the function, we need to aquire mutex when checking and modfying states in drain and partial_drain handlers
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org --- sound/core/compress_offload.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 868aefd..e640f8c 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here + * so while using the stream state auquire the lock but relase before invoking + * DSP as the call will possibly take a while + */ static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
+ mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED || - stream->runtime->state == SNDRV_PCM_STATE_SETUP) - return -EPERM; + stream->runtime->state == SNDRV_PCM_STATE_SETUP) { + retval = -EPERM; + goto ret; + } + mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN); + mutex_lock(&stream->device->lock); if (!retval) { stream->runtime->state = SNDRV_PCM_STATE_DRAINING; wake_up(&stream->runtime->sleep); } +ret: + mutex_unlock(&stream->device->lock); return retval; }
@@ -716,9 +727,14 @@ static int snd_compr_next_track(struct snd_compr_stream *stream) static int snd_compr_partial_drain(struct snd_compr_stream *stream) { int retval; + + mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED || - stream->runtime->state == SNDRV_PCM_STATE_SETUP) + stream->runtime->state == SNDRV_PCM_STATE_SETUP) { + mutex_unlock(&stream->device->lock); return -EPERM; + } + mutex_unlock(&stream->device->lock); /* stream can be drained only when next track has been signalled */ if (stream->next_track == false) return -EPERM;
Both draining and partial draning states will take a while getting executed. The lock aquired will block the other operations like pause, stop etc which are perfectly valid cmds during these states. So dont use mutex while invoking DSP for these ops
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org --- sound/soc/soc-compress.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 06a8000..800ee89 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -171,6 +171,16 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd) struct snd_soc_dai *codec_dai = rtd->codec_dai; int ret = 0;
+ /* for partial drain and drain cmd dont aquire lock while invoking DSP. + * These calls will be blocked till these operation can complete while + * will be a while. And during that app can invoke STOP, PAUSE etc + */ + if (cmd == SND_COMPR_TRIGGER_PARTIAL_DRAIN || cmd == SND_COMPR_TRIGGER_DRAIN) { + if (platform->driver->compr_ops && + platform->driver->compr_ops->trigger) + return platform->driver->compr_ops->trigger(cstream, cmd); + } + mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
if (platform->driver->compr_ops && platform->driver->compr_ops->trigger) {
To prevent possiblity of exceeding buffer size
Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/core/compress_offload.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index e640f8c..1e722c2 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -871,7 +871,7 @@ static int snd_compress_dev_register(struct snd_device *device) return -EBADFD; compr = device->device_data;
- sprintf(str, "comprC%iD%i", compr->card->number, compr->device); + snprintf(str, sizeof(str), "comprC%iD%i", compr->card->number, compr->device); pr_debug("reg %s for device %s, direction %d\n", str, compr->name, compr->direction); /* register compressed device */
this allows the poll to be unblocked, if it is blocked and yeilds the control of userthread blocked
Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/core/compress_offload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 1e722c2..417a456 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -629,9 +629,12 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) return -EPERM; + retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH); - if (!retval) + if (!retval) { stream->runtime->state = SNDRV_PCM_STATE_PAUSED; + wake_up(&stream->runtime->sleep); + } return retval; }
and return bytes written as 0 and not error
Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/core/compress_offload.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 417a456..fb570c8 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -256,7 +256,7 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, struct snd_compr_file *data = f->private_data; struct snd_compr_stream *stream; size_t avail; - int retval; + int retval = 0;
if (snd_BUG_ON(!data)) return -EFAULT; @@ -264,6 +264,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, stream = &data->stream; mutex_lock(&stream->device->lock); /* write is allowed when stream is running or has been steup */ + /* + * if the stream is in paused state, return the + * number of bytes consumed as 0 + */ + if (stream->runtime->state == SNDRV_PCM_STATE_PAUSED) { + mutex_unlock(&stream->device->lock); + return retval; + } if (stream->runtime->state != SNDRV_PCM_STATE_SETUP && stream->runtime->state != SNDRV_PCM_STATE_RUNNING) { mutex_unlock(&stream->device->lock);
We should allow write and thus poll as well when stream has been prepared. This allows usermode to write buffers ahead of time
Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/core/compress_offload.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index fb570c8..898a1d9 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -263,7 +263,6 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
stream = &data->stream; mutex_lock(&stream->device->lock); - /* write is allowed when stream is running or has been steup */ /* * if the stream is in paused state, return the * number of bytes consumed as 0 @@ -272,8 +271,11 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, mutex_unlock(&stream->device->lock); return retval; } + + /* write is allowed when stream is running or prepared or in setup */ if (stream->runtime->state != SNDRV_PCM_STATE_SETUP && - stream->runtime->state != SNDRV_PCM_STATE_RUNNING) { + stream->runtime->state != SNDRV_PCM_STATE_RUNNING && + stream->runtime->state != SNDRV_PCM_STATE_PREPARED) { mutex_unlock(&stream->device->lock); return -EBADFD; } @@ -380,8 +382,7 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait) return -EFAULT;
mutex_lock(&stream->device->lock); - if (stream->runtime->state == SNDRV_PCM_STATE_PAUSED || - stream->runtime->state == SNDRV_PCM_STATE_OPEN) { + if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) { retval = -EBADFD; goto out; }
to prevent racy claculations as multiple threads can invoke and update values
While at it give some emptylines in code to improve readablity
Signed-off-by: Vinod Koul vinod.koul@intel.com --- include/sound/compress_driver.h | 2 ++ sound/core/compress_offload.c | 13 +++++++++++++ 2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index 9031a26..30258f3 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -48,6 +48,7 @@ struct snd_compr_ops; * the ring buffer * @total_bytes_transferred: cumulative bytes transferred by offload DSP * @sleep: poll sleep + * @pointer_lock: lock used for buffer pointer update */ struct snd_compr_runtime { snd_pcm_state_t state; @@ -60,6 +61,7 @@ struct snd_compr_runtime { u64 total_bytes_transferred; wait_queue_head_t sleep; void *private_data; + struct mutex pointer_lock; };
/** diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 898a1d9..01b179f 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -122,6 +122,7 @@ static int snd_compr_open(struct inode *inode, struct file *f) return -ENOMEM; } runtime->state = SNDRV_PCM_STATE_OPEN; + mutex_init(&runtime->pointer_lock); init_waitqueue_head(&runtime->sleep); data->stream.runtime = runtime; f->private_data = (void *)data; @@ -151,13 +152,21 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, { if (!stream->ops->pointer) return -ENOTSUPP; + + mutex_lock(&stream->runtime->pointer_lock); + stream->ops->pointer(stream, tstamp); + pr_debug("dsp consumed till %d total %d bytes\n", tstamp->byte_offset, tstamp->copied_total); + if (stream->direction == SND_COMPRESS_PLAYBACK) stream->runtime->total_bytes_transferred = tstamp->copied_total; else stream->runtime->total_bytes_available = tstamp->copied_total; + + mutex_unlock(&stream->runtime->pointer_lock); + return 0; }
@@ -165,7 +174,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, struct snd_compr_avail *avail) { memset(avail, 0, sizeof(*avail)); + snd_compr_update_tstamp(stream, &avail->tstamp); + /* Still need to return avail even if tstamp can't be filled in */
if (stream->runtime->total_bytes_available == 0 && @@ -174,9 +185,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, pr_debug("detected init and someone forgot to do a write\n"); return stream->runtime->buffer_size; } + pr_debug("app wrote %lld, DSP consumed %lld\n", stream->runtime->total_bytes_available, stream->runtime->total_bytes_transferred); + if (stream->runtime->total_bytes_available == stream->runtime->total_bytes_transferred) { if (stream->direction == SND_COMPRESS_PLAYBACK) {
Clarify that its better to use the rate values like 8000, 12000, 11025, 44100, 48000 etc to send sampling rate values to driver. The changes kernel ABI but no drivers exist upstream so okay to change now Tinycompress to be fixed as well
For this change bump the API version as well
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org --- include/uapi/sound/compress_offload.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index d630163..cad87ff 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -62,7 +62,7 @@ struct snd_compr_params { * progress. It shall not be used for timing estimates. * @pcm_io_frames: Frames rendered or received by DSP into a mixer or an audio * output/input. This field should be used for A/V sync or time estimates. - * @sampling_rate: sampling rate of audio + * @sampling_rate: sampling rate value of audio, eg 12000, 48000 */ struct snd_compr_tstamp { __u32 byte_offset;
On Tue, Aug 27, 2013 at 12:22:31PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:31 +0530, Vinod Koul wrote:
Some simple ioctls like timsetamp query, capabities query can be done anytime and should not be under the stream lock. Move these to snd_compress_simple_iotcls() which is invoked without lock held
While at it, improve readblity a bit by sprinkling some empty lines
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
Why it's needed for stable? Fixing any real bugs?
yup, users are complaining that while streams are draining they can't read timstamps. Also one case where a user hit pause didnt go thru as lock preveted the pause to be executed..
Since 3.10 is next LTS kernel, I forsee lots of folks using taht for a while so makes sense to fix there as well
- default:
mutex_unlock(&stream->device->lock);return snd_compress_simple_ioctls(f, stream, cmd, arg);In this code, it still locks/unlocks shortly unnecessarily. It should be rather like: switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION): ... case _IOC_NR(SNDRV_COMPRESS_GET_CAPS): .... default: retval = snd_compress_locked_ioctls(f, stream, cmd, arg); }
Hmmm, okay no point in blocking. I will reverse the flow
~Vinod
--
On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:32 +0530, Vinod Koul wrote:
Since we dont have lock over the function, we need to aquire mutex when checking and modfying states in drain and partial_drain handlers
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
sound/core/compress_offload.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 868aefd..e640f8c 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here. The point of lock is to sync the stream states here. We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
~Vinod
On Tue, Aug 27, 2013 at 12:31:33PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:38 +0530, Vinod Koul wrote:
to prevent racy claculations as multiple threads can invoke and update values
Why a new lock? Can't it be the same lock?
Pointer query and updates IMO should be independent of the trigger ops. I dont see a reason why we should blcok pointers while we are pausing...
~Vinod
On Tue, Aug 27, 2013 at 12:46:36PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:30 +0530, Vinod Koul wrote:
Hi Takashi,
Here is the patch series which fixes various issues being reported by users (out of tree sadly)
The first three and and last one are marked to stable as would like these to be fixed in older kernels as well. It would be good if you can send them as fixes to linus for 3.11.
Sorry, no go. If it's only about out-of-tree drivers, it's even questionable whether it worth for stable kernel, because we have no real test case for our own.
Since lot of embedded folks will be on 3.10 then would have been nicer if it went to stable. I know users will backport, but if a kernel provided the desired functionalty then would have been great.
Fixes:
- using lock for all operation was a very bad idead. This is bad as some of the ioctls like drain, partial drain can be time consuming and thus prevent any
other operation while these are ongoing like Pause, Stop or timestamp query, so fix this be removing bunch of ioctls not to use device lock.
Although not all of them need locks, it's easier to manage in most cases to perform in the lock. For drain or such operation, you can simply unlock and re-lock in the call itself, as your patch in the series does.a
- Now we dont have lock for pointer updates so this maybe racy, so use lock for doing lowest level calculation.
Similarly, introducing yet another lock would just choke you neck.
Well i thought that pointer updates are orthogonal to other triggers so there is no issue if we seprate the two. How do you think in long run will this impact..?
- As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to prgram decoders. Only meaning of the field is changing now.
- Plus few fix like use snprintf, state chacks for pause, write etc..
I don't like to merge irrelevant space changes into a patch if it's a fix patch, especially if it's targeted to stable. The fix is the fix. Please separate.
Sure makes sense...
So do you want these to be sent to stable or not. I would prefer to be sent
Thanks ~Vinod
On Tue, Aug 27, 2013 at 12:53:54PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 15:17:41 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
--- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here.
What's about other places calling the trigger ops within lock? You can't mix things.
Well i was going to treat drain only as exception to this. The issue here is during the long drain other triggers are perfectly valid cases
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
that part can be checked by seeing if we are already draining.
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
The reason for blocked implementation is to treat return of the call as notifcation that draining is completed.
For example user has written all the buffers, lets says worth 3 secs and now has triggered drain. User needs to wait till drain is complete before closing the device etc. So he waits on drain to compelete..
Do you have a better way to manage this?
~Vinod
At Tue, 27 Aug 2013 12:10:31 +0530, Vinod Koul wrote:
Some simple ioctls like timsetamp query, capabities query can be done anytime and should not be under the stream lock. Move these to snd_compress_simple_iotcls() which is invoked without lock held
While at it, improve readblity a bit by sprinkling some empty lines
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
Why it's needed for stable? Fixing any real bugs?
sound/core/compress_offload.c | 79 ++++++++++++++++++++++++++++++----------- 1 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 99db892..868aefd 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -729,70 +729,107 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) return retval; }
-static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) +static int snd_compress_simple_ioctls(struct file *file,
struct snd_compr_stream *stream,unsigned int cmd, unsigned long arg){
struct snd_compr_file *data = f->private_data;
struct snd_compr_stream *stream; int retval = -ENOTTY;
if (snd_BUG_ON(!data))
return -EFAULT;stream = &data->stream;
if (snd_BUG_ON(!stream))
return -EFAULT;mutex_lock(&stream->device->lock); switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION): put_user(SNDRV_COMPRESS_VERSION, (int __user *)arg) ? -EFAULT : 0; break;
- case _IOC_NR(SNDRV_COMPRESS_GET_CAPS): retval = snd_compr_get_caps(stream, arg); break;
- case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS): retval = snd_compr_get_codec_caps(stream, arg); break;
- case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_AVAIL):
retval = snd_compr_ioctl_avail(stream, arg);break;/* drain and partial drain need special handling* we need to drop the locks here as the streams would get blocked on the* dsp to get drained. The locking would be handled in respective* function here*/- case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);break;- case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
retval = snd_compr_partial_drain(stream);break;- }
- return retval;
+}
+static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) +{
- struct snd_compr_file *data = f->private_data;
- struct snd_compr_stream *stream;
- int retval = -ENOTTY;
- if (snd_BUG_ON(!data))
return -EFAULT;- stream = &data->stream;
- if (snd_BUG_ON(!stream))
return -EFAULT;- mutex_lock(&stream->device->lock);
- switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS): retval = snd_compr_set_params(stream, arg); break;
- case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS): retval = snd_compr_get_params(stream, arg); break;
- case _IOC_NR(SNDRV_COMPRESS_SET_METADATA): retval = snd_compr_set_metadata(stream, arg); break;
- case _IOC_NR(SNDRV_COMPRESS_GET_METADATA): retval = snd_compr_get_metadata(stream, arg); break;
- case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_AVAIL):
retval = snd_compr_ioctl_avail(stream, arg);break;
- case _IOC_NR(SNDRV_COMPRESS_PAUSE): retval = snd_compr_pause(stream); break;
- case _IOC_NR(SNDRV_COMPRESS_RESUME): retval = snd_compr_resume(stream); break;
- case _IOC_NR(SNDRV_COMPRESS_START): retval = snd_compr_start(stream); break;
- case _IOC_NR(SNDRV_COMPRESS_STOP): retval = snd_compr_stop(stream); break;
- case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);break;- case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
retval = snd_compr_partial_drain(stream);break;
case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK): retval = snd_compr_next_track(stream); break;
default:
mutex_unlock(&stream->device->lock);return snd_compress_simple_ioctls(f, stream, cmd, arg);
In this code, it still locks/unlocks shortly unnecessarily. It should be rather like: switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION): ... case _IOC_NR(SNDRV_COMPRESS_GET_CAPS): .... default: retval = snd_compress_locked_ioctls(f, stream, cmd, arg); }
Takashi
At Tue, 27 Aug 2013 12:10:32 +0530, Vinod Koul wrote:
Since we dont have lock over the function, we need to aquire mutex when checking and modfying states in drain and partial_drain handlers
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
sound/core/compress_offload.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 868aefd..e640f8c 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
Takashi
- mutex_lock(&stream->device->lock); if (!retval) { stream->runtime->state = SNDRV_PCM_STATE_DRAINING; wake_up(&stream->runtime->sleep); }
+ret:
- mutex_unlock(&stream->device->lock); return retval;
}
@@ -716,9 +727,14 @@ static int snd_compr_next_track(struct snd_compr_stream *stream) static int snd_compr_partial_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {mutex_unlock(&stream->device->lock);- }
- mutex_unlock(&stream->device->lock); /* stream can be drained only when next track has been signalled */ if (stream->next_track == false) return -EPERM;
-- 1.7.0.4
At Tue, 27 Aug 2013 12:10:39 +0530, Vinod Koul wrote:
Clarify that its better to use the rate values like 8000, 12000, 11025, 44100, 48000 etc to send sampling rate values to driver. The changes kernel ABI but no drivers exist upstream so okay to change now Tinycompress to be fixed as well
For this change bump the API version as well
I see no corresponding changes at all...
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
Again, why it must be to stable?
Takashi
include/uapi/sound/compress_offload.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index d630163..cad87ff 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -62,7 +62,7 @@ struct snd_compr_params {
- progress. It shall not be used for timing estimates.
- @pcm_io_frames: Frames rendered or received by DSP into a mixer or an audio
- output/input. This field should be used for A/V sync or time estimates.
- @sampling_rate: sampling rate of audio
*/
- @sampling_rate: sampling rate value of audio, eg 12000, 48000
struct snd_compr_tstamp { __u32 byte_offset; -- 1.7.0.4
At Tue, 27 Aug 2013 12:10:38 +0530, Vinod Koul wrote:
to prevent racy claculations as multiple threads can invoke and update values
Why a new lock? Can't it be the same lock?
Takashi
While at it give some emptylines in code to improve readablity
Signed-off-by: Vinod Koul vinod.koul@intel.com
include/sound/compress_driver.h | 2 ++ sound/core/compress_offload.c | 13 +++++++++++++ 2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index 9031a26..30258f3 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -48,6 +48,7 @@ struct snd_compr_ops;
- the ring buffer
- @total_bytes_transferred: cumulative bytes transferred by offload DSP
- @sleep: poll sleep
*/
- @pointer_lock: lock used for buffer pointer update
struct snd_compr_runtime { snd_pcm_state_t state; @@ -60,6 +61,7 @@ struct snd_compr_runtime { u64 total_bytes_transferred; wait_queue_head_t sleep; void *private_data;
- struct mutex pointer_lock;
};
/** diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 898a1d9..01b179f 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -122,6 +122,7 @@ static int snd_compr_open(struct inode *inode, struct file *f) return -ENOMEM; } runtime->state = SNDRV_PCM_STATE_OPEN;
- mutex_init(&runtime->pointer_lock); init_waitqueue_head(&runtime->sleep); data->stream.runtime = runtime; f->private_data = (void *)data;
@@ -151,13 +152,21 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, { if (!stream->ops->pointer) return -ENOTSUPP;
- mutex_lock(&stream->runtime->pointer_lock);
- stream->ops->pointer(stream, tstamp);
- pr_debug("dsp consumed till %d total %d bytes\n", tstamp->byte_offset, tstamp->copied_total);
- if (stream->direction == SND_COMPRESS_PLAYBACK) stream->runtime->total_bytes_transferred = tstamp->copied_total; else stream->runtime->total_bytes_available = tstamp->copied_total;
- mutex_unlock(&stream->runtime->pointer_lock);
- return 0;
}
@@ -165,7 +174,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, struct snd_compr_avail *avail) { memset(avail, 0, sizeof(*avail));
snd_compr_update_tstamp(stream, &avail->tstamp);
/* Still need to return avail even if tstamp can't be filled in */
if (stream->runtime->total_bytes_available == 0 &&
@@ -174,9 +185,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, pr_debug("detected init and someone forgot to do a write\n"); return stream->runtime->buffer_size; }
- pr_debug("app wrote %lld, DSP consumed %lld\n", stream->runtime->total_bytes_available, stream->runtime->total_bytes_transferred);
- if (stream->runtime->total_bytes_available == stream->runtime->total_bytes_transferred) { if (stream->direction == SND_COMPRESS_PLAYBACK) {
-- 1.7.0.4
At Tue, 27 Aug 2013 12:10:30 +0530, Vinod Koul wrote:
Hi Takashi,
Here is the patch series which fixes various issues being reported by users (out of tree sadly)
The first three and and last one are marked to stable as would like these to be fixed in older kernels as well. It would be good if you can send them as fixes to linus for 3.11.
Sorry, no go. If it's only about out-of-tree drivers, it's even questionable whether it worth for stable kernel, because we have no real test case for our own.
Rest can go in the merge window
Fixes:
- using lock for all operation was a very bad idead. This is bad as some of the ioctls like drain, partial drain can be time consuming and thus prevent any
other operation while these are ongoing like Pause, Stop or timestamp query, so fix this be removing bunch of ioctls not to use device lock.
Although not all of them need locks, it's easier to manage in most cases to perform in the lock. For drain or such operation, you can simply unlock and re-lock in the call itself, as your patch in the series does.
- Now we dont have lock for pointer updates so this maybe racy, so use lock for doing lowest level calculation.
Similarly, introducing yet another lock would just choke you neck.
- As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
I see no code touching sampling_rate field.
- Plus few fix like use snprintf, state chacks for pause, write etc..
I don't like to merge irrelevant space changes into a patch if it's a fix patch, especially if it's targeted to stable. The fix is the fix. Please separate.
thanks,
Takashi
Vinod Koul (9): ALSA: Compress - dont use lock for all ioctls ALSA: compress: use mutex in drain ASoC: compress: dont aquire lock for draining states ALSA: compress: use snprint instread of sprintf ALSA: compres: wakeup the poll thread on pause ALSA: compress: dont write when stream is paused ALSA: compress: allow write when stream is setup ALSA: compress: call pointer callback and updates under a lock ALSA: compress: use rate values for passing sampling rates
include/sound/compress_driver.h | 2 + include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 140 +++++++++++++++++++++++++------- sound/soc/soc-compress.c | 10 +++ 4 files changed, 122 insertions(+), 32 deletions(-)
Thanks ~Vinod
At Tue, 27 Aug 2013 15:17:41 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:32 +0530, Vinod Koul wrote:
Since we dont have lock over the function, we need to aquire mutex when checking and modfying states in drain and partial_drain handlers
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
sound/core/compress_offload.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 868aefd..e640f8c 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here.
What's about other places calling the trigger ops within lock? You can't mix things.
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
Takashi
At Tue, 27 Aug 2013 15:14:15 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:22:31PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:31 +0530, Vinod Koul wrote:
Some simple ioctls like timsetamp query, capabities query can be done anytime and should not be under the stream lock. Move these to snd_compress_simple_iotcls() which is invoked without lock held
While at it, improve readblity a bit by sprinkling some empty lines
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
Why it's needed for stable? Fixing any real bugs?
yup, users are complaining that while streams are draining they can't read timstamps. Also one case where a user hit pause didnt go thru as lock preveted the pause to be executed..
Then write the problems more clearly. I saw no such information in the changelog.
Since 3.10 is next LTS kernel, I forsee lots of folks using taht for a while so makes sense to fix there as well
Depends on the fix and the problem. The fact that this can't be tested only with 3.10 kernel (no real driver exists), I doubt it's worth. Stable kernels aren't for out-of-tree drivers.
And, if you really target to the stable tree, don't put any unnecessary changes like white space fixes. Read stable_kernel_rules.txt once more.
thanks,
Takashi
- default:
mutex_unlock(&stream->device->lock);return snd_compress_simple_ioctls(f, stream, cmd, arg);In this code, it still locks/unlocks shortly unnecessarily. It should be rather like: switch (_IOC_NR(cmd)) { case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION): ... case _IOC_NR(SNDRV_COMPRESS_GET_CAPS): .... default: retval = snd_compress_locked_ioctls(f, stream, cmd, arg); }
Hmmm, okay no point in blocking. I will reverse the flow
~Vinod
--
At Tue, 27 Aug 2013 15:18:59 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:31:33PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:38 +0530, Vinod Koul wrote:
to prevent racy claculations as multiple threads can invoke and update values
Why a new lock? Can't it be the same lock?
Pointer query and updates IMO should be independent of the trigger ops. I dont see a reason why we should blcok pointers while we are pausing...
What happens if the trigger or reset is called during your taking the timestamp?
Takashi
At Tue, 27 Aug 2013 15:46:14 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:53:54PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 15:17:41 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
--- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here.
What's about other places calling the trigger ops within lock? You can't mix things.
Well i was going to treat drain only as exception to this. The issue here is during the long drain other triggers are perfectly valid cases
An ops callback must be defined either to be locked or unlocked. Calling in the unlocked context only for some case doesn't sound right.
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
that part can be checked by seeing if we are already draining.
But how? The place you're calling trigger is unlocked. Suppose another thread calling trigger_stop just between mutex_unlock() and stream->ops->trigger(DRAIN) call in the above. The state check doesn't work there.
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
The reason for blocked implementation is to treat return of the call as notifcation that draining is completed.
For example user has written all the buffers, lets says worth 3 secs and now has triggered drain. User needs to wait till drain is complete before closing the device etc. So he waits on drain to compelete..
Do you have a better way to manage this?
Split the drain action in two parts, trigger and synchronization:
lock(); ... trigger(pause); while (!pause_finished) { unlock(); schedule_or_sleep_or_whatever(); lock(); } ... unlock();
Takashi
At Tue, 27 Aug 2013 15:39:55 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:46:36PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:30 +0530, Vinod Koul wrote:
Hi Takashi,
Here is the patch series which fixes various issues being reported by users (out of tree sadly)
The first three and and last one are marked to stable as would like these to be fixed in older kernels as well. It would be good if you can send them as fixes to linus for 3.11.
Sorry, no go. If it's only about out-of-tree drivers, it's even questionable whether it worth for stable kernel, because we have no real test case for our own.
Since lot of embedded folks will be on 3.10 then would have been nicer if it went to stable.
"It'd be nice" doesn't satisfy the condition for stable kernels, unfortunately.
I know users will backport, but if a kernel provided the desired functionalty then would have been great.
Fixes:
- using lock for all operation was a very bad idead. This is bad as some of the ioctls like drain, partial drain can be time consuming and thus prevent any
other operation while these are ongoing like Pause, Stop or timestamp query, so fix this be removing bunch of ioctls not to use device lock.
Although not all of them need locks, it's easier to manage in most cases to perform in the lock. For drain or such operation, you can simply unlock and re-lock in the call itself, as your patch in the series does.a
- Now we dont have lock for pointer updates so this maybe racy, so use lock for doing lowest level calculation.
Similarly, introducing yet another lock would just choke you neck.
Well i thought that pointer updates are orthogonal to other triggers so there is no issue if we seprate the two. How do you think in long run will this impact..?
If both locks can be never called in nested way, then it'd work. But if the locks can be nested, better to avoid as much as possible. Put in that way: what if the time for critical section via stream->device->lock is short enough (it should be)?
- As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to program decoders. Only meaning of the field is changing now.
So you're proposing a patch just changing the comment in the header file as a stable fix patch? Please reread stable_kernel_rules.txt once again.
- Plus few fix like use snprintf, state chacks for pause, write etc..
I don't like to merge irrelevant space changes into a patch if it's a fix patch, especially if it's targeted to stable. The fix is the fix. Please separate.
Sure makes sense...
So do you want these to be sent to stable or not. I would prefer to be sent
This pretty depends on how it can be "fixed"...
Takashi
On Tue, Aug 27, 2013 at 02:23:19PM +0200, Takashi Iwai wrote:
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here.
What's about other places calling the trigger ops within lock? You can't mix things.
Well i was going to treat drain only as exception to this. The issue here is during the long drain other triggers are perfectly valid cases
An ops callback must be defined either to be locked or unlocked. Calling in the unlocked context only for some case doesn't sound right.
well all the callbacks except drain is called with lock held from framework...
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
that part can be checked by seeing if we are already draining.
But how? The place you're calling trigger is unlocked. Suppose another thread calling trigger_stop just between mutex_unlock() and stream->ops->trigger(DRAIN) call in the above. The state check doesn't work there.
the framework does this with lock held and then calls the driver something like this will ensure this while making sync right... mutex_lock(&lock); if (state == DRAINING) { mutex_unlock(&lock); return -EPERM; } else state = DRAINING; mutex_unlock(&lock);
ops->drain(substream);
mutex_lock(&lock); state = DRAINED; mutex_unlock(&lock);
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
The reason for blocked implementation is to treat return of the call as notifcation that draining is completed.
For example user has written all the buffers, lets says worth 3 secs and now has triggered drain. User needs to wait till drain is complete before closing the device etc. So he waits on drain to compelete..
Do you have a better way to manage this?
Split the drain action in two parts, trigger and synchronization:
lock(); ... trigger(pause); while (!pause_finished) { unlock(); schedule_or_sleep_or_whatever(); lock(); } ... unlock();
okay, i guess above is on same lines...
~Vinod --
On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
- As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to program decoders. Only meaning of the field is changing now.
So you're proposing a patch just changing the comment in the header file as a stable fix patch? Please reread stable_kernel_rules.txt once again.
Yes along with header version so that tinycompress can cope with it.
The meaning of the value is changing here...
~Vinod --
On Tue, Aug 27, 2013 at 02:28:08PM +0100, Mark Brown wrote:
On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
Vinod Koul wrote:
Since lot of embedded folks will be on 3.10 then would have been nicer if it went to stable.
"It'd be nice" doesn't satisfy the condition for stable kernels, unfortunately.
On the other hand if there's an issue which essentially every user is most likely going to end up backporting a fix for it seems reasonable to consider getting that resolved. Personally I'd be a lot more conservative than Vinod with this one and only consider the fix for blocking during drain. The current series seems rather invasive though.
Not all are marked for stable. Only thecouple to fix locking problem and defination of sampling_rate. Rest will go thru usual cycle, though I suspect with folks in embedded will take complete series.
~Vinod --
On Tue, Aug 27, 2013 at 02:29:59PM +0100, Mark Brown wrote:
On Tue, Aug 27, 2013 at 12:10:39PM +0530, Vinod Koul wrote:
Clarify that its better to use the rate values like 8000, 12000, 11025, 44100, 48000 etc to send sampling rate values to driver. The changes kernel ABI but no drivers exist upstream so okay to change now Tinycompress to be fixed as well
- @sampling_rate: sampling rate of audio
- @sampling_rate: sampling rate value of audio, eg 12000, 48000
It's not clear why this is a change in the ABI - the values written and the types don't change. Personally I'd not change the ABI - it's a bit late now that the code has been in the kernel for several releases.
the problem is the meaning of sampling_rate. It was SND_PCM_RATE_XXX. It was a lack of foresight on my side, we should have done values which we are changing now to handle rates not defined here.
But since none of drivers are upstream, impact is IMO managble. I was planning to communicate this along with tinycompress fixes to all folks who i know are using this :)
~Vinod --
On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
Vinod Koul wrote:
Since lot of embedded folks will be on 3.10 then would have been nicer if it went to stable.
"It'd be nice" doesn't satisfy the condition for stable kernels, unfortunately.
On the other hand if there's an issue which essentially every user is most likely going to end up backporting a fix for it seems reasonable to consider getting that resolved. Personally I'd be a lot more conservative than Vinod with this one and only consider the fix for blocking during drain. The current series seems rather invasive though.
On Tue, Aug 27, 2013 at 12:10:39PM +0530, Vinod Koul wrote:
Clarify that its better to use the rate values like 8000, 12000, 11025, 44100, 48000 etc to send sampling rate values to driver. The changes kernel ABI but no drivers exist upstream so okay to change now Tinycompress to be fixed as well
- @sampling_rate: sampling rate of audio
- @sampling_rate: sampling rate value of audio, eg 12000, 48000
It's not clear why this is a change in the ABI - the values written and the types don't change. Personally I'd not change the ABI - it's a bit late now that the code has been in the kernel for several releases.
On Tue, Aug 27, 2013 at 04:05:36PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 18:44:38 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
- As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to program decoders. Only meaning of the field is changing now.
So you're proposing a patch just changing the comment in the header file as a stable fix patch? Please reread stable_kernel_rules.txt once again.
Yes along with header version so that tinycompress can cope with it.
I've seen nothing but changing the comment in the patch. What's missing?
The meaning of the value is changing here...
Again, is this a bug fix? Stable patches are only for bug fixes.
yup... we can't do 12K, 24K decoding without this. We actually found when folks tried aac with 12K
~Vinod --
On Tue, Aug 27, 2013 at 04:22:39PM +0200, Takashi Iwai wrote:
I understand it, but my question is what fixes your patch except for the comment in the header file? In other words, by changing the comment text, what will be fixed in the end?
the meaning and API version which tinycompress will use to send right values..
~Vinod
At Tue, 27 Aug 2013 18:39:22 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 02:23:19PM +0200, Takashi Iwai wrote:
> +/* this fn is called without lock being held and we change stream states here > + * so while using the stream state auquire the lock but relase before invoking > + * DSP as the call will possibly take a while > + */ > static int snd_compr_drain(struct snd_compr_stream *stream) > { > int retval; > > + mutex_lock(&stream->device->lock); > if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED || > - stream->runtime->state == SNDRV_PCM_STATE_SETUP) > - return -EPERM; > + stream->runtime->state == SNDRV_PCM_STATE_SETUP) { > + retval = -EPERM; > + goto ret; > + } > + mutex_unlock(&stream->device->lock); > retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here.
What's about other places calling the trigger ops within lock? You can't mix things.
Well i was going to treat drain only as exception to this. The issue here is during the long drain other triggers are perfectly valid cases
An ops callback must be defined either to be locked or unlocked. Calling in the unlocked context only for some case doesn't sound right.
well all the callbacks except drain is called with lock held from framework...
You're calling the same ops with and without the lock. I'd understand better if you created a new ops for drain, but in the current patch, it's still the same ops...
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
that part can be checked by seeing if we are already draining.
But how? The place you're calling trigger is unlocked. Suppose another thread calling trigger_stop just between mutex_unlock() and stream->ops->trigger(DRAIN) call in the above. The state check doesn't work there.
the framework does this with lock held and then calls the driver something like this will ensure this while making sync right...
It's still racy. In your code below:
mutex_lock(&lock); if (state == DRAINING) { mutex_unlock(&lock); return -EPERM; } else state = DRAINING; mutex_unlock(&lock);
Suppose that another thread calls stop at this point before ops->drain() is called. It'll change the state to STOP, call ops->trigger(STOP). And ops->drain() (or ops->trigger(DRAIN)) may be called at the same time because there is no protection without knowing that it's being stopped.
ops->drain(substream);
mutex_lock(&lock); state = DRAINED; mutex_unlock(&lock);
Takashi
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
The reason for blocked implementation is to treat return of the call as notifcation that draining is completed.
For example user has written all the buffers, lets says worth 3 secs and now has triggered drain. User needs to wait till drain is complete before closing the device etc. So he waits on drain to compelete..
Do you have a better way to manage this?
Split the drain action in two parts, trigger and synchronization:
lock(); ... trigger(pause); while (!pause_finished) { unlock(); schedule_or_sleep_or_whatever(); lock(); } ... unlock();
okay, i guess above is on same lines...
~Vinod
At Tue, 27 Aug 2013 18:44:38 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
- As disscused on our sample rate problem, lets move to use rate values and I will fix the lib too. Since the driver are not upstream the impact of this
change wont be huge.
I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to program decoders. Only meaning of the field is changing now.
So you're proposing a patch just changing the comment in the header file as a stable fix patch? Please reread stable_kernel_rules.txt once again.
Yes along with header version so that tinycompress can cope with it.
I've seen nothing but changing the comment in the patch. What's missing?
The meaning of the value is changing here...
Again, is this a bug fix? Stable patches are only for bug fixes.
Takashi
On Tue, Aug 27, 2013 at 04:03:03PM +0200, Takashi Iwai wrote:
An ops callback must be defined either to be locked or unlocked. Calling in the unlocked context only for some case doesn't sound right.
well all the callbacks except drain is called with lock held from framework...
You're calling the same ops with and without the lock. I'd understand better if you created a new ops for drain, but in the current patch, it's still the same ops...
yup but cant different callbacks in the ops have different conditions. Anyway all except drain ones are supposed to NOT block.
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
that part can be checked by seeing if we are already draining.
But how? The place you're calling trigger is unlocked. Suppose another thread calling trigger_stop just between mutex_unlock() and stream->ops->trigger(DRAIN) call in the above. The state check doesn't work there.
the framework does this with lock held and then calls the driver something like this will ensure this while making sync right...
It's still racy. In your code below:
mutex_lock(&lock); if (state == DRAINING) { mutex_unlock(&lock); return -EPERM; } else state = DRAINING; mutex_unlock(&lock);
Suppose that another thread calls stop at this point before ops->drain() is called. It'll change the state to STOP, call ops->trigger(STOP). And ops->drain() (or ops->trigger(DRAIN)) may be called at the same time because there is no protection without knowing that it's being stopped.
yup...
ops->drain(substream);
mutex_lock(&lock); state = DRAINED; mutex_unlock(&lock);
Takashi
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
The reason for blocked implementation is to treat return of the call as notifcation that draining is completed.
For example user has written all the buffers, lets says worth 3 secs and now has triggered drain. User needs to wait till drain is complete before closing the device etc. So he waits on drain to compelete..
Do you have a better way to manage this?
Split the drain action in two parts, trigger and synchronization:
lock(); ... trigger(pause); while (!pause_finished) { unlock(); schedule_or_sleep_or_whatever(); lock(); } ... unlock();
with this am thinking of handling it differently now on the above lines and split the drain action and drain blocking parts. Trigger and action.
How about making the drain and partial drain triggers calls NOT blocking. This will take care of sync issues etc.
But we also need to have notification for when drain completes.
IMO this can be handled in two ways a) like in above we sleep and wake up on notification from driver. Driver can call the drain_complete() which wakes up and we return to user b) we call the driver for drain and then call blocking new callback which returns when action is complete
~Vinod --
On Tue, Aug 27, 2013 at 04:41:13PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 19:11:46 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 04:22:39PM +0200, Takashi Iwai wrote:
I understand it, but my question is what fixes your patch except for the comment in the header file? In other words, by changing the comment text, what will be fixed in the end?
the meaning and API version which tinycompress will use to send right values..
It's just a comment text, which itself is nothing bug a explanation, and doesn't define the actual behavior. And there is no code in the upstream tree regarding this field, so no place to "fix" there.
Moreover, tinycompress library should be self-contained so that it can be compiled without the kernel source tree.
yes it is and can be compiled without kernel sources...
~Vinod
--
At Tue, 27 Aug 2013 19:00:30 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 04:05:36PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 18:44:38 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 02:32:28PM +0200, Takashi Iwai wrote:
> - As disscused on our sample rate problem, lets move to use rate values and I > will fix the lib too. Since the driver are not upstream the impact of this > change wont be huge.
I see no code touching sampling_rate field.
Yes its passed directly to the drivers, where tehy use values to program decoders. Only meaning of the field is changing now.
So you're proposing a patch just changing the comment in the header file as a stable fix patch? Please reread stable_kernel_rules.txt once again.
Yes along with header version so that tinycompress can cope with it.
I've seen nothing but changing the comment in the patch. What's missing?
The meaning of the value is changing here...
Again, is this a bug fix? Stable patches are only for bug fixes.
yup... we can't do 12K, 24K decoding without this. We actually found when folks tried aac with 12K
I understand it, but my question is what fixes your patch except for the comment in the header file? In other words, by changing the comment text, what will be fixed in the end?
Takashi
On Tue, Aug 27, 2013 at 06:56:41PM +0530, Vinod Koul wrote:
- @sampling_rate: sampling rate of audio
- @sampling_rate: sampling rate value of audio, eg 12000, 48000
It's not clear why this is a change in the ABI - the values written and the types don't change. Personally I'd not change the ABI - it's a bit late now that the code has been in the kernel for several releases.
the problem is the meaning of sampling_rate. It was SND_PCM_RATE_XXX. It was a lack of foresight on my side, we should have done values which we are changing now to handle rates not defined here.
Given that the documentation didn't mention the constants and there's no example users I'm not sure this is actually a change at all...
At Tue, 27 Aug 2013 19:11:46 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 04:22:39PM +0200, Takashi Iwai wrote:
I understand it, but my question is what fixes your patch except for the comment in the header file? In other words, by changing the comment text, what will be fixed in the end?
the meaning and API version which tinycompress will use to send right values..
It's just a comment text, which itself is nothing bug a explanation, and doesn't define the actual behavior. And there is no code in the upstream tree regarding this field, so no place to "fix" there.
Moreover, tinycompress library should be self-contained so that it can be compiled without the kernel source tree.
Takashi
participants (3)
-
Mark Brown -
Takashi Iwai -
Vinod Koul