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

[Dylan Reid]

On Tue, Sep 18, 2012 at 5:59 AM, Takashi Iwai <tiwai at 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 at 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 at 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 at 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
>