[alsa-devel] MTP-AV Bugs
Hi everybody,
I recently started again to use my older music equipment and try to get it work under Linux.
One of these things is an Motu Midi Express XT (8 Port Midi Interface at parallel port), which is listed as supported along with the Motu MidiTimePiece AV.
After upgrading to a recent Kernel, sending Midi from an Sequencer Programm to the Interface was working, but connecting an input port resulted in an immediate kernel freeze.
After trying some different parallel port settings and some other things, I upgraded to the latest alsa release (alsa-driver-1.0.19) and after the problem remains, started debugging the driver. (Because I never did any driver programming I only used print statements and returns to locate the point of failure..., if anyone of you has a good tutorial about debugging kernel modules - please sent a private Mail...)
After some time I found the following bug:
static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ --> snd_mtpav_inmidi_process(mcrd, inbyte); //here the inmidiport is not translated to the prtnumber, so // snd_mtpav_inmidi_process the port with an index of some 80000 // in the ports array of the mtpav struct was called return; }
if (mcrd->inmidistate == 0) { // awaiting command if (inbyte == 0xf5) // MTP port # mcrd->inmidistate = 1; else snd_mtpav_inmidi_process(mcrd, inbyte); } else if (mcrd->inmidistate) { mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte); mcrd->inmidistate = 0; }
The following change made the driver work: static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte); snd_mtpav_inmidi_process(mcrd, inbyte); return; } ....
After that I realised, that with the recent driver - even without my change - playing midi files din not work anymore. Only the first midi event was sent to the interface, everything else was dropped somewhere.
So a diff between the last workin version and the recent version showed only one change: 699,701c703,705 < card = snd_card_new(index, id, THIS_MODULE, sizeof(*mtp_card)); < if (! card) < return -ENOMEM; ---
err = snd_card_create(index, id, THIS_MODULE, sizeof(*mtp_card),
&card);
if (err < 0) return err;
which seems due to an api change. After rolling that back to the old (deprecated) way of creating the sound card, everything works fine.
Could someone with a little bit more experience in driver programming please look into the first change.
For the second change I would be happy about every tip why the old way works and the new way not. I'm willing to correct this as well.
Thank you for listening!
Holger
At Tue, 10 Feb 2009 21:54:32 +0100, Holger Dehnhardt wrote:
After some time I found the following bug:
static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ --> snd_mtpav_inmidi_process(mcrd, inbyte); //here the inmidiport is not translated to the prtnumber, so // snd_mtpav_inmidi_process the port with an index of some 80000 // in the ports array of the mtpav struct was called
Good catch. I think the patch below should fix the problem.
The following change made the driver work: static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte);
This isn't correct since inbyte is supposed to be under 0x13.
After that I realised, that with the recent driver - even without my change - playing midi files din not work anymore. Only the first midi event was sent to the interface, everything else was dropped somewhere.
So a diff between the last workin version and the recent version showed only one change:
What was the last working version exactly?
699,701c703,705 < card = snd_card_new(index, id, THIS_MODULE, sizeof(*mtp_card)); < if (! card)
< return -ENOMEM;
err = snd_card_create(index, id, THIS_MODULE, sizeof(*mtp_card),&card);
if (err < 0) return err;which seems due to an api change. After rolling that back to the old (deprecated) way of creating the sound card, everything works fine.
This must be some coincidence. This API change has nothing to do.
thanks,
Takashi
--- diff --git a/sound/drivers/mtpav.c b/sound/drivers/mtpav.c index 5b89c08..48b64e6 100644 --- a/sound/drivers/mtpav.c +++ b/sound/drivers/mtpav.c @@ -706,7 +706,6 @@ static int __devinit snd_mtpav_probe(struct platform_device *dev) mtp_card->card = card; mtp_card->irq = -1; mtp_card->share_irq = 0; - mtp_card->inmidiport = 0xffffffff; mtp_card->inmidistate = 0; mtp_card->outmidihwport = 0xffffffff; init_timer(&mtp_card->timer); @@ -719,6 +718,8 @@ static int __devinit snd_mtpav_probe(struct platform_device *dev) if (err < 0) goto __error;
+ mtp_card->inmidiport = mtp_card->num_ports + MTPAV_PIDX_BROADCAST; + err = snd_mtpav_get_ISA(mtp_card); if (err < 0) goto __error;
Dear Takashi,
thank you for your quick response! See my inline comments.
Am Mittwoch, 11. Februar 2009 13:08:22 schrieb Takashi Iwai:
At Tue, 10 Feb 2009 21:54:32 +0100,
Holger Dehnhardt wrote:
After some time I found the following bug:
static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ --> snd_mtpav_inmidi_process(mcrd, inbyte); //here the inmidiport is not translated to the prtnumber, so // snd_mtpav_inmidi_process the port with an index of some 80000 // in the ports array of the mtpav struct was called
Good catch. I think the patch below should fix the problem
The following change made the driver work: static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte);
This isn't correct since inbyte is supposed to be under 0x13.
Sorry, I don't understand what you mean: inbyte should always be lower than 0x13 or is the real time midi code always lower then 0x13?
After that I realised, that with the recent driver - even without my change - playing midi files din not work anymore. Only the first midi event was sent to the interface, everything else was dropped somewhere.
So a diff between the last workin version and the recent version showed only one change:
What was the last working version exactly?
1.0.18 (from the debian experimental repository)
--snip
Holger
At Wed, 11 Feb 2009 14:18:59 +0100, Holger Dehnhardt wrote:
Dear Takashi,
thank you for your quick response! See my inline comments.
Am Mittwoch, 11. Februar 2009 13:08:22 schrieb Takashi Iwai:
At Tue, 10 Feb 2009 21:54:32 +0100,
Holger Dehnhardt wrote:
After some time I found the following bug:
static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ --> snd_mtpav_inmidi_process(mcrd, inbyte); //here the inmidiport is not translated to the prtnumber, so // snd_mtpav_inmidi_process the port with an index of some 80000 // in the ports array of the mtpav struct was called
Good catch. I think the patch below should fix the problem
Did you try my patch?
The following change made the driver work: static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte);
This isn't correct since inbyte is supposed to be under 0x13.
Sorry, I don't understand what you mean: inbyte should always be lower than 0x13 or is the real time midi code always lower then 0x13?
I meant that the data passed to translate_hwport_to_subdevice() is supposed to be <= 0x13. In that code path, inbyte is >= 0xf8, thus you pass an invalid data.
After that I realised, that with the recent driver - even without my change - playing midi files din not work anymore. Only the first midi event was sent to the interface, everything else was dropped somewhere.
So a diff between the last workin version and the recent version showed only one change:
What was the last working version exactly?
1.0.18 (from the debian experimental repository)
Weird. There is really no essential change since that. Are you sure that you just copied 1.0.18 mtpav.c fixes the problem?
thanks,
Takashi
Am Mittwoch, 11. Februar 2009 14:26:23 schrieb Takashi Iwai:
At Wed, 11 Feb 2009 14:18:59 +0100,
Holger Dehnhardt wrote:
Dear Takashi,
thank you for your quick response! See my inline comments.
Am Mittwoch, 11. Februar 2009 13:08:22 schrieb Takashi Iwai:
At Tue, 10 Feb 2009 21:54:32 +0100,
Holger Dehnhardt wrote:
After some time I found the following bug:
static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ --> snd_mtpav_inmidi_process(mcrd, inbyte); //here the inmidiport is not translated to the prtnumber, so // snd_mtpav_inmidi_process the port with an index of some 80000 // in the ports array of the mtpav struct was called
Good catch. I think the patch below should fix the problem
Did you try my patch?
Sorry, Takashi, I'm still at work. I will try your path tonight when I'm home and my children are in bed;-)
The following change made the driver work: static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte);
This isn't correct since inbyte is supposed to be under 0x13.
Sorry, I don't understand what you mean: inbyte should always be lower than 0x13 or is the real time midi code always lower then 0x13?
I meant that the data passed to translate_hwport_to_subdevice() is supposed to be <= 0x13. In that code path, inbyte is >= 0xf8, thus you pass an invalid data.
After that I realised, that with the recent driver - even without my change - playing midi files din not work anymore. Only the first midi event was sent to the interface, everything else was dropped somewhere.
So a diff between the last workin version and the recent version showed only one change:
What was the last working version exactly?
1.0.18 (from the debian experimental repository)
Weird. There is really no essential change since that. Are you sure that you just copied 1.0.18 mtpav.c fixes the problem?
The only change I did was to change snd_card_create(...) back to snd_card_new(..) in the recent driver (1.0.19) to get the MIDI OUT working again.
After applying your patch I will go into this again.
Thanks
Holger
At Wed, 11 Feb 2009 14:44:13 +0100, Holger Dehnhardt wrote:
Am Mittwoch, 11. Februar 2009 14:26:23 schrieb Takashi Iwai:
At Wed, 11 Feb 2009 14:18:59 +0100,
Holger Dehnhardt wrote:
Dear Takashi,
thank you for your quick response! See my inline comments.
Am Mittwoch, 11. Februar 2009 13:08:22 schrieb Takashi Iwai:
At Tue, 10 Feb 2009 21:54:32 +0100,
Holger Dehnhardt wrote:
After some time I found the following bug:
static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ --> snd_mtpav_inmidi_process(mcrd, inbyte); //here the inmidiport is not translated to the prtnumber, so // snd_mtpav_inmidi_process the port with an index of some 80000 // in the ports array of the mtpav struct was called
Good catch. I think the patch below should fix the problem
Did you try my patch?
Sorry, Takashi, I'm still at work. I will try your path tonight when I'm home and my children are in bed;-)
Heh, you can teach your children driver programming :)
The following change made the driver work: static void snd_mtpav_inmidi_h(struct mtpav *mcrd, u8 inbyte) { if (inbyte >= 0xf8) { /* real-time midi code */ mcrd->inmidiport = translate_hwport_to_subdevice(mcrd, inbyte);
This isn't correct since inbyte is supposed to be under 0x13.
Sorry, I don't understand what you mean: inbyte should always be lower than 0x13 or is the real time midi code always lower then 0x13?
I meant that the data passed to translate_hwport_to_subdevice() is supposed to be <= 0x13. In that code path, inbyte is >= 0xf8, thus you pass an invalid data.
After that I realised, that with the recent driver - even without my change - playing midi files din not work anymore. Only the first midi event was sent to the interface, everything else was dropped somewhere.
So a diff between the last workin version and the recent version showed only one change:
What was the last working version exactly?
1.0.18 (from the debian experimental repository)
Weird. There is really no essential change since that. Are you sure that you just copied 1.0.18 mtpav.c fixes the problem?
The only change I did was to change snd_card_create(...) back to snd_card_new(..) in the recent driver (1.0.19) to get the MIDI OUT working again.
After applying your patch I will go into this again.
Hm, then I'm really puzzled. Anyway, it'd be helpful if you can double-check it.
thanks,
Takashi
Yes Takashi - your patch works - even if I had to apply the changes manually because patch always gave me errors and I was not willing to go deeper into it because of 2 lines of code. As stated before - I'm not used to this all ;-)
I did not have to do use the deprecated snd_card_new() method either... . Even MIDI out works as expected. I'm sorry that I was stupid enough to overwrite my changes, so I can not comprehend what I've done before. One last thing:
Heh, you can teach your children driver programming :)
You would not wish that my 3 year old son is doing this stuff - hopefully even I am better than he;-)
So Takashi, thank you very much for your help! Have a nice evening - or whatever time is in your place!
Holger
At Wed, 11 Feb 2009 22:08:42 +0100, Holger Dehnhardt wrote:
Yes Takashi - your patch works - even if I had to apply the changes manually because patch always gave me errors and I was not willing to go deeper into it because of 2 lines of code. As stated before - I'm not used to this all ;-)
I did not have to do use the deprecated snd_card_new() method either... . Even MIDI out works as expected. I'm sorry that I was stupid enough to overwrite my changes, so I can not comprehend what I've done before.
Good to hear. Now I applied the patch to sound git tree. It'll be included in the next pull request, too.
Thanks!
Takashi
participants (2)
-
Holger Dehnhardt -
Takashi Iwai