[alsa-devel] [PATCH v2 0/3] Allow suspend/resume with USB audio.
Problem being addressed: shell one$ aplay -Dhw:<usbcard> file.wav shell two$ echo mem > /sys/power/state After resume, aplay attempts resume, then restart, then after a timeout gets a write error and no audio will be played until it is restarted.
During suspend/resume, if the USB port remains powered the audio device will still be present upon resume. The interface and endpoint have to be reconfigured however. These patches move the configuration from hw_params to prepare causing re-configuration during resume.
Is this the right way to fix this? Or is there an easier way I am missing?
Thanks for looking.
v2: fix unused variable warnings.
Dylan Reid (3): ALSA: usb-audio: set period_bytes in substream. ALSA: usb-audio: Don't require hw_params in endpoint. ALSA: usb-audio: Move configuration to prepare.
sound/usb/card.h | 2 + sound/usb/endpoint.c | 29 +++++++----- sound/usb/endpoint.h | 5 ++- sound/usb/pcm.c | 121 +++++++++++++++++++++++++++++++-------------------- 4 files changed, 97 insertions(+), 60 deletions(-)
Set the peiod_bytes member of snd_usb_substream. It was no longer being set, but will be needed to resume properly in a future commit.
Change-Id: I4cf4681f16f8a71d26c586520afed9e8c27433b2 Signed-off-by: Dylan Reid dgreid@chromium.org --- sound/usb/pcm.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f782ce1..786f7a0 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -486,6 +486,8 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, }
if (changed) { + subs->period_bytes = params_period_bytes(hw_params); + mutex_lock(&subs->stream->chip->shutdown_mutex); /* format changed */ stop_endpoints(subs, 0, 0, 0);
Change the interface to configure an endpoint so that it doesn't require a hw_params struct. This will allow it to be called from prepare instead of hw_params, configuring it after system resume.
Change-Id: I9cded1644d3f6fee3c5707adffca4ef18c0befd7 Signed-off-by: Dylan Reid dgreid@chromium.org --- sound/usb/endpoint.c | 31 ++++++++++++++++++------------- sound/usb/endpoint.h | 5 ++++- sound/usb/pcm.c | 16 +++++++++++++--- 3 files changed, 35 insertions(+), 17 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index a83a18d..152bfd4 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -567,20 +567,19 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force) * configure a data endpoint */ static int data_ep_set_params(struct snd_usb_endpoint *ep, - struct snd_pcm_hw_params *hw_params, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, struct audioformat *fmt, struct snd_usb_endpoint *sync_ep) { unsigned int maxsize, i, urb_packs, total_packs, packs_per_ms; - int period_bytes = params_period_bytes(hw_params); - int format = params_format(hw_params); int is_playback = usb_pipeout(ep->pipe); - int frame_bits = snd_pcm_format_physical_width(params_format(hw_params)) * - params_channels(hw_params); + int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
ep->datainterval = fmt->datainterval; ep->stride = frame_bits >> 3; - ep->silence_value = format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0; + ep->silence_value = pcm_format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0;
/* calculate max. frequency */ if (ep->maxpacksize) { @@ -693,7 +692,6 @@ out_of_memory: * configure a sync endpoint */ static int sync_ep_set_params(struct snd_usb_endpoint *ep, - struct snd_pcm_hw_params *hw_params, struct audioformat *fmt) { int i; @@ -736,7 +734,10 @@ out_of_memory: * snd_usb_endpoint_set_params: configure an snd_usb_endpoint * * @ep: the snd_usb_endpoint to configure - * @hw_params: the hardware parameters + * @pcm_format: the audio fomat. + * @channels: the number of audio channels. + * @period_bytes: the number of bytes in one alsa period. + * @rate: the frame rate. * @fmt: the USB audio format information * @sync_ep: the sync endpoint to use, if any * @@ -745,7 +746,10 @@ out_of_memory: * An endpoint that is already running can not be reconfigured. */ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, - struct snd_pcm_hw_params *hw_params, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, + unsigned int rate, struct audioformat *fmt, struct snd_usb_endpoint *sync_ep) { @@ -765,9 +769,9 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, ep->fill_max = !!(fmt->attributes & UAC_EP_CS_ATTR_FILL_MAX);
if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_FULL) - ep->freqn = get_usb_full_speed_rate(params_rate(hw_params)); + ep->freqn = get_usb_full_speed_rate(rate); else - ep->freqn = get_usb_high_speed_rate(params_rate(hw_params)); + ep->freqn = get_usb_high_speed_rate(rate);
/* calculate the frequency in 16.16 format */ ep->freqm = ep->freqn; @@ -777,10 +781,11 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
switch (ep->type) { case SND_USB_ENDPOINT_TYPE_DATA: - err = data_ep_set_params(ep, hw_params, fmt, sync_ep); + err = data_ep_set_params(ep, pcm_format, channels, + period_bytes, fmt, sync_ep); break; case SND_USB_ENDPOINT_TYPE_SYNC: - err = sync_ep_set_params(ep, hw_params, fmt); + err = sync_ep_set_params(ep, fmt); break; default: err = -EINVAL; diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index cbbbdf2..6376ccf 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -9,7 +9,10 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, int ep_num, int direction, int type);
int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, - struct snd_pcm_hw_params *hw_params, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, + unsigned int rate, struct audioformat *fmt, struct snd_usb_endpoint *sync_ep);
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 786f7a0..62ab4fd 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -491,14 +491,24 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, mutex_lock(&subs->stream->chip->shutdown_mutex); /* format changed */ stop_endpoints(subs, 0, 0, 0); - ret = snd_usb_endpoint_set_params(subs->data_endpoint, hw_params, fmt, + ret = snd_usb_endpoint_set_params(subs->data_endpoint, + format, + channels, + subs->period_bytes, + rate, + fmt, subs->sync_endpoint); if (ret < 0) goto unlock;
if (subs->sync_endpoint) - ret = snd_usb_endpoint_set_params(subs->sync_endpoint, - hw_params, fmt, NULL); + ret = snd_usb_endpoint_set_params(subs->data_endpoint, + format, + channels, + subs->period_bytes, + rate, + fmt, + NULL); unlock: mutex_unlock(&subs->stream->chip->shutdown_mutex); }
Move interface and endpoint configuration from hw_params to prepare callback. During system suspend/resume when the USB device power isn't cycled the interface and endpoint configuration need to be set before audio playback can continue. Resume involves another call to prepare but not to hw_params, moving it here allows a playing stream to continue after resume.
Change-Id: I6a34251c1245352ff2b9f004d781afcb3bf7cda2 Signed-off-by: Dylan Reid dgreid@chromium.org --- sound/usb/card.h | 2 + sound/usb/pcm.c | 134 ++++++++++++++++++++++++++++++------------------------- 2 files changed, 76 insertions(+), 60 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 23b6f23..6cc883c 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -107,6 +107,8 @@ struct snd_usb_substream { int interface; /* current interface */ int endpoint; /* assigned endpoint */ struct audioformat *cur_audiofmt; /* current audioformat pointer (for hw_params callback) */ + snd_pcm_format_t pcm_format; /* current audio format (for hw_params callback) */ + unsigned int channels; /* current number of channels (for hw_params callback) */ unsigned int cur_rate; /* current rate (for hw_params callback) */ unsigned int period_bytes; /* current period bytes (for hw_params callback) */ unsigned int altset_idx; /* USB data format: index of alternate setting */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 62ab4fd..ae783d4 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -82,8 +82,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream /* * find a matching audio format */ -static struct audioformat *find_format(struct snd_usb_substream *subs, unsigned int format, - unsigned int rate, unsigned int channels) +static struct audioformat *find_format(struct snd_usb_substream *subs) { struct list_head *p; struct audioformat *found = NULL; @@ -92,16 +91,17 @@ static struct audioformat *find_format(struct snd_usb_substream *subs, unsigned list_for_each(p, &subs->fmt_list) { struct audioformat *fp; fp = list_entry(p, struct audioformat, list); - if (!(fp->formats & (1uLL << format))) + if (!(fp->formats & (1uLL << subs->pcm_format))) continue; - if (fp->channels != channels) + if (fp->channels != subs->channels) continue; - if (rate < fp->rate_min || rate > fp->rate_max) + if (subs->cur_rate < fp->rate_min || + subs->cur_rate > fp->rate_max) continue; if (! (fp->rates & SNDRV_PCM_RATE_CONTINUOUS)) { unsigned int i; for (i = 0; i < fp->nr_rates; i++) - if (fp->rate_table[i] == rate) + if (fp->rate_table[i] == subs->cur_rate) break; if (i >= fp->nr_rates) continue; @@ -436,6 +436,42 @@ add_sync_ep: }
/* + * configure endpoint params + * + * called during initial setup and upon resume + */ +static int configure_endpoint(struct snd_usb_substream *subs) +{ + int ret; + + mutex_lock(&subs->stream->chip->shutdown_mutex); + /* format changed */ + stop_endpoints(subs, 0, 0, 0); + ret = snd_usb_endpoint_set_params(subs->data_endpoint, + subs->pcm_format, + subs->channels, + subs->period_bytes, + subs->cur_rate, + subs->cur_audiofmt, + subs->sync_endpoint); + if (ret < 0) + goto unlock; + + if (subs->sync_endpoint) + ret = snd_usb_endpoint_set_params(subs->data_endpoint, + subs->pcm_format, + subs->channels, + subs->period_bytes, + subs->cur_rate, + subs->cur_audiofmt, + NULL); + +unlock: + mutex_unlock(&subs->stream->chip->shutdown_mutex); + return ret; +} + +/* * hw_params callback * * allocate a buffer and set the given audio format. @@ -450,75 +486,32 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, { struct snd_usb_substream *subs = substream->runtime->private_data; struct audioformat *fmt; - unsigned int channels, rate, format; - int ret, changed; + int ret;
ret = snd_pcm_lib_alloc_vmalloc_buffer(substream, params_buffer_bytes(hw_params)); if (ret < 0) return ret;
- format = params_format(hw_params); - rate = params_rate(hw_params); - channels = params_channels(hw_params); - fmt = find_format(subs, format, rate, channels); + subs->pcm_format = params_format(hw_params); + subs->period_bytes = params_period_bytes(hw_params); + subs->channels = params_channels(hw_params); + subs->cur_rate = params_rate(hw_params); + + fmt = find_format(subs); if (!fmt) { snd_printd(KERN_DEBUG "cannot set format: format = %#x, rate = %d, channels = %d\n", - format, rate, channels); + subs->pcm_format, subs->cur_rate, subs->channels); return -EINVAL; }
- changed = subs->cur_audiofmt != fmt || - subs->period_bytes != params_period_bytes(hw_params) || - subs->cur_rate != rate; if ((ret = set_format(subs, fmt)) < 0) return ret;
- if (subs->cur_rate != rate) { - struct usb_host_interface *alts; - struct usb_interface *iface; - iface = usb_ifnum_to_if(subs->dev, fmt->iface); - alts = &iface->altsetting[fmt->altset_idx]; - ret = snd_usb_init_sample_rate(subs->stream->chip, fmt->iface, alts, fmt, rate); - if (ret < 0) - return ret; - subs->cur_rate = rate; - } - - if (changed) { - subs->period_bytes = params_period_bytes(hw_params); + subs->interface = fmt->iface; + subs->altset_idx = fmt->altset_idx;
- mutex_lock(&subs->stream->chip->shutdown_mutex); - /* format changed */ - stop_endpoints(subs, 0, 0, 0); - ret = snd_usb_endpoint_set_params(subs->data_endpoint, - format, - channels, - subs->period_bytes, - rate, - fmt, - subs->sync_endpoint); - if (ret < 0) - goto unlock; - - if (subs->sync_endpoint) - ret = snd_usb_endpoint_set_params(subs->data_endpoint, - format, - channels, - subs->period_bytes, - rate, - fmt, - NULL); -unlock: - mutex_unlock(&subs->stream->chip->shutdown_mutex); - } - - if (ret == 0) { - subs->interface = fmt->iface; - subs->altset_idx = fmt->altset_idx; - } - - return ret; + return 0; }
/* @@ -549,6 +542,9 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_usb_substream *subs = runtime->private_data; + struct usb_host_interface *alts; + struct usb_interface *iface; + int ret;
if (! subs->cur_audiofmt) { snd_printk(KERN_ERR "usbaudio: no format is specified!\n"); @@ -558,6 +554,24 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) if (snd_BUG_ON(!subs->data_endpoint)) return -EIO;
+ ret = set_format(subs, subs->cur_audiofmt); + if (ret < 0) + return ret; + + iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface); + alts = &iface->altsetting[subs->cur_audiofmt->altset_idx]; + ret = snd_usb_init_sample_rate(subs->stream->chip, + subs->cur_audiofmt->iface, + alts, + subs->cur_audiofmt, + subs->cur_rate); + if (ret < 0) + return ret; + + ret = configure_endpoint(subs); + if (ret < 0) + return ret; + /* some unit conversions in runtime */ subs->data_endpoint->maxframesize = bytes_to_frames(runtime, subs->data_endpoint->maxpacksize);
At Tue, 18 Sep 2012 09:49:45 -0700, Dylan Reid wrote:
Problem being addressed: shell one$ aplay -Dhw:<usbcard> file.wav shell two$ echo mem > /sys/power/state After resume, aplay attempts resume, then restart, then after a timeout gets a write error and no audio will be played until it is restarted.
During suspend/resume, if the USB port remains powered the audio device will still be present upon resume. The interface and endpoint have to be reconfigured however. These patches move the configuration from hw_params to prepare causing re-configuration during resume.
Is this the right way to fix this? Or is there an easier way I am missing?
Thanks for looking.
v2: fix unused variable warnings.
I think the patches are OK. If no big objection comes up, I'm going to apply it tomorrow.
Meanwhile I found a problem in my patch. The re-setup might be needed if the app changes hw_params again. So, the flag has to be set in hw_params callback instead. The revised patch is below.
thanks,
Takashi
===
From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: usb-audio: Avoid unnecessary EP setups in prepare
The recent fix for USB suspend breakage moved the code to set up EP from hw_params to prepare, but it means also the EP setup might be called multiple times unnecessarily because the prepare callback can be called multiple times without starting the stream (e.g. OSS emulation).
This patch adds a new flag to struct snd_usb_substream indicating whether the setup of EP is required, and do it only when necessary, i.e. right after hw_params or suspend.
Signed-off-by: Takashi Iwai tiwai@suse.de --- v1->v2: set the flag in hw_params callback
sound/usb/card.c | 2 ++ sound/usb/card.h | 1 + sound/usb/pcm.c | 10 +++++++--- 3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/sound/usb/card.c b/sound/usb/card.c index 4a469f0..561bb74 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -646,6 +646,8 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message) list_for_each(p, &chip->pcm_list) { as = list_entry(p, struct snd_usb_stream, list); snd_pcm_suspend_all(as->pcm); + as->substream[0].need_setup_ep = + as->substream[1].need_setup_ep = true; } } } else { diff --git a/sound/usb/card.h b/sound/usb/card.h index 6cc883c..afa4f9e 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -125,6 +125,7 @@ struct snd_usb_substream { struct snd_usb_endpoint *data_endpoint; struct snd_usb_endpoint *sync_endpoint; unsigned long flags; + bool need_setup_ep; /* (re)configure EP at prepare? */
u64 formats; /* format bitmasks (all or'ed) */ unsigned int num_formats; /* number of supported audio formats (list) */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index ae783d4..55e19e1 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -510,6 +510,7 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
subs->interface = fmt->iface; subs->altset_idx = fmt->altset_idx; + subs->need_setup_ep = true;
return 0; } @@ -568,9 +569,12 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) if (ret < 0) return ret;
- ret = configure_endpoint(subs); - if (ret < 0) - return ret; + if (subs->need_setup_ep) { + ret = configure_endpoint(subs); + if (ret < 0) + return ret; + subs->need_setup_ep = false; + }
/* some unit conversions in runtime */ subs->data_endpoint->maxframesize =
On Tue, Sep 18, 2012 at 12:24 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 18 Sep 2012 09:49:45 -0700, Dylan Reid wrote:
Problem being addressed: shell one$ aplay -Dhw:<usbcard> file.wav shell two$ echo mem > /sys/power/state After resume, aplay attempts resume, then restart, then after a timeout gets a write error and no audio will be played until it is restarted.
During suspend/resume, if the USB port remains powered the audio device will still be present upon resume. The interface and endpoint have to be reconfigured however. These patches move the configuration from hw_params to prepare causing re-configuration during resume.
Is this the right way to fix this? Or is there an easier way I am missing?
Thanks for looking.
v2: fix unused variable warnings.
I think the patches are OK. If no big objection comes up, I'm going to apply it tomorrow.
Meanwhile I found a problem in my patch. The re-setup might be needed if the app changes hw_params again. So, the flag has to be set in hw_params callback instead. The revised patch is below.
Good catch. I wrote a quick test that plays something, calls drain, calls hw_params to a different rate, then start and plays again. It failed with the old patch and works with the new one.
Thanks,
Dylan
thanks,
Takashi
===
From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: usb-audio: Avoid unnecessary EP setups in prepare
The recent fix for USB suspend breakage moved the code to set up EP from hw_params to prepare, but it means also the EP setup might be called multiple times unnecessarily because the prepare callback can be called multiple times without starting the stream (e.g. OSS emulation).
This patch adds a new flag to struct snd_usb_substream indicating whether the setup of EP is required, and do it only when necessary, i.e. right after hw_params or suspend.
Signed-off-by: Takashi Iwai tiwai@suse.de
v1->v2: set the flag in hw_params callback
sound/usb/card.c | 2 ++ sound/usb/card.h | 1 + sound/usb/pcm.c | 10 +++++++--- 3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/sound/usb/card.c b/sound/usb/card.c index 4a469f0..561bb74 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -646,6 +646,8 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message) list_for_each(p, &chip->pcm_list) { as = list_entry(p, struct snd_usb_stream, list); snd_pcm_suspend_all(as->pcm);
as->substream[0].need_setup_ep =as->substream[1].need_setup_ep = true; } } } else {diff --git a/sound/usb/card.h b/sound/usb/card.h index 6cc883c..afa4f9e 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -125,6 +125,7 @@ struct snd_usb_substream { struct snd_usb_endpoint *data_endpoint; struct snd_usb_endpoint *sync_endpoint; unsigned long flags;
bool need_setup_ep; /* (re)configure EP at prepare? */ u64 formats; /* format bitmasks (all or'ed) */ unsigned int num_formats; /* number of supported audio formats (list) */diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index ae783d4..55e19e1 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -510,6 +510,7 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
subs->interface = fmt->iface; subs->altset_idx = fmt->altset_idx;
subs->need_setup_ep = true; return 0;} @@ -568,9 +569,12 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) if (ret < 0) return ret;
ret = configure_endpoint(subs);if (ret < 0)return ret;
if (subs->need_setup_ep) {ret = configure_endpoint(subs);if (ret < 0)return ret;subs->need_setup_ep = false;} /* some unit conversions in runtime */ subs->data_endpoint->maxframesize =-- 1.7.11.5
participants (2)
-
Dylan Reid -
Takashi Iwai