by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/4] ALSA: usb-audio: Initial Power Domain support

Hi Jorge,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sound/for-next]
[also build test ERROR on v4.18-rc5 next-20180719]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/usb-audio-Add-UAC3-Power-Domains/20180719-212009
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm

All error/warnings (new ones prefixed by >>):

sound/usb/power.c: In function 'snd_usb_find_power_domain':
>> sound/usb/power.c:36:7: error: implicit declaration of function 'kzalloc'; did you mean 'd_alloc'? [-Werror=implicit-function-declaration]
pd = kzalloc(sizeof(*pd), GFP_KERNEL);
^~~~~~~
d_alloc
>> sound/usb/power.c:36:5: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
pd = kzalloc(sizeof(*pd), GFP_KERNEL);
^
>> sound/usb/power.c:59:2: error: implicit declaration of function 'kfree' [-Werror=implicit-function-declaration]
kfree(pd);
^~~~~
cc1: some warnings being treated as errors

vim +36 sound/usb/power.c

28
29 struct snd_usb_power_domain *
30 snd_usb_find_power_domain(struct usb_host_interface *ctrl_iface,
31 unsigned char id)
32 {
33 struct snd_usb_power_domain *pd;
34 void *p;
35
> 36 pd = kzalloc(sizeof(*pd), GFP_KERNEL);
37 if (!pd)
38 return NULL;
39
40 p = NULL;
41 while ((p = snd_usb_find_csint_desc(ctrl_iface->extra,
42 ctrl_iface->extralen,
43 p, UAC3_POWER_DOMAIN)) != NULL) {
44 struct uac3_power_domain_descriptor *pd_desc = p;
45 int i;
46
47 for (i = 0; i < pd_desc->bNrEntities; i++) {
48 if (pd_desc->baEntityID[i] == id) {
49 pd->pd_id = pd_desc->bPowerDomainID;
50 pd->pd_d1d0_rec =
51 le16_to_cpu(pd_desc->waRecoveryTime1);
52 pd->pd_d2d0_rec =
53 le16_to_cpu(pd_desc->waRecoveryTime2);
54 return pd;
55 }
56 }
57 }
58
> 59 kfree(pd);
60 return NULL;
61 }
62

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (2.69 kB)
.config.gz (42.87 kB)
Download all attachments

2018-07-19 17:12:08

Make use of UAC3 Power Domains associated to an Audio Streaming
path within the PCM's logic. This means, when there is no audio
being transferred (pcm is closed), the host will set the Power Domain
associated to that substream to state D1. When audio is being transferred
(from hw_params onwards), the Power Domain will be set to D0 state.

This is the way the host lets the device know which Terminal
is going to be actively used and it is for the device to
manage its own internal resources on that UAC3 Power Domain.

Note the resume method now sets the Power Domain to D1 state as
resuming the device doesn't mean audio streaming will occur.

Signed-off-by: Jorge Sanjuan <[email protected]>
---
sound/usb/pcm.c | 24 +++++++++++++++++++-----
sound/usb/stream.c | 6 +++++-
2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 99ec9d5caa58..bbc7116c9543 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -748,11 +748,11 @@ int snd_usb_pcm_resume(struct snd_usb_stream *as)
{
int ret;

- ret = snd_usb_pcm_change_state(&as->substream[0], UAC3_PD_STATE_D0);
+ ret = snd_usb_pcm_change_state(&as->substream[0], UAC3_PD_STATE_D1);
if (ret < 0)
return ret;

- ret = snd_usb_pcm_change_state(&as->substream[1], UAC3_PD_STATE_D0);
+ ret = snd_usb_pcm_change_state(&as->substream[1], UAC3_PD_STATE_D1);
if (ret < 0)
return ret;

@@ -803,16 +803,22 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
ret = snd_usb_lock_shutdown(subs->stream->chip);
if (ret < 0)
return ret;
+
+ ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D0);
+ if (ret < 0)
+ goto unlock;
+
ret = set_format(subs, fmt);
- snd_usb_unlock_shutdown(subs->stream->chip);
if (ret < 0)
- return ret;
+ goto unlock;

subs->interface = fmt->iface;
subs->altset_idx = fmt->altset_idx;
subs->need_setup_ep = true;

