Page MenuHomeFreeBSD
Differential D48336

sound: Handle multiple primary channel cases in vchan sysctls
AcceptedPublic
Actions

Authored by christos on Jan 6 2025, 1:57 PM.
Tags
None
Referenced Files
F109556349: D48336.id149651.diff
Thu, Feb 6, 4:12 PM
Unknown Object (File)
Tue, Feb 4, 5:24 PM
Unknown Object (File)
Mon, Feb 3, 2:32 AM
Unknown Object (File)
Mon, Feb 3, 12:17 AM
Unknown Object (File)
Thu, Jan 23, 5:16 PM
Unknown Object (File)
Tue, Jan 14, 9:29 PM
Unknown Object (File)
Mon, Jan 13, 7:35 PM
Unknown Object (File)
Sat, Jan 11, 4:01 PM
View All 13 Files
Subscribers
imp

Details

Reviewers
markj
emaste
dev_submerge.ch
Summary

vchan_getparentchannel() is used by various vchan sysctl functions to
fetch the first primary channel. However, this assumes that all devices
have only one primary channel per direction. If a device does not meet
this assumption, then the sysctl functions will be applying the
configurations on the first primary channel only.

Since we now have the "primary" channel sublist, we can retire
vchan_getparentchannel() and iterate through the "primary" list in each
sysctl function and apply the settings to all primary channels.

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 61913
Build 58797: arc lint + arc unit

Event Timeline

christos created this revision.Jan 6 2025, 1:57 PM
Herald added a subscriber: imp. · View Herald TranscriptJan 6 2025, 1:57 PM
christos requested review of this revision.Jan 6 2025, 1:57 PM
Harbormaster completed remote builds in B61519: Diff 148812.Jan 6 2025, 1:57 PM
christos mentioned this in D47918: sound: Simplify vchan_getparentchannel().Jan 6 2025, 1:58 PM
christos added parent revisions: D48335: sound: Cache vchanmode, D47917: sound: Allocate vchans on-demand.
dev_submerge.ch added a comment.Jan 7 2025, 12:10 AM

With this stack of changes I'm not able to set a different vchanrate than the 48000 default value. Didn't investigate yet, Christos can you reproduce this?

sys/dev/sound/pcm/vchan.c
560–564

Wrong order, the if block should come after the CHN_LOCK(c);, this panics.

christos marked an inline comment as done.Jan 7 2025, 3:31 PM
In D48336#1102533, @dev_submerge.ch wrote:

With this stack of changes I'm not able to set a different vchanrate than the 48000 default value. Didn't investigate yet, Christos can you reproduce this?

I can still change the vchanrate just fine. Does your device have more than 1 primary channels per direction?

sys/dev/sound/pcm/vchan.c
560–564

Sorry for this obvious mistake... I was travelling back home for hours and was kind of tired. Thanks for pointing this and the others out. :-)

christos updated this revision to Diff 148878.Jan 7 2025, 3:31 PM
christos marked an inline comment as done.

Fix dumb error.

Harbormaster completed remote builds in B61547: Diff 148878.Jan 7 2025, 3:31 PM
dev_submerge.ch added a comment.Jan 7 2025, 4:31 PM

No problem, I suppose you just wanted to test the attention of us reviewers ;-)

Regarding the vchanrate setting, the main difference is probably that this USB interface supports only one sample rate (selected via hardware switch). If I set it to 44.1kHz, the behavior is somewhat strange:

root@current:~ # sysctl dev.pcm.0.rec.vchanrate
dev.pcm.0.rec.vchanrate: 48000
root@current:~ # sysctl dev.pcm.0.play.vchanrate=96000
dev.pcm.0.play.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=48000
dev.pcm.0.rec.vchanrate: 44100 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 44100 -> 44100

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

christos added a comment.Jan 7 2025, 4:38 PM
In D48336#1102813, @dev_submerge.ch wrote:

No problem, I suppose you just wanted to test the attention of us reviewers ;-)

That's a more generous interpretation. :-)

Regarding the vchanrate setting, the main difference is probably that this USB interface supports only one sample rate (selected via hardware switch). If I set it to 44.1kHz, the behavior is somewhat strange:

root@current:~ # sysctl dev.pcm.0.rec.vchanrate
dev.pcm.0.rec.vchanrate: 48000
root@current:~ # sysctl dev.pcm.0.play.vchanrate=96000
dev.pcm.0.play.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=48000
dev.pcm.0.rec.vchanrate: 44100 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 44100 -> 44100

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

I am trying to understand how the patch would affect this behavior though. The previous implementation picks the first primary channel in a given direction, and this one loops through them, discarding the opposite direction's ones. With 1 primary channel per direction the behavior should be exactly the same. Are you sure this isn't the case without the patches, or at least not this one?

dev_submerge.ch added a comment.Jan 7 2025, 4:48 PM
In D48336#1102814, @christos wrote:
In D48336#1102813, @dev_submerge.ch wrote:

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

I am trying to understand how the patch would affect this behavior though. The previous implementation picks the first primary channel in a given direction, and this one loops through them, discarding the opposite direction's ones. With 1 primary channel per direction the behavior should be exactly the same. Are you sure this isn't the case without the patches, or at least not this one?

No, could be any commit, I did just test the whole stack up to this change. I have to go now, but I'll bisect the commits when I find time, maybe tomorrow.

