| Message ID | 1460384855-10567-3-git-send-email-ckeepax@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 11 Apr 2016 16:27:33 +0200, Charles Keepax wrote: > > Currently, the avail IOCTL doesn't pass any error status, which > means typically on error it simply shows no data available. This > can lead to situations where user-space is waiting indefinitely > for data that will never come as the DSP has suffered an > unrecoverable error. > > Add snd_compr_stop_error which end drivers can call to indicate > the stream has suffered an unrecoverable error and stop it. The > avail and poll IOCTLs are then updated to report if the stream is > in an error state to user-space. Allowing the error to propagate > out. Processing of the actual snd_compr_stop needs to be deferred > to a worker thread as the end driver may detect the errors during > an existing operation callback. > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> > --- > include/sound/compress_driver.h | 4 +++ > sound/core/compress_offload.c | 60 ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h > index c0abcdc..a057038 100644 > --- a/include/sound/compress_driver.h > +++ b/include/sound/compress_driver.h > @@ -78,6 +78,7 @@ struct snd_compr_stream { > struct snd_compr_ops *ops; > struct snd_compr_runtime *runtime; > struct snd_compr *device; > + struct delayed_work error_work; > enum snd_compr_direction direction; > bool metadata_set; > bool next_track; > @@ -187,4 +188,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream) > wake_up(&stream->runtime->sleep); > } > > +int snd_compr_stop_error(struct snd_compr_stream *stream, > + snd_pcm_state_t state); > + > #endif > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > index 507071d..86d1d85 100644 > --- a/sound/core/compress_offload.c > +++ b/sound/core/compress_offload.c > @@ -67,6 +67,8 @@ struct snd_compr_file { > struct snd_compr_stream stream; > }; > > +static void error_delayed_work(struct work_struct *work); > + > /* > * a note on stream states used: > * we use following states in the compressed core > @@ -123,6 +125,9 @@ static int snd_compr_open(struct inode *inode, struct file *f) > snd_card_unref(compr->card); > return -ENOMEM; > } > + > + INIT_DELAYED_WORK(&data->stream.error_work, error_delayed_work); > + > data->stream.ops = compr->ops; > data->stream.direction = dirn; > data->stream.private_data = compr->private_data; > @@ -153,6 +158,8 @@ static int snd_compr_free(struct inode *inode, struct file *f) > struct snd_compr_file *data = f->private_data; > struct snd_compr_runtime *runtime = data->stream.runtime; > > + cancel_delayed_work_sync(&data->stream.error_work); > + > switch (runtime->state) { > case SNDRV_PCM_STATE_RUNNING: > case SNDRV_PCM_STATE_DRAINING: > @@ -237,6 +244,14 @@ snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg) > avail = snd_compr_calc_avail(stream, &ioctl_avail); > ioctl_avail.avail = avail; > > + switch (stream->runtime->state) { > + case SNDRV_PCM_STATE_OPEN: > + case SNDRV_PCM_STATE_XRUN: > + return -EBADFD; One question is whether we want a dedicated error code for XRUN or such a DSP error. On PCM, for example, we return -EPIPE traditionally for XRUN state. This is a clear indicator for user what to do at next. Other than that, the patch series looks good to me. thanks, Takashi
On Mon, Apr 11, 2016 at 04:41:23PM +0200, Takashi Iwai wrote: > On Mon, 11 Apr 2016 16:27:33 +0200, > Charles Keepax wrote: > > + switch (stream->runtime->state) { > > + case SNDRV_PCM_STATE_OPEN: > > + case SNDRV_PCM_STATE_XRUN: > > + return -EBADFD; > > One question is whether we want a dedicated error code for XRUN or > such a DSP error. On PCM, for example, we return -EPIPE traditionally > for XRUN state. This is a clear indicator for user what to do at > next. > > Other than that, the patch series looks good to me. I think it probably makes sense to copy what the PCM framework does here, I will respin and use EPIPE. Thanks, Charles
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index c0abcdc..a057038 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -78,6 +78,7 @@ struct snd_compr_stream { struct snd_compr_ops *ops; struct snd_compr_runtime *runtime; struct snd_compr *device; + struct delayed_work error_work; enum snd_compr_direction direction; bool metadata_set; bool next_track; @@ -187,4 +188,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream) wake_up(&stream->runtime->sleep); } +int snd_compr_stop_error(struct snd_compr_stream *stream, + snd_pcm_state_t state); + #endif diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 507071d..86d1d85 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -67,6 +67,8 @@ struct snd_compr_file { struct snd_compr_stream stream; }; +static void error_delayed_work(struct work_struct *work); + /* * a note on stream states used: * we use following states in the compressed core @@ -123,6 +125,9 @@ static int snd_compr_open(struct inode *inode, struct file *f) snd_card_unref(compr->card); return -ENOMEM; } + + INIT_DELAYED_WORK(&data->stream.error_work, error_delayed_work); + data->stream.ops = compr->ops; data->stream.direction = dirn; data->stream.private_data = compr->private_data; @@ -153,6 +158,8 @@ static int snd_compr_free(struct inode *inode, struct file *f) struct snd_compr_file *data = f->private_data; struct snd_compr_runtime *runtime = data->stream.runtime; + cancel_delayed_work_sync(&data->stream.error_work); + switch (runtime->state) { case SNDRV_PCM_STATE_RUNNING: case SNDRV_PCM_STATE_DRAINING: @@ -237,6 +244,14 @@ snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg) avail = snd_compr_calc_avail(stream, &ioctl_avail); ioctl_avail.avail = avail; + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_OPEN: + case SNDRV_PCM_STATE_XRUN: + return -EBADFD; + default: + break; + } + if (copy_to_user((__u64 __user *)arg, &ioctl_avail, sizeof(ioctl_avail))) return -EFAULT; @@ -400,10 +415,16 @@ 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_OPEN) { + + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_OPEN: + case SNDRV_PCM_STATE_XRUN: retval = -EBADFD; goto out; + default: + break; } + poll_wait(f, &stream->runtime->sleep, wait); avail = snd_compr_get_avail(stream); @@ -423,6 +444,9 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait) if (avail >= stream->runtime->fragment_size) retval = snd_compr_get_poll(stream); break; + case SNDRV_PCM_STATE_XRUN: + retval = -EBADFD; + break; default: if (stream->direction == SND_COMPRESS_PLAYBACK) retval = POLLOUT | POLLWRNORM | POLLERR; @@ -701,6 +725,40 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; } +static void error_delayed_work(struct work_struct *work) +{ + struct snd_compr_stream *stream; + + stream = container_of(work, struct snd_compr_stream, error_work.work); + + mutex_lock(&stream->device->lock); + snd_compr_stop(stream); + mutex_unlock(&stream->device->lock); +} + +/* + * snd_compr_stop_error: Report a fatal error on a stream + * @stream: pointer to stream + * @state: state to transition the stream to + * + * Stop the stream and set its state. + * + * Should be called with compressed device lock held. + */ +int snd_compr_stop_error(struct snd_compr_stream *stream, + snd_pcm_state_t state) +{ + if (stream->runtime->state == state) + return 0; + + stream->runtime->state = state; + + queue_delayed_work(system_power_efficient_wq, &stream->error_work, 0); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_compr_stop_error); + static int snd_compress_wait_for_drain(struct snd_compr_stream *stream) { int ret;
Currently, the avail IOCTL doesn't pass any error status, which means typically on error it simply shows no data available. This can lead to situations where user-space is waiting indefinitely for data that will never come as the DSP has suffered an unrecoverable error. Add snd_compr_stop_error which end drivers can call to indicate the stream has suffered an unrecoverable error and stop it. The avail and poll IOCTLs are then updated to report if the stream is in an error state to user-space. Allowing the error to propagate out. Processing of the actual snd_compr_stop needs to be deferred to a worker thread as the end driver may detect the errors during an existing operation callback. Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> --- include/sound/compress_driver.h | 4 +++ sound/core/compress_offload.c | 60 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-)