[alsa-devel] [PATCH v2 0/2] USB audio fixes

older
[alsa-devel] BugReport: Dice works...

Pierre-Louis Bossart

15 Aug 2015 15 Aug '15

12:19 a.m.

While trying to understand how asynchronous endpoints are supported, I identified two major issues. On my M-Audio transit, the following script tries to play using the 3 different endpoints supported and fails (error and no audio) after the first playback without the patches. With the patches on top of 4.1.5 or Takashi's for-next (4.2.0-rc5), I stopped the audio after an hour.

while true do aplay -d5 -Dhw:2,0 ~/16_48.wav # play 48kHz, 16 bit on async ep aplay -d5 -Dhw:2,0 ~/24_48.wav # play 48kHz, 24 bit on 'none'/async ep aplay -d5 -Dhw:2,0 ~/24_96.wav # play 96 kHz, 24 bit on adaptive ep aplay -d5 -Dhw:2,0 ~/24_48.wav aplay -d5 -Dhw:2,0 ~/16_48.wav aplay -d5 -Dhw:2,0 ~/24_96.wav done

The first issue is pretty bad with corrupted pointers. When transitioning from a alternate setting with a sync_endpoint to another which doesn't have one by construction (synchronous, adaptive, async on capture), the pointers are not reset and the code tries to set the parameters of a non-existent endpoint.

The second issue is that the M-audio Transit descriptors are illegal but the logic can be changed without harm.

I sometimes hear noise on playback start, probably the code needs to be changed to add enough silent samples to let the PLL lock. I haven't had time to work on this and this is more of an enhancement.

v2: applied Takashi's comments unconditional clean-up of sync_endpoint pointers better logical tests and removal of goto

Pierre-Louis Bossart (2): ALSA: usb: fix corrupted pointers due to interface setting change ALSA: usb: handle descriptor with SYNC_NONE illegal value

sound/usb/pcm.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)

-- 1.9.1

Show replies by date

Pierre-Louis Bossart

15 Aug 15 Aug

12:19 a.m.

New subject: [alsa-devel] [PATCH v2 1/2] ALSA: usb: fix corrupted pointers due to interface setting change

When a transition occurs between alternate settings that do not use the same synchronization method, the substream pointers were not reset. This prevents audio from being played during the second transition.

Identified and tested with M-Audio Transit device (0763:2006 Midiman M-Audio Transit)

Details of the issue:

First playback to adaptive endpoint: $ aplay -Dhw:1,0 ~/24_96.wav Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes, Rate 96000 Hz, Stereo

[ 3169.297556] usb 1-2: setting usb interface 1:1 [ 3169.297568] usb 1-2: Creating new playback data endpoint #3 [ 3169.298563] usb 1-2: Setting params for ep #3 (type 0, 3 urbs), ret=0 [ 3169.298574] usb 1-2: Starting data EP @ffff880035fc8000

first playback to asynchronous endpoint: $ aplay -Dhw:1,0 ~/16_48.wav Playing WAVE '/home/plb/16_48.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo

[ 3204.520251] usb 1-2: setting usb interface 1:3 [ 3204.520264] usb 1-2: Creating new playback data endpoint #3 [ 3204.520272] usb 1-2: Creating new capture sync endpoint #83 [ 3204.521162] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0 [ 3204.521177] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0 [ 3204.521182] usb 1-2: Starting data EP @ffff880035fce000 [ 3204.521204] usb 1-2: Starting sync EP @ffff8800bd616000

second playback to adaptive endpoint: no audio and error on terminal: $ aplay -Dhw:1,0 ~/24_96.wav Playing WAVE '/home/plb/24_96.wav' : Signed 24 bit Little Endian in 3bytes, Rate 96000 Hz, Stereo aplay: pcm_write:1939: write error: Input/output error

[ 3239.483589] usb 1-2: setting usb interface 1:1 [ 3239.483601] usb 1-2: Re-using EP 3 in iface 1,1 @ffff880035fc8000 [ 3239.484590] usb 1-2: Setting params for ep #3 (type 0, 4 urbs), ret=0 [ 3239.484606] usb 1-2: Setting params for ep #83 (type 1, 4 urbs), ret=0

This last line shows that a sync endpoint is used when it shouldn't. The sync endpoint is no longer valid and the pointers are corrupted

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/usb/pcm.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index b4ef410..7fb17c8 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -391,6 +391,20 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, */ attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;

+ if ((is_playback && (attr != USB_ENDPOINT_SYNC_ASYNC)) || + (!is_playback && (attr != USB_ENDPOINT_SYNC_ADAPTIVE))) { + + /* + * In these modes the notion of sync_endpoint is irrelevant. + * Reset pointers to avoid using stale data from previously + * used settings, e.g. when configuration and endpoints were + * changed + */ + + subs->sync_endpoint = NULL; + subs->data_endpoint->sync_master = NULL; + } + err = set_sync_ep_implicit_fb_quirk(subs, dev, altsd, attr); if (err < 0) return err;

-- 1.9.1

Pierre-Louis Bossart

12:19 a.m.

