sound: Retire midi_devs and mstat_lock
Needs ReviewPublic
Actions

Authored by christos on Mon, Dec 8, 3:31 PM.

Details

Reviewers

markj
emaste

Summary

Nowadays midi_devs only has two uses:

To verify in midi_init() if a given device unit exists, as well as to calculate the next device unit. Instead, make sure we always get a unique unit number using unr(9).
To keep track of all midi devices, so that it can destroy them all at once in midi_sysuninit(). However, since we stopped building midi as a kernel module, there is no reason to do that, simply let the drivers tear down the devices themselves.

Also retire mstat_lock since it's only used to protect midi_devs.

PR: 261071
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 69206
Build 66089: arc lint + arc unit

Event Timeline

christos created this revision.Mon, Dec 8, 3:31 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, Dec 8, 3:31 PM

christos requested review of this revision.Mon, Dec 8, 3:31 PM

Harbormaster completed remote builds in B69123: Diff 167708.Mon, Dec 8, 3:31 PM

christos added a child revision: D54127: snd_dummy: Initial MIDI support.Mon, Dec 8, 3:31 PM

christos added a parent revision: D54125: sound: Stop building midi as a module.Mon, Dec 8, 3:32 PM

christos edited the summary of this revision. (Show Details)Mon, Dec 8, 6:46 PM

christos mentioned this in D54129: sound: Retire snd_midi->qlock.Thu, Dec 11, 11:37 AM

Don't forget that some cards, as my common Soundblaster, have two ports; they actually appear as /dev/midi0.0 and /dev/midi0.1, which is useful to know that they belong to the same card.

Using more than one card could make difficult to find which port is tied to which card (although the devices discovery may be always done in the same order ?).

markj added inline comments.Fri, Dec 12, 2:59 PM

sys/dev/sound/midi/midi.c
162	What is this comment referring to?
628	What is this comment referring to?

christos mentioned this in D54125: sound: Stop building midi as a module.Fri, Dec 12, 3:16 PM

In D54126#1237784, @dev_nicolas-provost.fr wrote:

Don't forget that some cards, as my common Soundblaster, have two ports; they actually appear as /dev/midi0.0 and /dev/midi0.1, which is useful to know that they belong to the same card.

Using more than one card could make difficult to find which port is tied to which card (although the devices discovery may be always done in the same order ?).

I am aware, see discussion with Mark in the inline comments.

sys/dev/sound/midi/midi.c
162	To handle the channel number assignment. It's hardcoded to 0 because the patch removes the `channel` argument from `midi_init()`, which was also always hardcoded to 0 (see callers of `midi_init()`). However, this is not correct, and I want to address it. I'm probably going to update this patch to use unr(9), just as I'm doing at the line exactly above.
628	A reminder to handle this case, because as of this patch the `midiuninit` argument is never true (it was true only when `midi_destroy()` was called from `midi_sysuninit()`). I address this in D54192, which I ...forgot to submit for review up until now.

Do what I said in my last comment and allocate a unique channel number with
unr(9) as well.

Harbormaster completed remote builds in B69206: Diff 167926.Fri, Dec 12, 4:06 PM

christos mentioned this in D54192: sound: Merge midi_destroy() with midi_uninit().Fri, Dec 12, 4:08 PM

@dev_nicolas-provost.fr can you test again now? Are channel numbers allocated properly?

christos added a child revision: D54192: sound: Merge midi_destroy() with midi_uninit().Fri, Dec 12, 4:10 PM

Sorry, I can apply diff 54125 but no 54126 over it ?

Note: I don't know where to report this, but I found a regression in PCM (while testing MIDI for D54129, the sound was bad). After switching from release/13.5 to release/15, my Audigy card does not work well when vchans are enabled (osstest reports a sound rate of ~33khz for a 48khz playback). Furthermore, disabling vchans at boot time using sysctl.conf does not really disable the feature.

In D54126#1239027, @dev_nicolas-provost.fr wrote:

Sorry, I can apply diff 54125 but no 54126 over it ?

Probably not without modification.

Note: I don't know where to report this, but I found a regression in PCM (while testing MIDI for D54129, the sound was bad). After switching from release/13.5 to release/15, my Audigy card does not work well when vchans are enabled (osstest reports a sound rate of ~33khz for a 48khz playback). Furthermore, disabling vchans at boot time using sysctl.conf does not really disable the feature.

Please open a bug report on https://bugs.freebsd.org/bugzilla/ and add me as the assignee. Also make sure to provide sufficient information (minimum: sndctl -v, dmesg, sysctl hw.snd dev.pcm, uname -a).

Well, I tried to create a bugzilla account last week, but could not manage to get a password!

In D54126#1239049, @dev_nicolas-provost.fr wrote:

Well, I tried to create a bugzilla account last week, but could not manage to get a password!

You can contact linimon@freebsd.org for that, but in the meantime send me an email instead.

I suggest you to commit D54129 and my mpu_init changes (+changes here) so that we can make proper tests and incremental changes over it more simply.

In D54126#1239514, @dev_nicolas-provost.fr wrote:

I suggest you to commit D54129 and my mpu_init changes (+changes here) so that we can make proper tests and incremental changes over it more simply.

D54129 depends on this. As I said in another review, there is a stack of patches which you can see here. If you want to test this patch, just apply D54125 and then this one on top and it should work fine on a clean main branch.