| Message ID | 20201123085347.19667-15-tiwai@suse.de |
|---|---|
| State | Accepted |
| Commit | 54cb31901b831befb4f9347dd002dcc8ff2cc263 |
| Headers | show |
| Series | USB audio refactoring for better implicit feedback support | expand |
On Sun, 03 Jan 2021 18:09:41 +0100, František Kučera wrote: > > Dne 23. 11. 20 v 9:53 Takashi Iwai napsal(a): > > Currently snd_usb_endpoint objects are created at first when the > > substream is opened and tries to assign the endpoints corresponding to > > the matching audioformat. But since basically the all endpoints have > > been already parsed and the information have been obtained, we may > > create the endpoint objects statically at the init phase. It's easier > > to manage for the implicit fb case, for example. > > > > This patch changes the endpoint object management and lets the parser > > to create the all endpoint objects. > > > > This change shouldn't bring any functional changes. > > The Pioneer DJ DJM-250MK2 (implemented in 73d8c9408434, 14335d8b9e1a, cdc01a1558de) stopped working. > > After bisecting, it seems that it worked in 5fd255f4fe97 and stopped working in 54cb31901b83. > > The arecord says: set_params:1407: Unable to install hw params > > And even playback is impossible in this version. But there is no error in the log. However in later versions (e.g. eda809aef534) I found errors in kern.log: > > usb 1-3: Cannot find EP 0x1 to open > > usb 1-3: Cannot find EP 0x82 to open > > (each repeats 20 or more times, seems to be in pcm.c in subs->data_endpoint = snd_usb_endpoint_open(chip, fmt, hw_params, false);) > > Could you please check it? I am not sure whether it should be fixed in the DJM-250MK2 specific code or here. Could you give lsusb -v output as well as /proc/asound/card*/usbmixer and stream* files? The proc files are at best from both older and new kernels. thanks, Takashi
On Sun, 03 Jan 2021 19:15:48 +0100, František Kučera wrote: > > Dne 03. 01. 21 v 18:19 Takashi Iwai napsal(a): > > Could you give lsusb -v output as well as /proc/asound/card*/usbmixer > > and stream* files? The proc files are at best from both older and new > > kernels. > > Here are the files. They look same. The only difference is in the stream0, but it is probably only formatting. I also attached the kern.log from the new version. Thanks. It must be something specific to the devices with the fixed stream qurik. Could you try the patch below? Takashi -- 8< -- --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -155,9 +155,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, stream = (fp->endpoint & USB_DIR_IN) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; - err = snd_usb_add_audio_stream(chip, stream, fp); - if (err < 0) - goto error; if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber || fp->altset_idx >= iface->num_altsetting) { err = -EINVAL; @@ -176,6 +173,11 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, fp->datainterval = snd_usb_parse_datainterval(chip, alts); if (fp->maxpacksize == 0) fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize); + + err = snd_usb_add_audio_stream(chip, stream, fp); + if (err < 0) + goto error; + usb_set_interface(chip->dev, fp->iface, 0); snd_usb_init_pitch(chip, fp); snd_usb_init_sample_rate(chip, fp, fp->rate_max);
On Tue, 05 Jan 2021 10:29:24 +0100, Takashi Iwai wrote: > > On Sun, 03 Jan 2021 19:15:48 +0100, > František Kučera wrote: > > > > Dne 03. 01. 21 v 18:19 Takashi Iwai napsal(a): > > > Could you give lsusb -v output as well as /proc/asound/card*/usbmixer > > > and stream* files? The proc files are at best from both older and new > > > kernels. > > > > Here are the files. They look same. The only difference is in the stream0, but it is probably only formatting. I also attached the kern.log from the new version. > > Thanks. > > It must be something specific to the devices with the fixed stream > qurik. Could you try the patch below? Scratch that. The call of snd_usb_add_endpoint() is needed explicitly now at the quirk itself. Below is the v2 patch. Please give it a try. thanks, Takashi -- 8< -- --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -120,6 +120,38 @@ static int create_standard_audio_quirk(struct snd_usb_audio *chip, return 0; } +static int add_audio_stream_from_fixed_fmt(struct snd_usb_audio *chip, + struct audioformat *fp) +{ + int stream, err; + + stream = (fp->endpoint & USB_DIR_IN) ? + SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; + + snd_usb_audioformat_set_sync_ep(chip, fp); + + err = snd_usb_add_audio_stream(chip, stream, fp); + if (err < 0) + return err; + + /* add endpoints */ + err = snd_usb_add_endpoint(chip, fp->endpoint, + SND_USB_ENDPOINT_TYPE_DATA); + if (err < 0) + return err; + + if (fp->sync_ep) { + err = snd_usb_add_endpoint(chip, fp->sync_ep, + fp->implicit_fb ? + SND_USB_ENDPOINT_TYPE_DATA : + SND_USB_ENDPOINT_TYPE_SYNC); + if (err < 0) + return err; + } + + return 0; +} + /* * create a stream for an endpoint/altsetting without proper descriptors */ @@ -131,8 +163,8 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, struct audioformat *fp; struct usb_host_interface *alts; struct usb_interface_descriptor *altsd; - int stream, err; unsigned *rate_table = NULL; + int err; fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL); if (!fp) @@ -153,11 +185,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, fp->rate_table = rate_table; } - stream = (fp->endpoint & USB_DIR_IN) - ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; - err = snd_usb_add_audio_stream(chip, stream, fp); - if (err < 0) - goto error; if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber || fp->altset_idx >= iface->num_altsetting) { err = -EINVAL; @@ -176,6 +203,11 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, fp->datainterval = snd_usb_parse_datainterval(chip, alts); if (fp->maxpacksize == 0) fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize); + + err = add_audio_stream_from_fixed_fmt(chip, fp); + if (err < 0) + goto error; + usb_set_interface(chip->dev, fp->iface, 0); snd_usb_init_pitch(chip, fp); snd_usb_init_sample_rate(chip, fp, fp->rate_max); @@ -417,7 +449,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip, struct usb_host_interface *alts; struct usb_interface_descriptor *altsd; struct audioformat *fp; - int stream, err; + int err; /* both PCM and MIDI interfaces have 2 or more altsettings */ if (iface->num_altsetting < 2) @@ -482,9 +514,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip, return -ENXIO; } - stream = (fp->endpoint & USB_DIR_IN) - ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; - err = snd_usb_add_audio_stream(chip, stream, fp); + err = add_audio_stream_from_fixed_fmt(chip, fp); if (err < 0) { list_del(&fp->list); /* unlink for avoiding double-free */ kfree(fp);
Dne 05. 01. 21 v 14:20 Takashi Iwai napsal(a): > The call of snd_usb_add_endpoint() is needed explicitly > now at the quirk itself. > > Below is the v2 patch. Please give it a try. I applied your v2 patch to f6e7a024bfe5 version and tested with DJM-250MK2. It seems working (playback, recording). The original error messages (usb 1-3: Cannot find EP 0x1 to open, usb 1-3: Cannot find EP 0x82 to open) disappeared from the log. And new one occurred (Incompatible EP setup for 0x82 - repeats many times). However DJM-250MK2 works again. Thanks, Franta
Dne 06. 01. 21 v 10:03 Takashi Iwai napsal(a):
> OK, then could you try the v3 patch below?
I tried v3 patch. Playback works, but recording does not - i found errors like these in the log during recording:
Jan 6 18:07:08 antracit kernel: [ 619.973372] retire_capture_urb: 21 callbacks suppressed
Jan 6 18:07:13 antracit kernel: [ 624.974175] retire_capture_urb: 9993 callbacks suppressed
Jan 6 18:07:18 antracit kernel: [ 629.978015] retire_capture_urb: 9999 callbacks suppressed
Jan 6 18:08:04 antracit kernel: [ 675.932930] retire_capture_urb: 102 callbacks suppressed
Jan 6 18:08:09 antracit kernel: [ 680.936305] retire_capture_urb: 9998 callbacks suppressed
Jan 6 18:08:14 antracit kernel: [ 685.940643] retire_capture_urb: 10000 callbacks suppressed
Jan 6 18:09:45 antracit kernel: [ 776.168655] retire_capture_urb: 302 callbacks suppressed
Jan 6 18:09:50 antracit kernel: [ 781.169504] retire_capture_urb: 9993 callbacks suppressed
Jan 6 18:09:55 antracit kernel: [ 786.173337] retire_capture_urb: 9999 callbacks suppressed
(the original "Incompatible EP setup for 0x82" error was also present in the log)
The arecord command does nothing and after few seconds it fails with:
$ arecord -D hw:CARD=DJM250MK2 -f S24_3LE -c 8 -r 48000 > v3.wav
Recording WAVE 'stdin' : Signed 24 bit Little Endian in 3bytes, Rate 48000 Hz, Channels 8
arecord: pcm_read:2153: read error: Input/output error
The stream0 files from v2 and v3 differs in: Playback / Implicit Feedback Mode: Yes/No
BTW: playback or capture sometimes returns error for the first time but next attempts are successful. However I saw this behavior even in the older versions, so it is nothing v3 specific.
This is the error I got sometimes during the first run:
$ arecord -D hw:CARD=DJM250MK2 -f S24_3LE -c 8 -r 48000 > v3.wav
Recording WAVE 'stdin' : Signed 24 bit Little Endian in 3bytes, Rate 48000 Hz, Channels 8
arecord: set_params:1407: Unable to install hw params:
ACCESS: RW_INTERLEAVED
FORMAT: S24_3LE
SUBFORMAT: STD
SAMPLE_BITS: 24
FRAME_BITS: 192
CHANNELS: 8
RATE: 48000
PERIOD_TIME: (455104 455105)
PERIOD_SIZE: 21845
PERIOD_BYTES: 524280
PERIODS: 2
BUFFER_TIME: (910208 910209)
BUFFER_SIZE: 43690
BUFFER_BYTES: 1048560
TICK_TIME: 0
If I execute the same command again, it works. (but not in v3).
Franta
Pioneer DJ Corporation DJM-250MK2 at usb-0000:00:12.2-3, high speed : USB Audio
Playback:
Status: Stop
Interface 0
Altset 1
Format: S24_3LE
Channels: 8
Endpoint: 0x01 (1 OUT) (ASYNC)
Rates: 48000
Data packet interval: 500 us
Bits: 0
Sync Endpoint: 0x82 (2 IN)
Sync EP Interface: 0
Sync EP Altset: 1
Implicit Feedback Mode: No
Capture:
Status: Stop
Interface 0
Altset 1
Format: S24_3LE
Channels: 8
Endpoint: 0x82 (2 IN) (ASYNC)
Rates: 48000
Data packet interval: 500 us
Bits: 0
Sync Endpoint: 0x82 (2 IN)
Sync EP Interface: 0
Sync EP Altset: 1
Implicit Feedback Mode: No
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 94490d706013..eb459db511f8 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -440,22 +440,19 @@ static void snd_complete_urb(struct urb *urb) } /* - * Get the existing endpoint object corresponding EP, iface and alt numbers + * Get the existing endpoint object corresponding EP * Returns NULL if not present. - * Call inside chip->mutex locking for avoiding the race. */ struct snd_usb_endpoint * -snd_usb_get_endpoint(struct snd_usb_audio *chip, - int ep_num, int iface, int altsetting) +snd_usb_get_endpoint(struct snd_usb_audio *chip, int ep_num) { struct snd_usb_endpoint *ep; list_for_each_entry(ep, &chip->ep_list, list) { - if (ep->ep_num == ep_num && - ep->iface == iface && - ep->altsetting == altsetting) + if (ep->ep_num == ep_num) return ep; } + return NULL; } @@ -466,14 +463,13 @@ snd_usb_get_endpoint(struct snd_usb_audio *chip, * snd_usb_add_endpoint: Add an endpoint to an USB audio chip * * @chip: The chip - * @alts: The USB host interface * @ep_num: The number of the endpoint to use - * @direction: SNDRV_PCM_STREAM_PLAYBACK or SNDRV_PCM_STREAM_CAPTURE * @type: SND_USB_ENDPOINT_TYPE_DATA or SND_USB_ENDPOINT_TYPE_SYNC * * If the requested endpoint has not been added to the given chip before, - * a new instance is created. Otherwise, a pointer to the previoulsy - * created instance is returned. In case of any error, NULL is returned. + * a new instance is created. + * + * Returns zero on success or a negative error code. * * New endpoints will be added to chip->ep_list and must be freed by * calling snd_usb_endpoint_free(). @@ -481,74 +477,59 @@ snd_usb_get_endpoint(struct snd_usb_audio *chip, * For SND_USB_ENDPOINT_TYPE_SYNC, the caller needs to guarantee that * bNumEndpoints > 1 beforehand. */ -struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, - struct usb_host_interface *alts, - int ep_num, int direction, int type) +int snd_usb_add_endpoint(struct snd_usb_audio *chip, int ep_num, int type) { struct snd_usb_endpoint *ep; - int is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK; - - if (WARN_ON(!alts)) - return NULL; + bool is_playback; - mutex_lock(&chip->mutex); - - ep = snd_usb_get_endpoint(chip, ep_num, - alts->desc.bInterfaceNumber, - alts->desc.bAlternateSetting); - if (ep) { - usb_audio_dbg(ep->chip, "Re-using EP %x in iface %d,%d\n", - ep_num, ep->iface, ep->altsetting); - goto __exit_unlock; - } + ep = snd_usb_get_endpoint(chip, ep_num); + if (ep) + return 0; - usb_audio_dbg(chip, "Creating new %s %s endpoint #%x\n", - is_playback ? "playback" : "capture", + usb_audio_dbg(chip, "Creating new %s endpoint #%x\n", ep_type_name(type), ep_num); - ep = kzalloc(sizeof(*ep), GFP_KERNEL); if (!ep) - goto __exit_unlock; + return -ENOMEM; ep->chip = chip; spin_lock_init(&ep->lock); ep->type = type; ep->ep_num = ep_num; - ep->iface = alts->desc.bInterfaceNumber; - ep->altsetting = alts->desc.bAlternateSetting; INIT_LIST_HEAD(&ep->ready_playback_urbs); - ep_num &= USB_ENDPOINT_NUMBER_MASK; + is_playback = ((ep_num & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT); + ep_num &= USB_ENDPOINT_NUMBER_MASK; if (is_playback) ep->pipe = usb_sndisocpipe(chip->dev, ep_num); else ep->pipe = usb_rcvisocpipe(chip->dev, ep_num); - if (type == SND_USB_ENDPOINT_TYPE_SYNC) { - if (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE && - get_endpoint(alts, 1)->bRefresh >= 1 && - get_endpoint(alts, 1)->bRefresh <= 9) - ep->syncinterval = get_endpoint(alts, 1)->bRefresh; + list_add_tail(&ep->list, &chip->ep_list); + return 0; +} + +/* Set up syncinterval and maxsyncsize for a sync EP */ +void snd_usb_endpoint_set_syncinterval(struct snd_usb_audio *chip, + struct snd_usb_endpoint *ep, + struct usb_host_interface *alts) +{ + struct usb_endpoint_descriptor *desc = get_endpoint(alts, 1); + + if (ep->type == SND_USB_ENDPOINT_TYPE_SYNC) { + if (desc->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE && + desc->bRefresh >= 1 && desc->bRefresh <= 9) + ep->syncinterval = desc->bRefresh; else if (snd_usb_get_speed(chip->dev) == USB_SPEED_FULL) ep->syncinterval = 1; - else if (get_endpoint(alts, 1)->bInterval >= 1 && - get_endpoint(alts, 1)->bInterval <= 16) - ep->syncinterval = get_endpoint(alts, 1)->bInterval - 1; + else if (desc->bInterval >= 1 && desc->bInterval <= 16) + ep->syncinterval = desc->bInterval - 1; else ep->syncinterval = 3; - ep->syncmaxsize = le16_to_cpu(get_endpoint(alts, 1)->wMaxPacketSize); + ep->syncmaxsize = le16_to_cpu(desc->wMaxPacketSize); } - - list_add_tail(&ep->list, &chip->ep_list); - - ep->is_implicit_feedback = 0; - -__exit_unlock: - mutex_unlock(&chip->mutex); - - return ep; } /* diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 61487095a766..76b6de7de991 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -6,12 +6,9 @@ #define SND_USB_ENDPOINT_TYPE_SYNC 1 struct snd_usb_endpoint *snd_usb_get_endpoint(struct snd_usb_audio *chip, - int ep_num, int iface, - int altsetting); + int ep_num); -struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, - struct usb_host_interface *alts, - int ep_num, int direction, int type); +int snd_usb_add_endpoint(struct snd_usb_audio *chip, int ep_num, int type); int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, snd_pcm_format_t pcm_format, @@ -34,6 +31,9 @@ void snd_usb_endpoint_free(struct snd_usb_endpoint *ep); int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep); int snd_usb_endpoint_slave_next_packet_size(struct snd_usb_endpoint *ep); int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep); +void snd_usb_endpoint_set_syncinterval(struct snd_usb_audio *chip, + struct snd_usb_endpoint *ep, + struct usb_host_interface *alts); void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sender, diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 8ae7d2fdba0d..03b1a02bcff4 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -585,11 +585,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, if (!alts) return 0; - subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip, - alts, ep, !subs->direction, - fmt->implicit_fb ? - SND_USB_ENDPOINT_TYPE_DATA : - SND_USB_ENDPOINT_TYPE_SYNC); + subs->sync_endpoint = snd_usb_get_endpoint(subs->stream->chip, ep); if (!subs->sync_endpoint) { if (is_playback && (fmt->ep_attr & USB_ENDPOINT_SYNCTYPE) == USB_ENDPOINT_SYNC_NONE) @@ -597,10 +593,14 @@ static int set_sync_endpoint(struct snd_usb_substream *subs, return -EINVAL; } + subs->sync_endpoint->iface = fmt->sync_iface; + subs->sync_endpoint->altsetting = fmt->sync_altsetting; subs->sync_endpoint->is_implicit_feedback = fmt->implicit_fb; subs->data_endpoint->sync_master = subs->sync_endpoint; + snd_usb_endpoint_set_syncinterval(subs->stream->chip, subs->sync_endpoint, alts); + if (!subs->sync_endpoint->use_count && (subs->data_endpoint->iface != subs->sync_endpoint->iface || subs->data_endpoint->altsetting != subs->sync_endpoint->altsetting)) { @@ -641,8 +641,7 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) /* shared EP with implicit fb */ if (fmt->implicit_fb && !subs->need_setup_fmt) { - ep = snd_usb_get_endpoint(subs->stream->chip, fmt->endpoint, - fmt->iface, fmt->altsetting); + ep = snd_usb_get_endpoint(subs->stream->chip, fmt->endpoint); if (ep && ep->use_count > 0) goto add_data_ep; } @@ -688,12 +687,12 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) add_data_ep: subs->interface = fmt->iface; subs->altset_idx = fmt->altset_idx; - subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip, - alts, fmt->endpoint, subs->direction, - SND_USB_ENDPOINT_TYPE_DATA); - + subs->data_endpoint = snd_usb_get_endpoint(subs->stream->chip, + fmt->endpoint); if (!subs->data_endpoint) return -EINVAL; + subs->data_endpoint->iface = fmt->iface; + subs->data_endpoint->altsetting = fmt->altsetting; err = set_sync_endpoint(subs, fmt); if (err < 0) @@ -1294,15 +1293,13 @@ static int apply_hw_constraint_from_sync(struct snd_pcm_runtime *runtime, subs->fixed_hw = 0; list_for_each_entry(fp, &subs->fmt_list, list) { - ep = snd_usb_get_endpoint(chip, fp->endpoint, fp->iface, - fp->altsetting); + ep = snd_usb_get_endpoint(chip, fp->endpoint); if (ep && ep->cur_rate) goto found; if (!fp->implicit_fb) continue; /* for the implicit fb, check the sync ep as well */ - ep = snd_usb_get_endpoint(chip, fp->sync_ep, fp->sync_iface, - fp->sync_altsetting); + ep = snd_usb_get_endpoint(chip, fp->sync_ep); if (ep && ep->cur_rate) goto found; } diff --git a/sound/usb/stream.c b/sound/usb/stream.c index 7087ee2c8174..816fd3e5aada 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -1206,6 +1206,22 @@ static int __snd_usb_parse_audio_interface(struct snd_usb_audio *chip, kfree(pd); return err; } + + /* add endpoints */ + err = snd_usb_add_endpoint(chip, fp->endpoint, + SND_USB_ENDPOINT_TYPE_DATA); + if (err < 0) + return err; + + if (fp->sync_ep) { + err = snd_usb_add_endpoint(chip, fp->sync_ep, + fp->implicit_fb ? + SND_USB_ENDPOINT_TYPE_DATA : + SND_USB_ENDPOINT_TYPE_SYNC); + if (err < 0) + return err; + } + /* try to set the interface... */ usb_set_interface(chip->dev, iface_no, altno); snd_usb_init_pitch(chip, iface_no, alts, fp);