New subject: [alsa-devel] [PATCH v2 2/2] ALSA: usb: handle descriptor with SYNC_NONE illegal value

The M-Audio Transit exposes an interface with a SYNC_NONE attribute. This is not a valid value according to the USB audio classspec. However there is a sync endpoint associated to this record. Changing the logic to try to use this sync endpoint allows for seamless transitions between altset 2 and altset 3. If any errors happen, the behavior remains the same.

$ more /proc/asound/card1/stream0 M-Audio Transit USB at usb-0000:00:14.0-2, full speed : USB Audio

Playback: Status: Stop Interface 1 Altset 1 Format: S24_3LE Channels: 2 Endpoint: 3 OUT (ADAPTIVE) Rates: 48001 - 96000 (continuous) Interface 1 Altset 2 Format: S24_3LE Channels: 2 Endpoint: 3 OUT (NONE) Rates: 8000 - 48000 (continuous) Interface 1 Altset 3 Format: S16_LE Channels: 2 Endpoint: 3 OUT (ASYNC) Rates: 8000 - 48000 (continuous)

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/usb/pcm.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 7fb17c8..3079726 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -412,10 +412,17 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, if (altsd->bNumEndpoints < 2) return 0;

- if ((is_playback && attr != USB_ENDPOINT_SYNC_ASYNC) || + if ((is_playback && (attr == USB_ENDPOINT_SYNC_SYNC || + attr == USB_ENDPOINT_SYNC_ADAPTIVE)) || (!is_playback && attr != USB_ENDPOINT_SYNC_ADAPTIVE)) return 0;

+ /* + * In case of illegal SYNC_NONE for OUT endpoint, we keep going to see + * if we don't find a sync endpoint, as on M-Audio Transit. In case of + * error fall back to SYNC mode and don't create sync endpoint + */ + /* check sync-pipe endpoint */ /* ... and check descriptor size before accessing bSynchAddress because there is a version of the SB Audigy 2 NX firmware lacking @@ -429,6 +436,8 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, get_endpoint(alts, 1)->bmAttributes, get_endpoint(alts, 1)->bLength, get_endpoint(alts, 1)->bSynchAddress); + if (is_playback && attr == USB_ENDPOINT_SYNC_NONE) + return 0; return -EINVAL; } ep = get_endpoint(alts, 1)->bEndpointAddress; @@ -439,6 +448,8 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, "%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n", fmt->iface, fmt->altsetting, is_playback, ep, get_endpoint(alts, 0)->bSynchAddress); + if (is_playback && attr == USB_ENDPOINT_SYNC_NONE) + return 0; return -EINVAL; }

@@ -450,8 +461,11 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, implicit_fb ? SND_USB_ENDPOINT_TYPE_DATA : SND_USB_ENDPOINT_TYPE_SYNC); - if (!subs->sync_endpoint) + if (!subs->sync_endpoint) { + if (is_playback && attr == USB_ENDPOINT_SYNC_NONE) + return 0; return -EINVAL; + }

subs->data_endpoint->sync_master = subs->sync_endpoint;

-- 1.9.1

Takashi Iwai

16 Aug 16 Aug

8:49 a.m.

On Sat, 15 Aug 2015 00:19:41 +0200, Pierre-Louis Bossart wrote:

...

While trying to understand how asynchronous endpoints are supported, I identified two major issues. On my M-Audio transit, the following script tries to play using the 3 different endpoints supported and fails (error and no audio) after the first playback without the patches. With the patches on top of 4.1.5 or Takashi's for-next (4.2.0-rc5), I stopped the audio after an hour.

while true do aplay -d5 -Dhw:2,0 ~/16_48.wav # play 48kHz, 16 bit on async ep aplay -d5 -Dhw:2,0 ~/24_48.wav # play 48kHz, 24 bit on 'none'/async ep aplay -d5 -Dhw:2,0 ~/24_96.wav # play 96 kHz, 24 bit on adaptive ep aplay -d5 -Dhw:2,0 ~/24_48.wav aplay -d5 -Dhw:2,0 ~/16_48.wav aplay -d5 -Dhw:2,0 ~/24_96.wav done

The first issue is pretty bad with corrupted pointers. When transitioning from a alternate setting with a sync_endpoint to another which doesn't have one by construction (synchronous, adaptive, async on capture), the pointers are not reset and the code tries to set the parameters of a non-existent endpoint.

The second issue is that the M-audio Transit descriptors are illegal but the logic can be changed without harm.

I sometimes hear noise on playback start, probably the code needs to be changed to add enough silent samples to let the PLL lock. I haven't had time to work on this and this is more of an enhancement.

v2: applied Takashi's comments unconditional clean-up of sync_endpoint pointers better logical tests and removal of goto

Pierre-Louis Bossart (2): ALSA: usb: fix corrupted pointers due to interface setting change ALSA: usb: handle descriptor with SYNC_NONE illegal value

Applied both now. Thanks.

Takashi

3416

Age (days ago)

3418

Last active (days ago)

List overview

Download

3 comments

2 participants

tags (0)

participants (2)

Pierre-Louis Bossart
Takashi Iwai