sound: Handle multiple primary channel cases in vchan sysctls

[christos]

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

[dev_submerge.ch]

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

558–562

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

[christos]

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

558–562

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]

Fix dumb error.

[dev_submerge.ch]

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]

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]

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]

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]

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]

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.