snd_uaudio: skip duplicate configurations
AbandonedPublic
Actions

Authored by christos on Feb 6 2024, 11:21 AM.

Details

Reviewers

markj
dev_submerge.ch

Summary

If the user has supplied non-default values through
hw.usb.uaudio.default_*, snd_uaudio(4) will create a duplicate entry if
it matches the same configuration twice. This is because
uaudio_chan_fill_info() will create both the user-supplied
configuration, as well as the ones it automatically detects.

Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks

Test Plan

Before:

# sysctl hw.usb.uaudio.default_rate=44100
[...]
uaudio0: Play[0]: 44100 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 96000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 88200 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 48000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 44100 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 44100 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 96000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 88200 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 48000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 44100 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.

After:

# sysctl hw.usb.uaudio.default_rate=44100
[...]
uaudio0: Play[0]: 44100 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 96000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 88200 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Play[0]: 48000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 44100 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 96000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 88200 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.
uaudio0: Record[0]: 48000 Hz, 2 ch, 32-bit S-LE PCM format, 2x2ms buffer.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 55827
Build 52716: arc lint + arc unit

Event Timeline

christos created this revision.Feb 6 2024, 11:21 AM

Herald added a subscriber: imp. · View Herald TranscriptFeb 6 2024, 11:21 AM

christos requested review of this revision.Feb 6 2024, 11:21 AM

Harbormaster completed remote builds in B55827: Diff 133916.Feb 6 2024, 11:21 AM

christos edited the test plan for this revision. (Show Details)Feb 6 2024, 11:24 AM

The only case here is a duplicate sample rate, right? I'm uneasy to do this in the descriptor loop (why?).

I think it's much simpler and less invasive to integrate this with the changes in D43679. Would that be ok for you? I'll update it anyway today.

In D43767#998111, @dev_submerge.ch wrote:

The only case here is a duplicate sample rate, right?

For my device yeah, but I'm not sure if more sophisticated devices would print configs with multiple different channels or bits, which is why I added a check for channels and bits as well.

I'm uneasy to do this in the descriptor loop (why?).
I think it's much simpler and less invasive to integrate this with the changes in D43679. Would that be ok for you? I'll update it anyway today.

We could add an inner loop in uaudio_chan_fill_info() indeed. The reason I did it in the descriptor loop was so that we only check against valid values (the loop is placed after we've made sure the rate/channel/bit config is supported), whereas in uaudio_chan_fill_info() we'd get check against non-supported values as well.

That being said, even if we move the loop in uaudio_chan_fill_info(), I think it's better to have 2 separate patches as these patches are not directly related.

In D43767#998113, @christos wrote:

In D43767#998111, @dev_submerge.ch wrote:

The only case here is a duplicate sample rate, right?

For my device yeah, but I'm not sure if more sophisticated devices would print configs with multiple different channels or bits, which is why I added a check for channels and bits as well.

I'm uneasy to do this in the descriptor loop (why?).
I think it's much simpler and less invasive to integrate this with the changes in D43679. Would that be ok for you? I'll update it anyway today.

We could add an inner loop in uaudio_chan_fill_info() indeed. The reason I did it in the descriptor loop was so that we only check against valid values (the loop is placed after we've made sure the rate/channel/bit config is supported), whereas in uaudio_chan_fill_info() we'd get check against non-supported values as well.

That being said, even if we move the loop in uaudio_chan_fill_info(), I think it's better to have 2 separate patches as these patches are not directly related.

No need for an inner loop. It's a simple if clause to skip the default value when we iterate through the sample rates. Which is practically the same as I'm doing with the other default values there.

In D43767#998119, @dev_submerge.ch wrote:

In D43767#998113, @christos wrote:

In D43767#998111, @dev_submerge.ch wrote:

The only case here is a duplicate sample rate, right?

For my device yeah, but I'm not sure if more sophisticated devices would print configs with multiple different channels or bits, which is why I added a check for channels and bits as well.

I'm uneasy to do this in the descriptor loop (why?).
I think it's much simpler and less invasive to integrate this with the changes in D43679. Would that be ok for you? I'll update it anyway today.

We could add an inner loop in uaudio_chan_fill_info() indeed. The reason I did it in the descriptor loop was so that we only check against valid values (the loop is placed after we've made sure the rate/channel/bit config is supported), whereas in uaudio_chan_fill_info() we'd get check against non-supported values as well.

That being said, even if we move the loop in uaudio_chan_fill_info(), I think it's better to have 2 separate patches as these patches are not directly related.

No need for an inner loop. It's a simple if clause to skip the default value when we iterate through the sample rates. Which is practically the same as I'm doing with the other default values there.