sound/mpu: clarify MPU-401 init #1888

n-p-soft · 2025-11-13T08:09:15Z

First, we try to better initialize the MPU-401 chip so that it is put in UART mode using the recommended way: <wait_for_ack><enable_uart_mode>. This must be done before the interrupt handler is active, so that the ACK is the only data received between the two commands. Actually this setup may occur after interrupts are enabled for the card.

Second, there is a mechanism to periodically trigger the interrupt handler which is using the Giant lock, probably to address very old (ISA ?) or spurious cards. Giant lock being removed, we disable this when our new init succeeds.

Rather old hardware is concerned; but a PCI soundcard may work fine using a PCIe-to-PCI bridge, and some cards have pretty decent chipsets.

Signed-off-by: Nicolas Provost [email protected]

Signed-off-by: Nicolas Provost <[email protected]>

github-actions · 2025-11-13T08:09:26Z

Thank you for taking the time to contribute to FreeBSD!
There are a few issues that need to be fixed:

Real email address is needed (407d0e9, eb375f6, fc23f08)
Missing Signed-off-by lines (5ded8c9)

Please review CONTRIBUTING.md, then update and push your branch again.

christosmarg

This is an initial review. Also note that I do not own a physical MIDI device (only USB ones), so I'm unable to test this.

I think it is necessary to update the commit message with an explanation of your approach and what these changes do, as they are not really obvious.

christosmarg · 2025-11-17T16:31:13Z

sys/dev/sound/midi/mpu401.c

 static void mpu401_mcallback(struct snd_midi *, void *, int);
 static void mpu401_mcallbackp(struct snd_midi *, void *, int);
+static const char *mpu401_mdescr(struct snd_midi *, void *, int);
+static const char *mpu401_mprovider(struct snd_midi *, void *);


Are these needed somewhere?

Probably not, I started working on 13.5 source tree..

I have made a few cleanups in the MIDI codebase, so I'd strongly advise you do your development on main (-CURRENT)

christosmarg · 2025-11-17T16:31:53Z

sys/dev/sound/midi/mpu401.c

 }
+
+#define MPU_TIMEOUT 50
+#define MPU_PAUSE pause_sbt("mpusetup", SBT_1MS, 0, 0)


I would suggest you define this as an inline function instead.

christosmarg · 2025-11-17T16:40:50Z

sys/dev/sound/midi/mpu401.c

-	}
-	printf("mpu401_minit failed active sensing\n");
-	return 1;
+	return (0);


What is the rationale for merging parts of this with mpu401_setup()?

This init is now run before the interrupt handler is active. The function returns 0 even on failure because we may have a card that is always in UART mode.

That should also definitely be documented, and also explain why this should run before the interrupt handler.

christosmarg · 2025-11-17T16:42:57Z

sys/dev/sound/midi/mpu401.c

