[PATCH 0/3] Allow suspend/resume with USB audio.

[PATCH 0/3] Allow suspend/resume with USB audio.

[Dylan Reid]

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.

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(-)

-- 
1.7.12.146.g16d26b1

[PATCH 1/3] ALSA: usb-audio: set period_bytes in substream.

[Dylan Reid]

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);
-- 
1.7.12.146.g16d26b1

[PATCH 2/3] ALSA: usb-audio: Don't require hw_params in endpoint.

[Dylan Reid]

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);
 	}
-- 
1.7.12.146.g16d26b1

[PATCH 3/3] ALSA: usb-audio: Move configuration to prepare.

[Dylan Reid]

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  | 133 +++++++++++++++++++++++++++++++------------------------
 2 files changed, 76 insertions(+), 59 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..d19ff07 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.
@@ -451,74 +487,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, 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 +543,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 +555,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);
-- 
1.7.12.146.g16d26b1

Re: [PATCH 3/3] ALSA: usb-audio: Move configuration to prepare.

[Takashi Iwai]

At Mon, 17 Sep 2012 13:11:39 -0700,
Dylan Reid wrote:
> 
> 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>

I don't think we need to set up EP unconditionally in the prepare.
For normal operations, setting up in hw_params makes more sense, as
it's necessary done only once.

So, maybe better to add some flag indicating that the re-setup is
needed in prepare, and set it in the suspend callback.


Takashi

> ---
>  sound/usb/card.h |   2 +
>  sound/usb/pcm.c  | 133 +++++++++++++++++++++++++++++++------------------------
>  2 files changed, 76 insertions(+), 59 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..d19ff07 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.
> @@ -451,74 +487,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, 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 +543,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 +555,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);
> -- 
> 1.7.12.146.g16d26b1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 3/3] ALSA: usb-audio: Move configuration to prepare.

[Takashi Iwai]

At Tue, 18 Sep 2012 10:20:43 +0200,
Takashi Iwai wrote:
> 
> At Mon, 17 Sep 2012 13:11:39 -0700,
> Dylan Reid wrote:
> > 
> > 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>
> 
> I don't think we need to set up EP unconditionally in the prepare.
> For normal operations, setting up in hw_params makes more sense, as
> it's necessary done only once.
> 
> So, maybe better to add some flag indicating that the re-setup is
> needed in prepare, and set it in the suspend callback.

I meant the patch like below.


Takashi

===

From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] 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 open or suspend.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 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 d19ff07..ceb3342 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -569,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 =
@@ -983,6 +986,7 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
 	runtime->hw = snd_usb_hardware;
 	runtime->private_data = subs;
 	subs->pcm_substream = substream;
+	subs->need_setup_ep = true;
 	/* runtime PM is also done there */
 	return setup_hw_info(runtime, subs);
 }
-- 
1.7.11.5

Re: [PATCH 3/3] ALSA: usb-audio: Move configuration to prepare.

[Dylan Reid]

On Tue, Sep 18, 2012 at 5:59 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Tue, 18 Sep 2012 10:20:43 +0200,
> Takashi Iwai wrote:
>>
>> At Mon, 17 Sep 2012 13:11:39 -0700,
>> Dylan Reid wrote:
>> >
>> > 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>
>>
>> I don't think we need to set up EP unconditionally in the prepare.
>> For normal operations, setting up in hw_params makes more sense, as
>> it's necessary done only once.
>>
>> So, maybe better to add some flag indicating that the re-setup is
>> needed in prepare, and set it in the suspend callback.
>
> I meant the patch like below.

I tested this on two systems, works great, thanks for the quick review
and optimization!


>
>
> Takashi
>
> ===
>
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] 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 open or suspend.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  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 d19ff07..ceb3342 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -569,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 =
> @@ -983,6 +986,7 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
>         runtime->hw = snd_usb_hardware;
>         runtime->private_data = subs;
>         subs->pcm_substream = substream;
> +       subs->need_setup_ep = true;
>         /* runtime PM is also done there */
>         return setup_hw_info(runtime, subs);
>  }
> --
> 1.7.11.5
>

Re: [PATCH 3/3] ALSA: usb-audio: Move configuration to prepare.

[Takashi Iwai]

At Mon, 17 Sep 2012 13:11:39 -0700,
Dylan Reid wrote:
> 
> @@ -451,74 +487,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;

Need to delete this line, as these variables won't be used any longer.

sound/usb/pcm.c: In function ‘snd_usb_hw_params’:
sound/usb/pcm.c:494:31: warning: unused variable ‘format’ [-Wunused-variable]
sound/usb/pcm.c:494:15: warning: unused variable ‘channels’ [-Wunused-variable]
sound/usb/pcm.c:509:15: warning: ‘rate’ may be used uninitialized in this function [-Wuninitialized]



Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] ALSA: usb-audio: Move configuration to prepare.

[Dylan Reid]

On Tue, Sep 18, 2012 at 5:47 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 17 Sep 2012 13:11:39 -0700,
> Dylan Reid wrote:
>>
>> @@ -451,74 +487,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;
>
> Need to delete this line, as these variables won't be used any longer.
>
> sound/usb/pcm.c: In function ‘snd_usb_hw_params’:
> sound/usb/pcm.c:494:31: warning: unused variable ‘format’ [-Wunused-variable]
> sound/usb/pcm.c:494:15: warning: unused variable ‘channels’ [-Wunused-variable]
> sound/usb/pcm.c:509:15: warning: ‘rate’ may be used uninitialized in this function [-Wuninitialized]

Sorry about that, thanks for catching it, I'll send an updated series.

>
>
>
> Takashi