tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

ALSA: usb-audio: Split endpoint setups for hw_params and prepare (take#2)

This is a second attempt to fix the bug appearing on Android with the
recent kernel; the first try was ff878b408a03 and reverted at commit
79764ec772bc.

The details taken from the v1 patch:

One of the former changes for the endpoint management was the more
consistent setup of endpoints at hw_params.
snd_usb_endpoint_configure() is a single function that does the full
setup, and it's called from both PCM hw_params and prepare callbacks.
Although the EP setup at the prepare phase is usually skipped (by
checking need_setup flag), it may be still effective in some cases
like suspend/resume that requires the interface setup again.

As it's a full and single setup, the invocation of
snd_usb_endpoint_configure() includes not only the USB interface setup
but also the buffer release and allocation. OTOH, doing the buffer
release and re-allocation at PCM prepare phase is rather superfluous,
and better to be done only in the hw_params phase.

For those optimizations, this patch splits the endpoint setup to two
phases: snd_usb_endpoint_set_params() and snd_usb_endpoint_prepare(),
to be called from hw_params and from prepare, respectively.

Note that this patch changes the driver operation slightly,
effectively moving the USB interface setup again to PCM prepare stage
instead of hw_params stage, while the buffer allocation and such
initializations are still done at hw_params stage.

And, the change of the USB interface setup timing (moving to prepare)
gave an interesting "fix", too: it was reported that the recent
kernels caused silent output at the beginning on playbacks on some
devices on Android, and this change casually fixed the regression.
It seems that those devices are picky about the sample rate change (or
the interface change?), and don't follow the too immediate rate
changes.

Meanwhile, Android operates the PCM in the following order:
- open, then hw_params with the possibly highest sample rate
- close without prepare
- re-open, hw_params with the normal sample rate
- prepare, and start streaming
This procedure ended up the hw_params twice with different rates, and
because the recent kernel did set up the sample rate twice one and
after, it screwed up the device. OTOH, the earlier kernels didn't set
up the USB interface at hw_params, hence this problem didn't appear.

Now, with this patch, the USB interface setup is again back to the
prepare phase, and it works around the problem automagically.
Although we should address the sample rate problem in a more solid
way in future, let's keep things working as before for now.

***

What's new in the take#2 patch:
- The regression caused by the v1 patch (bko#216500) was due to the
missing check of need_setup flag at hw_params. Now the check is
added, and the snd_usb_endpoint_set_params() call is skipped when
the running EP is re-opened.

- There was another bug in v1 where the clock reference rate wasn't
updated at hw_params phase, which may lead to a lack of the proper
hw constraints when an application doesn't issue the prepare but
only the hw_params call. This patch fixes it as well by tracking
the clock rate change in the prepare callback with a new flag
"need_update" for the clock reference object, just like others.

- The configure_endpoints() are simplified and folded back into
snd_usb_pcm_prepare().

Fixes: bf6313a0ff76 ("ALSA: usb-audio: Refactor endpoint management")
Fixes: ff878b408a03 ("ALSA: usb-audio: Split endpoint setups for hw_params and prepare")
Reported-by: chihhao chen <chihhao.chen@mediatek.com>
Link: https://lore.kernel.org/r/87e6d6ae69d68dc588ac9acc8c0f24d6188375c3.camel@mediatek.com
Link: https://lore.kernel.org/r/20220901124136.4984-1-tiwai@suse.de
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216500
Link: https://lore.kernel.org/r/20220920181106.4894-1-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>

Takashi Iwai 2be79d58 8e3392d3

+71 -64
+47 -31
sound/usb/endpoint.c
··· 40 40 unsigned char clock; 41 41 atomic_t locked; 42 42 int rate; 43 + bool need_setup; 43 44 struct list_head list; 44 45 }; 45 46 ··· 759 758 * The endpoint needs to be closed via snd_usb_endpoint_close() later. 760 759 * 761 760 * Note that this function doesn't configure the endpoint. The substream 762 - * needs to set it up later via snd_usb_endpoint_configure(). 761 + * needs to set it up later via snd_usb_endpoint_set_params() and 762 + * snd_usb_endpoint_prepare(). 763 763 */ 764 764 struct snd_usb_endpoint * 765 765 snd_usb_endpoint_open(struct snd_usb_audio *chip, ··· 1291 1289 return -ENOMEM; 1292 1290 } 1293 1291 1292 + /* update the rate of the referred clock; return the actual rate */ 1293 + static int update_clock_ref_rate(struct snd_usb_audio *chip, 1294 + struct snd_usb_endpoint *ep) 1295 + { 1296 + struct snd_usb_clock_ref *clock = ep->clock_ref; 1297 + int rate = ep->cur_rate; 1298 + 1299 + if (!clock || clock->rate == rate) 1300 + return rate; 1301 + if (clock->rate) { 1302 + if (atomic_read(&clock->locked)) 1303 + return clock->rate; 1304 + if (clock->rate != rate) { 1305 + usb_audio_err(chip, "Mismatched sample rate %d vs %d for EP 0x%x\n", 1306 + clock->rate, rate, ep->ep_num); 1307 + return clock->rate; 1308 + } 1309 + } 1310 + clock->rate = rate; 1311 + clock->need_setup = true; 1312 + return rate; 1313 + } 1314 + 1294 1315 /* 1295 1316 * snd_usb_endpoint_set_params: configure an snd_usb_endpoint 1296 1317 * 1318 + * It's called either from hw_params callback. 1297 1319 * Determine the number of URBs to be used on this endpoint. 1298 1320 * An endpoint must be configured before it can be started. 1299 1321 * An endpoint that is already running can not be reconfigured. 1300 1322 */ 1301 - static int snd_usb_endpoint_set_params(struct snd_usb_audio *chip, 1302 - struct snd_usb_endpoint *ep) 1323 + int snd_usb_endpoint_set_params(struct snd_usb_audio *chip, 1324 + struct snd_usb_endpoint *ep) 1303 1325 { 1304 1326 const struct audioformat *fmt = ep->cur_audiofmt; 1305 1327 int err; ··· 1375 1349 ep->maxframesize = ep->maxpacksize / ep->cur_frame_bytes; 1376 1350 ep->curframesize = ep->curpacksize / ep->cur_frame_bytes; 1377 1351 1378 - return 0; 1352 + return update_clock_ref_rate(chip, ep); 1379 1353 } 1380 1354 1381 1355 static int init_sample_rate(struct snd_usb_audio *chip, 1382 1356 struct snd_usb_endpoint *ep) 1383 1357 { 1384 1358 struct snd_usb_clock_ref *clock = ep->clock_ref; 1385 - int err; 1359 + int rate, err; 1386 1360 1387 - if (clock) { 1388 - if (atomic_read(&clock->locked)) 1389 - return 0; 1390 - if (clock->rate == ep->cur_rate) 1391 - return 0; 1392 - if (clock->rate && clock->rate != ep->cur_rate) { 1393 - usb_audio_dbg(chip, "Mismatched sample rate %d vs %d for EP 0x%x\n", 1394 - clock->rate, ep->cur_rate, ep->ep_num); 1395 - return -EINVAL; 1396 - } 1361 + rate = update_clock_ref_rate(chip, ep); 1362 + if (rate < 0) 1363 + return rate; 1364 + if (clock && !clock->need_setup) 1365 + return 0; 1366 + 1367 + err = snd_usb_init_sample_rate(chip, ep->cur_audiofmt, rate); 1368 + if (err < 0) { 1369 + if (clock) 1370 + clock->rate = 0; /* reset rate */ 1371 + return err; 1397 1372 } 1398 1373 1399 - err = snd_usb_init_sample_rate(chip, ep->cur_audiofmt, ep->cur_rate); 1400 - if (err < 0) 1401 - return err; 1402 - 1403 1374 if (clock) 1404 - clock->rate = ep->cur_rate; 1375 + clock->need_setup = false; 1405 1376 return 0; 1406 1377 } 1407 1378 1408 1379 /* 1409 - * snd_usb_endpoint_configure: Configure the endpoint 1380 + * snd_usb_endpoint_prepare: Prepare the endpoint 1410 1381 * 1411 1382 * This function sets up the EP to be fully usable state. 1412 - * It's called either from hw_params or prepare callback. 1383 + * It's called either from prepare callback. 1413 1384 * The function checks need_setup flag, and performs nothing unless needed, 1414 1385 * so it's safe to call this multiple times. 1415 1386 * 1416 1387 * This returns zero if unchanged, 1 if the configuration has changed, 1417 1388 * or a negative error code. 1418 1389 */ 1419 - int snd_usb_endpoint_configure(struct snd_usb_audio *chip, 1420 - struct snd_usb_endpoint *ep) 1390 + int snd_usb_endpoint_prepare(struct snd_usb_audio *chip, 1391 + struct snd_usb_endpoint *ep) 1421 1392 { 1422 1393 bool iface_first; 1423 1394 int err = 0; ··· 1435 1412 if (err < 0) 1436 1413 goto unlock; 1437 1414 } 1438 - err = snd_usb_endpoint_set_params(chip, ep); 1439 - if (err < 0) 1440 - goto unlock; 1441 1415 goto done; 1442 1416 } 1443 1417 ··· 1459 1439 goto unlock; 1460 1440 1461 1441 err = init_sample_rate(chip, ep); 1462 - if (err < 0) 1463 - goto unlock; 1464 - 1465 - err = snd_usb_endpoint_set_params(chip, ep); 1466 1442 if (err < 0) 1467 1443 goto unlock; 1468 1444
+4 -2
sound/usb/endpoint.h
··· 17 17 bool is_sync_ep); 18 18 void snd_usb_endpoint_close(struct snd_usb_audio *chip, 19 19 struct snd_usb_endpoint *ep); 20 - int snd_usb_endpoint_configure(struct snd_usb_audio *chip, 21 - struct snd_usb_endpoint *ep); 20 + int snd_usb_endpoint_set_params(struct snd_usb_audio *chip, 21 + struct snd_usb_endpoint *ep); 22 + int snd_usb_endpoint_prepare(struct snd_usb_audio *chip, 23 + struct snd_usb_endpoint *ep); 22 24 int snd_usb_endpoint_get_clock_rate(struct snd_usb_audio *chip, int clock); 23 25 24 26 bool snd_usb_endpoint_compatible(struct snd_usb_audio *chip,
+20 -31
sound/usb/pcm.c
··· 433 433 } 434 434 } 435 435 436 - static int configure_endpoints(struct snd_usb_audio *chip, 437 - struct snd_usb_substream *subs) 438 - { 439 - int err; 440 - 441 - if (subs->data_endpoint->need_setup) { 442 - /* stop any running stream beforehand */ 443 - if (stop_endpoints(subs, false)) 444 - sync_pending_stops(subs); 445 - if (subs->sync_endpoint) { 446 - err = snd_usb_endpoint_configure(chip, subs->sync_endpoint); 447 - if (err < 0) 448 - return err; 449 - } 450 - err = snd_usb_endpoint_configure(chip, subs->data_endpoint); 451 - if (err < 0) 452 - return err; 453 - snd_usb_set_format_quirk(subs, subs->cur_audiofmt); 454 - } else { 455 - if (subs->sync_endpoint) { 456 - err = snd_usb_endpoint_configure(chip, subs->sync_endpoint); 457 - if (err < 0) 458 - return err; 459 - } 460 - } 461 - 462 - return 0; 463 - } 464 - 465 436 /* 466 437 * hw_params callback 467 438 * ··· 522 551 subs->cur_audiofmt = fmt; 523 552 mutex_unlock(&chip->mutex); 524 553 525 - ret = configure_endpoints(chip, subs); 554 + if (!subs->data_endpoint->need_setup) 555 + goto unlock; 556 + 557 + if (subs->sync_endpoint) { 558 + ret = snd_usb_endpoint_set_params(chip, subs->sync_endpoint); 559 + if (ret < 0) 560 + goto unlock; 561 + } 562 + 563 + ret = snd_usb_endpoint_set_params(chip, subs->data_endpoint); 526 564 527 565 unlock: 528 566 if (ret < 0) ··· 614 634 goto unlock; 615 635 } 616 636 617 - ret = configure_endpoints(chip, subs); 637 + if (subs->sync_endpoint) { 638 + ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint); 639 + if (ret < 0) 640 + goto unlock; 641 + } 642 + 643 + ret = snd_usb_endpoint_prepare(chip, subs->data_endpoint); 618 644 if (ret < 0) 619 645 goto unlock; 646 + else if (ret > 0) 647 + snd_usb_set_format_quirk(subs, subs->cur_audiofmt); 648 + ret = 0; 620 649 621 650 /* reset the pointer */ 622 651 subs->buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);