- return 0;
+ unlock:
+ snd_usb_unlock_shutdown(subs->stream->chip);
+ return ret;
}

/*
@@ -869,6 +875,10 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
snd_usb_endpoint_sync_pending_stop(subs->sync_endpoint);
snd_usb_endpoint_sync_pending_stop(subs->data_endpoint);

+ ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D0);
+ if (ret < 0)
+ goto unlock;
+
ret = set_format(subs, subs->cur_audiofmt);
if (ret < 0)
goto unlock;
@@ -1313,6 +1323,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
int direction = substream->stream;
struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
struct snd_usb_substream *subs = &as->substream[direction];
+ int ret;

stop_endpoints(subs, true);

@@ -1321,7 +1332,10 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
!snd_usb_lock_shutdown(subs->stream->chip)) {
usb_set_interface(subs->dev, subs->interface, 0);
subs->interface = -1;
+ ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D1);
snd_usb_unlock_shutdown(subs->stream->chip);
+ if (ret < 0)
+ return ret;
}

subs->pcm_substream = NULL;
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index c0567fa1e84b..8fe3b0e00e45 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -110,8 +110,12 @@ static void snd_usb_init_substream(struct snd_usb_stream *as,
if (fp->channels > subs->channels_max)
subs->channels_max = fp->channels;

- if (pd)
+ if (pd) {
subs->str_pd = pd;
+ /* Initialize Power Domain to idle status D1 */
+ snd_usb_power_domain_set(subs->stream->chip, pd,
+ UAC3_PD_STATE_D1);
+ }

snd_usb_preallocate_buffer(subs);
}
--
2.11.0

2018-07-31 13:13:30

by Takashi Iwai

[permalink] [raw]

Subject: Re: [PATCH v3 0/4] usb-audio: Add UAC3 Power Domains

On Tue, 31 Jul 2018 14:28:41 +0200,
Jorge Sanjuan wrote:
>
> This is what's new in this v3:
> - Add proper SPDX identifier for new source file.
> - Add delay unit in comment for BADD inferred recovery
> times and specify the actual delay time for them.
> - Suspend the usb stream *after* alsa has supended its stuff.
> - Make sure Power State changes to D0 happen *before* stream
> format is set.
> - Try to set the Power State to D0 on .prepare callback too as
> recovery from suspend state doesn't need .hw_params call.
>
> This patchset adds support for UAC3 Power Domains. This feature
> of the USB audio class 3 allows the host to notify the device
> what it is making use of so power comsumption can be optimized.
>
> This proposal implements this feature for Power Domains
> that include an Input/Output Terminal associated to an
> audio Streaming interface. This is the main usage of this
> feature according to the spec. For that reason, the logic
> for the Power Domain state change has been implemented
> within the ALSA PCMs logic and the suspend/resume callbacks
> of the usb_driver. The behaviour would be as follows:
>
> * Power Domain State D0: A Power Domain will reach this state
> only when the audio substream associated to that domain is
> being used (i,e. Audio playback/capture is happening).
> * Power Domain State D1: This is the Idle state where the driver
> is going to always want to be in order to reduce power
> consumption.
> * Power Domain State D2: This state is only set when the usb driver
> asumes the device is not going to be used anymore and hence, it
> wont care about getting any interrupts from the device. This
> will only happen when power level is set to "auto" in sysfs
> so the usb driver gets suspended when the interfaces are not in use.
>
> NOTE: The way this has been implemented will always try to put the
> Power Domain in state D1 if the Power Domain exists. The patch
> "ALSA: usb-audio: Operate UAC3 Power Domains in PCM callbacks"
> puts the logic for doing so inside the PCM's logic. Something to
> improve on that is to also tie up those D1<->D0 state changes
> to runtime PM maybe.
>
>
> Jorge Sanjuan (4):
> ALSA: usb-audio: Initial Power Domain support
> ALSA: usb-audio: AudioStreaming Power Domain parsing
> ALSA: usb-audio: Add UAC3 Power Domains to suspend/resume
> ALSA: usb-audio: Operate UAC3 Power Domains in PCM callbacks

Now I merge all these four patches. Thanks!

Takashi