snd_uaudio(4): Fix config detection with defaults set.

[dev_submerge.ch]

Summary

Let the USB audio descriptor iteration detect configurations with more
channels and larger sample size, even when the following global sysctl
tunables are set to a lower value:

hw.usb.uaudio.default_channels
hw.usb.uaudio.default_bits

This improves utility and is closer to the meaning of default.

[dev_submerge.ch]

Not very elegant, but the whole iteration method here is somewhat crude. The default_rate sysctl already acts like a default, and does not prevent other sample rates from the list. Leave it unchanged.

For the others, I tested with a bunch of different USB audio interfaces that:

• Leaving defaults for default_bits (32) and default_channels (0) acts the same as before.
• Setting defaults selects the wanted configurations if they match.
• Setting defaults does not prevent other (higher) bits and channels configurations to be detected (improvement).
• The special behavior regarding USB 1.1 devices is preserved (no more than 4 channels by default, more channels if default_channels >= channels).

Please note that it's not possible to solve all situations with global settings like this. But this is already improves flexibility, and thus utility of these sysctl tunables.

[christos]

If the defaults can only take values between a certain range (i.e channels can be up to 64 and bits up to 32), and this patch also allows the driver to detect configs with higher values than the default, why don't we just start from the maximum value for both channels and bits instead of doing this hack, since we know that the default_*s are going to fall into those ranges anyway? Since in the iteration we try all possible combinations, we know that one of the configurations will end up being what the user would have defined using the defaults. This way we might not even need those controls at all. Also, following the same logic, do we need default_rate if the driver will only create channels for supported rates?

Correct me if I am missing something here, but I don't see where else those tunable values are used except in uaudio_chan_fill_info() which seems to search for and create configs for all supported channel/bits/rate combinations, and just includes a case where it tests if the user-supplied value is supported.

[christos]

Can you test whether this patch produces the same configurations for your devices (it's diff'd against the original version)? It gets rid of all default_* sysctls and detects configs starting from the max supported channels/bits and rates.
https://reviews.freebsd.org/P627

[dev_submerge.ch]

In D43679#997248, @christos wrote:

If the defaults can only take values between a certain range (i.e channels can be up to 64 and bits up to 32), and this patch also allows the driver to detect configs with higher values than the default, why don't we just start from the maximum value for both channels and bits instead of doing this hack, since we know that the default_*s are going to fall into those ranges anyway? Since in the iteration we try all possible combinations, we know that one of the configurations will end up being what the user would have defined using the defaults. This way we might not even need those controls at all. Also, following the same logic, do we need default_rate if the driver will only create channels for supported rates?

Correct me if I am missing something here, but I don't see where else those tunable values are used except in uaudio_chan_fill_info() which seems to search for and create configs for all supported channel/bits/rate combinations, and just includes a case where it tests if the user-supplied value is supported.

Order matters here! It's a bit hard to follow uaudio_chan_fill_info_sub() and its side effects. But AFAIK the first matching configuration is selected for use. And I think the other configurations are detected, but not used. Or at least I would not know how to change the configuration as long as the driver is attached. Except for the sample rate, but only in the range of the selected configuration from the USB descriptor.

An example: My RME BabyFace has a 10 in 12 out channel configuration, and a 2 in 2 out channel configuration. Let's say I want to use the 2 in 2 out configuration because most audio applications cannot cope with 12 channels, and I can't route everything through Jack. I also have an SPL Crimson, 6 in 6 out. And I have a Focusrite Scarlett 18 in 20 out.

Current implementation: If I set default_channels=2, only the BabyFace is detected. I can set default_channels=6, and then I get both the BabyFace and the Crimson. But no way to use the Scarlett at the same time.

My proposal: I can set default_channels=2, and get all of them detected correctly. Caveat: Any other uaudio device with a 2 channel configuration will have that configuration selected.

Your proposal: I cannot use the 2 in 2 out channel configuration at all. It always selects 10 in 12 out.

[christos]

In D43679#997388, @dev_submerge.ch wrote:

In D43679#997248, @christos wrote:

If the defaults can only take values between a certain range (i.e channels can be up to 64 and bits up to 32), and this patch also allows the driver to detect configs with higher values than the default, why don't we just start from the maximum value for both channels and bits instead of doing this hack, since we know that the default_*s are going to fall into those ranges anyway? Since in the iteration we try all possible combinations, we know that one of the configurations will end up being what the user would have defined using the defaults. This way we might not even need those controls at all. Also, following the same logic, do we need default_rate if the driver will only create channels for supported rates?

Correct me if I am missing something here, but I don't see where else those tunable values are used except in uaudio_chan_fill_info() which seems to search for and create configs for all supported channel/bits/rate combinations, and just includes a case where it tests if the user-supplied value is supported.

Order matters here! It's a bit hard to follow uaudio_chan_fill_info_sub() and its side effects. But AFAIK the first matching configuration is selected for use. And I think the other configurations are detected, but not used. Or at least I would not know how to change the configuration as long as the driver is attached. Except for the sample rate, but only in the range of the selected configuration from the USB descriptor.

