On Thu, 23 Apr 2020 19:29:08 +0200, Takashi Iwai wrote:
On Thu, 23 Apr 2020 18:57:34 +0200, Alexander Tsoy wrote:
And some further notes:
- I removed locking from snd_usb_endpoint_next_packet_size() and this
seems completely fixed an issue with large URBs I reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28
So playing at 96 kHz, driver packs 48 frames per URB and no more audio discontinuities.
Hmm, that's weird.
If removing the lock from snd_usb_endpoint_next_packet_size() really fixes the problem, it implies the lock contention. But as far as I see the code performed in this lock isn't conflicting so much. The URB processing shouldn't happen in parallel for the same EP.
BTW, one potential racy code I found while looking at the code is the list management in queue_pending_output_urbs(). The fix patch is below.
Takashi
--- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -321,17 +321,17 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) ep->next_packet_read_pos %= MAX_URBS;
/* take URB out of FIFO */ - if (!list_empty(&ep->ready_playback_urbs)) + if (!list_empty(&ep->ready_playback_urbs)) { ctx = list_first_entry(&ep->ready_playback_urbs, struct snd_urb_ctx, ready_list); + list_del_init(&ctx->ready_list); + } } spin_unlock_irqrestore(&ep->lock, flags);
if (ctx == NULL) return;
- list_del_init(&ctx->ready_list); - /* copy over the length information */ for (i = 0; i < packet->packets; i++) ctx->packet_size[i] = packet->packet_size[i];