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

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

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