An example: My RME BabyFace has a 10 in 12 out channel configuration, and a 2 in 2 out channel configuration. Let's say I want to use the 2 in 2 out configuration because most audio applications cannot cope with 12 channels, and I can't route everything through Jack. I also have an SPL Crimson, 6 in 6 out. And I have a Focusrite Scarlett 18 in 20 out.

Current implementation: If I set default_channels=2, only the BabyFace is detected. I can set default_channels=6, and then I get both the BabyFace and the Crimson. But no way to use the Scarlett at the same time.

My proposal: I can set default_channels=2, and get all of them detected correctly. Caveat: Any other uaudio device with a 2 channel configuration will have that configuration selected.

We may need to make these sysctls per-device instead of global. I can take care of this if we agree to go ahead with it.

Your proposal: I cannot use the 2 in 2 out channel configuration at all. It always selects 10 in 12 out.

Yeap, makes sense. I had forgot that the driver will use the first matching configuration instead of the best one.

[christos]

sys/dev/sound/usb/uaudio.c

2211

We could define UAUDIO_BITS_MAX so that we don't have to hardcode 32 both here and a few lines below.

2266

From what I understand, you added the channels < channels_max test to avoid pointless computation in case the default value also happens to be the max, although this test is not included in the y == bits case above. I think it's easier to understand what's going on if we either include a comment explaining the logic behind it or simply remove it altogether.

[dev_submerge.ch]

In D43679#997460, @christos wrote:

In D43679#997388, @dev_submerge.ch wrote:

An example: My RME BabyFace has a 10 in 12 out channel configuration, and a 2 in 2 out channel configuration. Let's say I want to use the 2 in 2 out configuration because most audio applications cannot cope with 12 channels, and I can't route everything through Jack. I also have an SPL Crimson, 6 in 6 out. And I have a Focusrite Scarlett 18 in 20 out.

Current implementation: If I set default_channels=2, only the BabyFace is detected. I can set default_channels=6, and then I get both the BabyFace and the Crimson. But no way to use the Scarlett at the same time.

My proposal: I can set default_channels=2, and get all of them detected correctly. Caveat: Any other uaudio device with a 2 channel configuration will have that configuration selected.

We may need to make these sysctls per-device instead of global. I can take care of this if we agree to go ahead with it.

In my experiments with snd_hdspe(4) I wasn't able to make per device settings from e.g. /boot/loader.conf available at attach time. I think the way to go would be changing the selected configuration on request by the pcm infrastructure, similar to what is done in uaudio_chan_set_param_speed() for the sample rate. But maybe not now, that's probably a bigger endeavor.

[christos]

In D43679#997498, @dev_submerge.ch wrote:

In D43679#997460, @christos wrote:

In D43679#997388, @dev_submerge.ch wrote:

An example: My RME BabyFace has a 10 in 12 out channel configuration, and a 2 in 2 out channel configuration. Let's say I want to use the 2 in 2 out configuration because most audio applications cannot cope with 12 channels, and I can't route everything through Jack. I also have an SPL Crimson, 6 in 6 out. And I have a Focusrite Scarlett 18 in 20 out.

Current implementation: If I set default_channels=2, only the BabyFace is detected. I can set default_channels=6, and then I get both the BabyFace and the Crimson. But no way to use the Scarlett at the same time.

My proposal: I can set default_channels=2, and get all of them detected correctly. Caveat: Any other uaudio device with a 2 channel configuration will have that configuration selected.

We may need to make these sysctls per-device instead of global. I can take care of this if we agree to go ahead with it.

In my experiments with snd_hdspe(4) I wasn't able to make per device settings from e.g. /boot/loader.conf available at attach time. I think the way to go would be changing the selected configuration on request by the pcm infrastructure, similar to what is done in uaudio_chan_set_param_speed() for the sample rate. But maybe not now, that's probably a bigger endeavor.

I agree.

[dev_submerge.ch]

sys/dev/sound/usb/uaudio.c

2266

The reason for that comparison is in the comment further above:

		/*
		 * Due to high bandwidth usage and problems
		 * with HIGH-speed split transactions we
		 * disable surround setups on FULL-speed USB
		 * by default
		 */

For USB 1.1 devices, default_channels can be set higher than the 4 channels we search by default. To preserve the current behavior, we have to count down from the higher value here.

Maybe we can also adjust channels_max during the checks above. Would that be more understandable?

[christos]

sys/dev/sound/usb/uaudio.c

2266

I think a simple comment explaining this will do, otherwise it's not at all obvious why that check is there IMHO.

[dev_submerge.ch]

Moved the special treatment of USB 1.1 devices up to where the USB bus speed is
queried. This should clarify the intent to only detect more than 4 channels if
the default_channels is set to a higher value.

Skip default_rate when we go through the list of sample rates, to avoid
having duplicate entries in the configuration list. The default sample rate is
already handled at that point. Solves D43767.

[christos]

LGTM. Thank you.