christos added a comment.Jan 7 2025, 4:51 PM
In D48336#1102819, @dev_submerge.ch wrote:
In D48336#1102814, @christos wrote:
In D48336#1102813, @dev_submerge.ch wrote:

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

I am trying to understand how the patch would affect this behavior though. The previous implementation picks the first primary channel in a given direction, and this one loops through them, discarding the opposite direction's ones. With 1 primary channel per direction the behavior should be exactly the same. Are you sure this isn't the case without the patches, or at least not this one?

No, could be any commit, I did just test the whole stack up to this change. I have to go now, but I'll bisect the commits when I find time, maybe tomorrow.

My only guess would be D47917, but again I'm not sure how. Have a good evening.

dev_submerge.ch added a comment.Wed, Jan 8, 10:25 AM
In D48336#1102813, @dev_submerge.ch wrote:

Regarding the vchanrate setting, the main difference is probably that this USB interface supports only one sample rate (selected via hardware switch). If I set it to 44.1kHz, the behavior is somewhat strange:

root@current:~ # sysctl dev.pcm.0.rec.vchanrate
dev.pcm.0.rec.vchanrate: 48000
root@current:~ # sysctl dev.pcm.0.play.vchanrate=96000
dev.pcm.0.play.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=48000
dev.pcm.0.rec.vchanrate: 44100 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 44100 -> 44100

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

Looks like a case of faulty memory - in my brain, that is. This behavior is already present in earlier releases, no regression here. The sample rate gets clamped to the range supported by the device, in the RANGE(newspd, caps->minspeed, caps->maxspeed); statement, and that's code from 2009 or so. I just wouldn't notice this restriction on most devices, because they support a wider range of sample rates. While this behavior of vchanrate is inconsistent with other settings (user can choose vchanformat freely), it's not something to care about.

The one thing that we should investigate here is that the vchanrate setting starts with 48000, when 44100 is the only viable option. Are we missing an initialization step for the vchanrate? Current main branch starts with 44100 for this device / hardware setting.

christos added a comment.Wed, Jan 8, 10:35 AM
In D48336#1103256, @dev_submerge.ch wrote:
In D48336#1102813, @dev_submerge.ch wrote:

Regarding the vchanrate setting, the main difference is probably that this USB interface supports only one sample rate (selected via hardware switch). If I set it to 44.1kHz, the behavior is somewhat strange:

root@current:~ # sysctl dev.pcm.0.rec.vchanrate
dev.pcm.0.rec.vchanrate: 48000
root@current:~ # sysctl dev.pcm.0.play.vchanrate=96000
dev.pcm.0.play.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=48000
dev.pcm.0.rec.vchanrate: 44100 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 44100 -> 44100

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

Looks like a case of faulty memory - in my brain, that is. This behavior is already present in earlier releases, no regression here. The sample rate gets clamped to the range supported by the device, in the RANGE(newspd, caps->minspeed, caps->maxspeed); statement, and that's code from 2009 or so. I just wouldn't notice this restriction on most devices, because they support a wider range of sample rates. While this behavior of vchanrate is inconsistent with other settings (user can choose vchanformat freely), it's not something to care about.

The one thing that we should investigate here is that the vchanrate setting starts with 48000, when 44100 is the only viable option. Are we missing an initialization step for the vchanrate? Current main branch starts with 44100 for this device / hardware setting.

I guess that's because of D47917, which assigns vchanrate to VCHAN_DEFAULT_RATE (48000), during attach in pcm_init(). Perhaps we should initialize these values once the primary channels have been created, and use their rates instead of defaulting to VCHAN_DEFAULT_RATE. I suppose the same should apply to the format.

dev_submerge.ch accepted this revision.Thu, Jan 9, 12:05 PM
This revision is now accepted and ready to land.Thu, Jan 9, 12:05 PM
dev_submerge.ch mentioned this in D47917: sound: Allocate vchans on-demand.Thu, Jan 9, 12:36 PM
christos updated this revision to Diff 149649.Tue, Jan 21, 10:49 AM

Reflect changes in D48435.

This revision now requires review to proceed.Tue, Jan 21, 10:49 AM
Harbormaster completed remote builds in B61856: Diff 149649.Tue, Jan 21, 10:49 AM
christos updated this revision to Diff 149651.Tue, Jan 21, 11:01 AM

Reflect changes in D48435.

Harbormaster completed remote builds in B61858: Diff 149651.Tue, Jan 21, 11:01 AM
christos updated this revision to Diff 149755.Thu, Jan 23, 12:57 PM

Reflect changes in D48435. Hopefully last change.

Harbormaster completed remote builds in B61913: Diff 149755.Thu, Jan 23, 12:57 PM
christos added inline comments.Thu, Jan 23, 12:59 PM
sys/dev/sound/pcm/vchan.c
491

I am aware that in case there are >2 primary channels per-directions, then this will always be set to whatever the last primary channel's speed (and similarly for the format below) is. Not sure if we should care too much about this though.

dev_submerge.ch accepted this revision.Sat, Jan 25, 1:51 AM

Reapprove.

sys/dev/sound/pcm/vchan.c
491

That's fine, I don't see any other strategy that would make more sense.

This revision is now accepted and ready to land.Sat, Jan 25, 1:51 AM

Revision Contents
Changeset List

PathSize
sys/
dev/
sound/
pcm/
227 lines

Diff 149755

View Options

sys/dev/sound/pcm/vchan.c

Loading...