+static int
+mpu401_intr(struct mpu401 *m)
+{
+	MIDI_TYPE b;


Why this type? What does it expand to?

This type was used in the file before (see midi.h) ! I agree it should be resolved to unsigned char.

As I said in a previous comment, please write the patches on top of main, not 13.5.

Ah ok ! Was wondering is someone was interested by these changes, and the starting point for me was a project under 13.5.

christosmarg · 2025-11-17T16:44:07Z

sys/dev/sound/midi/mpu401.c

+	 * Note that pending input may inhibits data sending.
+	 * Spurious cards may also be sticky..
+	 */
+	for (i = 0; RXRDY(m) && i < 512; i++) {


I suppose your comment about "pending input may inhibits data sending" is probably why you cap to 512 iterations, but why 512 iterations specifically? Same question applies to loop below.

As you say, it is an upper bound. I've really not idea what to set here -512 is probably too much. I've no such sticky card but I read that this may happen (probably no more nowadays -ISA cards ?).

That should be documented IMHO.

christosmarg · 2025-11-17T16:45:09Z

sys/dev/sound/midi/mpu401.c

+		if (midi_out(m->mid, &b, 1))
+			WRITE(m, b);
+		else
+			break;


The current mpu401_intr() returns in this case, is there a reason we break here instead?

Should the callout_reset be called in this case ?
Also, we can avoid missing one byte in this code but this case is tied to midi_in/midi_out functions in midi.c.

Not really sure about the callout to be honest.

christosmarg · 2025-11-17T16:49:49Z

sys/dev/sound/midi/mpu401.c

 #define MPU_INPUTBUSY  0x80
 #define MPU_TRYDATA 50
 #define MPU_DELAY   2500
+#define MPU_INTR_BUF	64


The current value is 16, why are you increasing it?

This is related to the changes I'm currently working on in midi.c (functions midi_in and midi_out). I will probably publish changes for both midi.c and mpu401.c.

If this is needed by a follow-up patch, then that change should be in that patch instead, but also documented.

Ok, to explain my goals: I noticed the MIDI support in sound/pci/emu10kx.c is not enabled for unknown reasons. I've enabled it for my SoundBlaster Audigy 2 (two fixes in the code) but this brings me to review midi.c and mpu401.c. In midi.c, there may be locking issues due to the two mutexes (one is probably enough) and perhaps buffering problems. In mpu401.c, the init was not optimal. My Audigy's MIDI-IN port is now working but some work is to be done. Is there a PCI card with a really working MIDI port under FreeBSD ?

There are locking issues indeed, and I'm pretty sure quite a few Lock Order Reversals. In fact, for months now my plan has been to scrap the whole current MIDI codebase and rewrite it cleanly. We could try to address some of the obvious issues temporarily at least, but do know that I will most likely rewrite this anyway. The current structure is rather obsolete and confusing, and does not work with USB MIDI.

If you do own physical MIDI devices, I would appreciate it a lot if we could do some testing together.

Is there a PCI card with a really working MIDI port under FreeBSD ?

Not really sure to be honest. Haven't checked.

What I'm currently planning:

mpu401.c: update changes to match branch 'main'.

midi.c: remove one of the two mutexes to avoid possible inter-locks. I think only midi.c and mpu401.are directly concerned in the whole kernel.

midi.c: rework midi_in and replace the current buffering queue with a simple buffer [currently testing]. Same thing for midi-out (is it currently working ?).

midi.c: code cleaning

pci/: enable MIDI for SoundBlaster Audigy 2 cards [currently testing]. Test other cards for MIDI support.

take care of not breaking too much OSS !

PCI devices that are concerned are quite old, but work well using a PCIe-to-PCI bridge, and some have pretty decent hardware.

christosmarg · 2025-11-19T12:48:55Z

The new commits are confusing and quite hard to see what's going on. The second commit especially (35d1d1c) seems to be deleting what I already removed in e4b3198. The rest of the commits seem to be mixed up also.

Could you please clean this up first? That is, combine everything into one commit, and make sure your branch is tracking the most recent main. First git pull on main, and then in your branch (assuming you don't work on main directly): git rebase main.

n-p-soft · 2025-11-19T13:21:52Z

I'm sorry but I cancel this pull request (#1888): I'm now working on branch main and created a fresh one (#1892). It seems that the callout_xxx stuff is using the giant lock which may be remove d in 15.0. So I also disabled it and I'm testing this. N. Provost Le 19 novembre 2025 13:49:16 GMT+01:00, Christos Margiolis ***@***.***> a écrit :

…

christosmarg left a comment (freebsd/freebsd-src#1888) The new commits are confusing and quite hard to see what's going on. The second commit especially (35d1d1c) seems to be deleting what I already removed in e4b3198. The rest of the commits seem to be mixed up also. Could you please clean this up first? That is, combine everything into one commit, and make sure your branch is tracking the most recent main. First `git pull` on `main`, and then in your branch (assuming you don't work on `main` directly): `git rebase main`. -- Reply to this email directly or view it on GitHub: #1888 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

… in UART mode using the recommended way: <reset><wait_for_ack><enable_uart_mode>. This must be done before the interrupt handler is active, so that the ACK is the only data received between the two commands. Actually this setup may occur after interrupts are enabled for the card. Second, there is a mechanism to periodically trigger the interrupt handler which is using the Giant lock, probably to address very old (ISA ?) or spurious cards. Giant lock being removed, we disable this when our new init succeeds. Rather old hardware is concerned; but a PCI soundcard may work fine using a PCIe-to-PCI bridge, and some cards have pretty decent chipsets. Signed-off-by: Nicolas Provost <[email protected]>

Signed-off-by: Nicolas Provost <[email protected]>

…ode using the recommended way: <reset><wait_for_ack><enable_uart_mode>. This must be done before the interrupt handler is active, so that the ACK is the only data received between the two commands. Actually this setup may occur after interrupts are enabled for the card. Second, there is a mechanism to periodically trigger the interrupt handler which is using the Giant lock, probably to address very old (ISA ?) or spurious cards. Giant lock being removed, we disable this when our new init succeeds. Rather old hardware is concerned; but a PCI soundcard may work fine using a PCIe-to-PCI bridge, and some cards have pretty decent chipsets. Signed-off-by: Nicolas Provost <[email protected]>

…init

Signed-off-by: Nicolas Provost <[email protected]>

Add files via upload

407d0e9

Signed-off-by: Nicolas Provost <[email protected]>

concussious requested a review from christosmarg November 14, 2025 15:09

christosmarg reviewed Nov 17, 2025

View reviewed changes

n-p-soft closed this Nov 19, 2025

n-p-soft deleted the mpu-init branch November 19, 2025 13:06

n-p-soft and others added 5 commits November 19, 2025 13:23

Update changes to add recent ones in branch 'main'.

35d1d1c

Signed-off-by: Nicolas Provost <[email protected]>

Merge remote-tracking branch 'refs/remotes/origin/mpu-init' into mpu-…

5ded8c9

…init

Update commit message and add missing decls.

43c1ccc

Signed-off-by: Nicolas Provost <[email protected]>

sound/mpu: clarify MPU-401 init #1888

sound/mpu: clarify MPU-401 init #1888

Conversation

n-p-soft commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christosmarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christosmarg Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christosmarg commented Nov 19, 2025

Uh oh!

n-p-soft commented Nov 19, 2025 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

n-p-soft commented Nov 13, 2025 •

edited

Loading

github-actions bot commented Nov 13, 2025 •

edited

Loading

christosmarg Nov 17, 2025 •

edited

Loading