Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 74 | Some man pages have a .Sh LOADER TUNABLES or .Sh SYSCTL VARIABLES .Sh SYSCTL VARIABLES The following variables are available as both .Xr sysctl 8 variables and .Xr loader 8 tunables: are all of the variables here sysctls that can also be set as tunables by loader.conf? | |
Comment Actions
Thanks for picking this up, @christos.
I think the default_* sysctls definitely need some context here. The uaudio driver can only choose one of the device configurations it finds in the USB descriptor, and it will choose the "best" one. "Best" as in best quality, most channels. The default_ sysctls are there to sort of redefine what is considered "best". For the particular behavior of each one, I'd have to do some testing and code reading.
AFAIK, all these sysctls have to be set before the device is attached, to become effective. Although I'm not exactly sure what default_rate does, the sample rate is typically independent of the selected device configuration.
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 90–99 | This part is misleading. If I remember correctly, the driver selects the channel configuration with the most channels by default. It does so by looking for configurations with UAUDIO_CHANNELS_MAX channels, then for UAUDIO_CHANNELS_MAX - 1 channels, and so on until a matching device configuration is found (or 0 is reached). For USB 1.1 devices 4 channels are the maximum, due to bandwidth constraints. | |
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 74 |
Yes. | |
| 90–99 | I think we could re-phrase this as:
| |
Comment Actions
I think that might be a bit too technical to include in the man page, but I have no objection to include this information if you think it is vital information.
AFAIK, all these sysctls have to be set before the device is attached, to become effective. Although I'm not exactly sure what default_rate does, the sample rate is typically independent of the selected device configuration.
uaudio_chan_fill_info_sub() seems to be using default_rate if it's not set to 0 to find a matching channel configuration.
Comment Actions
Now that I think about it, the "default_" prefix is kind of misleading. These variables act like a ceiling value, so maybe we should rename them to "max_"? snd_uaudio configures the channels, rate and bits by searching in descending order either from the default_ value or the maximum defined in the source code.
Comment Actions
I think the default_ sysctl knobs would be more useful (and easier to document) if they actually acted like defaults. Would it be an option to improve the implementation in that way and handle them separately from the buffer_ms case?
Comment Actions
That would mean to document the buffer_ms sysctl now together with D41942, and improve and document the default_ knobs in a separate commit. Just an idea.
Comment Actions
I think it's better to have everything here as a contained unit. This patch already depends on D41942 anyway.
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 90–99 | @dev_submerge.ch Do you think the following is good? hw.usb.uaudio.default_channels
Default sample channels, from 0 to UAUDIO_CHANNELS_MAX (default
is 0). snd_uaudio will search in descending order for the
maximum-supported channel configuration, starting either from
UAUDIO_CHANNELS_MAX if hw.usb.uaudio.default_channels is set to
0, or, if set to a positive non-zero value, from
hw.usb.uaudio.default_channels. USB 1.1 devices are limited to 4
channels due to bandwidth constraints. | |
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 86 | @dev_submerge.ch Since we can set higher values than 4 on USB 1.1 devices, maybe the line would be more correct if it was something along the lines of "When set to 0, USB 1.1 devices default to 4 channels due to bandwidth constraints, but higher values can also be set." | |
Comment Actions
Sorry this took some time, but here is a list of info I'd want to have in the man page if I was an end user.
Be specific about when these settings are helpful. We don't want users to set them unnecessarily, due to possible side effects on other uaudio devices.
general info
- some devices support multiple configurations of sample sizes, channels, sample rates
- driver selects "best" configuration by default, if device supports multiple configurations
- overall limit of 64 channels
- USB 1.* devices limited to 4 channels by default, set higher default_channels to override
- sample rate can be adjusted through pcm device, vchanrate
- all other configuration parameters cannot change while device is attached
buffer_ms
- period of audio data processed at once, in ms from 1-8
- lower values mean less latency, beware of audible gaps if CPU cannot keep up with more frequent wakeups
- some devices will request higher values to operate correctly
default_bits
- preferred sample size in bits
- set this to select a smaller sample size if device supports multiple configurations
default_channels
- preferred number of channels
- set this to select less than maximum number of channels if device supports multiple configurations
default_rate
- preferred sample rate in Hz
- set this to limit the sample rate if device supports multiple configurations
handle_hid
- does that control mixer volume if device has a HID knob? (can't test myself)
Hope this gives some inspiration :-)
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 60–65 | This paragraph should probably start with the "If a device supports multiple configurations" part, to set the context first. Otherwise people will have to read that very long sentence (break it up?) twice. Also "bits" is vague, I'd use "sample size" here. | |
| 76 | I'd give some hint how to apply them, e.g. from vt(4) man page: These settings can be entered at the loader(8) prompt or in loader.conf(5) and can also be changed at runtime with the sysctl(8) command. Also I wouldn't expect readers to know what variables and tunables are. | |
| 80 | Wrong. Any of the preferences set here can have an effect on their own, if they match a device's configuration. Although the matching priority is channels > sample size > sample rate, but that's probably too much detail. What did you want to express here? | |
| 86 | Some devices might need higher values to work proprely. That's not what I meant. Some devices state in their USB descriptor that they need a longer period to operate. The driver respects that. Thus you can set a lower buffer_ms, but you'll not get the lower latency. Maybe we can just skip this detail, it's confusing. | |
| 89 | I'd use the term "preferred sample size" here, because it reflects that the driver selects from available configurations. But more importantly, we should explicitly state when to use this setting: Set this to select a smaller sample size if the device supports multiple sample sizes. It's the only situation this can be useful. Same for channels. | |
| 102 | A device doesn't have a default sample rate, AFAIK. The VCHAN setting lets a user choose a sample rate, but with default_rate set, current implementation limits this choice to default_rate max. | |
| 106 | So what is this used for? | |
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 76 | Copied as-is. | |
| 80 | Perhaps that was a poor choice of words, but I mean that in order for the value we set through the sysctl to take effect (be it rate, channels, bits), it has to be supported (i.e match a valid config) by the device in order to actually be part of the configuration. | |
| 106 | I don't really have a way to test this, my device doesn't support this, so I just used the definition from the sysctl description. | |
Comment Actions
Ok, hopefully last round of comments from my side :-)
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 66 | This sounds odd. How about "the configuration with the most channels and the highest quality in sample rate and sample size."? | |
| 80 | This is confusing unless very precisely formulated, and it really only concerns the default_* variables. I think the variables are now documented well enough on their own, we could just skip this sentence here. | |
| 87 | igor says "Lower values" should start on a new line. | |
| 97 | How about USB 1.1 devices are limited to 4 channels due to bandwidth constraints, unless a higher value is explicitly requested here. | |
| 101 | "sample" channel configurations? | |
| 106 | "overriden" - please do a spell-check again once we're done. | |
Comment Actions
One last thing, sorry for the mess of inline comments.
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 98–99 | Set this to select a smaller channel number if the device supports multiple sample channels. "sample channels" still doesn't make sense - how about "if the device supports multiple channel configurations."? | |
| share/man/man4/snd_uaudio.4 | ||
|---|---|---|
| 98–99 | I missed the "sample" while cleaning up the line. My bad. | |