Skip to contentSkip to conversation

Harmony driver - Reject AFMT_S16_LE (Little Endian)

PATCH In review Stuart Brady 6 participants 12 messages Feb 22, 2004 – May 29, 2004 1 message not in the archive

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Harmony driver - Reject AFMT_S16_LE (Little Endian)

Hello,

The OSS specification (http://www.opensound.com/pguide/oss.pdf - page 33) states:

"It is very important to check that the value returned in the argument after the [SNDCTL_DSP_SETFMT] ioctl call matches the requested format. If the device doesn't support this particular format, it rejects the call and returns another format that is supported by the hardware."

I would suggest the following change to the Harmony driver:

Index: harmony.c
===================================================================
RCS file: /var/cvs/linux-2.4/drivers/sound/harmony.c,v retrieving revision 1.28

harmony.c +4 -4
diff -u -r1.28 harmony.c
@@ -641,14 +641,14 @@
 			switch (ival) {
 			case AFMT_MU_LAW:	new_format = HARMONY_DF_8BIT_ULAW; break;
 			case AFMT_A_LAW:	new_format = HARMONY_DF_8BIT_ALAW; break;
-			case AFMT_S16_LE:	/* fall through, but not really supported */
-			case AFMT_S16_BE:	new_format = HARMONY_DF_16BIT_LINEAR;
-						ival = AFMT_S16_BE;
-						break; 
+			case AFMT_S16_BE:	new_format = HARMONY_DF_16BIT_LINEAR; break;
 			default: {
 				DPRINTK(KERN_WARNING PFX 
 					"unsupported sound format 0x%04x requested.\n",
 					ival);
+				ival = AFMT_S16_BE;
+				if (put_user(ival, (int *) arg))
+					return -EFAULT;
 				return -EINVAL;
 			}
 			}

Since mu-law and a-law aren't the most popular of formats, I think AFMT_S16_BE (signed, 16-bit, big endian) is the best thing to return. Some drivers don't seem to return a supported format in arg - I'm not sure why. At the very least, I think -EINVAL should be returned, and the format should not be set. (Unless of course, the driver actually _does_ the conversion - okay if I implement this?)

This change may break some apps. The soundcard.h fix might make a significant difference. Anything else was writing big endian data using a little endian format.

One question: are there any audio devices for hppa that need drivers, that have documentation? I see that audio on the J5k/C3k is unsupported, but there are apparently no docs for it. Audio on the 705 and 710 doesn't seem supported either. Again, no docs AFAICS.

Signature · 1 line
Stuart Brady
#1 lore raw
Quoted 4 lines · I said:

One question: are there any audio devices for hppa that need drivers, that have documentation? I see that audio on the J5k/C3k is unsupported, but there are apparently no docs for it. Audio on the 705 and 710 doesn't seem supported either. Again, no docs AFAICS.

I do apologise - I've now noticed that there _is_ a AD1889 driver, although I don't know whether it works on hppa.

http://lists.parisc-linux.org/pipermail/parisc-linux/2002-January/015138.html
looks quite useful - I'll have to see if I can get docs on the PSB2160.

It's interesting that the 720/730/750 has 3 channel beeper sound. Could someone tell me the name of the chip, please?

http://members.chello.nl/h.otten/vortexion.htm has a list of popular sound chips for microcomputers in the 80s. It'd be great if it turned out to be an AY-3-8910, YM2149 or SN76489, as there's a lot of music that's been written for those chips.

Signature · 1 line
Stuart Brady
#2 lore raw

Re: [parisc-linux] ad1889 driver/docs for c3k/j5k audio

In reply to Stuart Brady

On Sun, Feb 22, 2004 at 09:20:57AM +0000, Stuart Brady wrote:
...

Quoted 2 lines

One question: are there any audio devices for hppa that need drivers, that have documentation? I see that audio on the J5k/C3k is unsupported,

As you noted ad1889 is available but only sort-of works:
http://lists.parisc-linux.org/pipermail/parisc-linux/2003-April/019650.html

I posted the only documentation we have for ad1889:
http://lists.parisc-linux.org/pipermail/parisc-linux/2002-June/016704.html

If you have patches for ad1889, I'm happy to test (and commit) them for you.

thanks,
grant

#3 lore raw

[parisc-linux] Re: ad1889 driver/docs for c3k/j5k audio

Quoted 4 lines · Grant Grundler

I posted the only documentation we have for ad1889:
http://lists.parisc-linux.org/pipermail/parisc-linux/2002-June/016704.html

If you have patches for ad1889, I'm happy to test (and commit) them for you.

Unfortunately, the documentation you're referring to is no longer there. Do you still have it?

If so, I don't have a box with an ad1889, but I should be able to make some small improvements.

Signature · 5 lines
Stuart Brady
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
#4 lore raw

Re: [parisc-linux] Re: ad1889 driver/docs for c3k/j5k audio

Quoted 2 lines

Unfortunately, the documentation you're referring to is no longer there. Do you still have it?

try: ftp://ftp.parisc-linux.org/docs/AD1889.pdf

Quoted 2 lines

If so, I don't have a box with an ad1889, but I should be able to make some small improvements.

I think Mort is also looking at this.....

thanks :)
randolph

Signature · 7 lines
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
#5 lore raw

PATCH 1/1 Harmony driver - Reject AFMT_S16_LE & Implement SNDCTL_DSP_CHANNELS

In reply to Stuart Brady

Review

Quoted 2 lines · Stuart Brady
(quoted diff) +2 -0
+				if (put_user(ival, (int *) arg))
+					return -EFAULT;

I've been looking at the specification, I think this should have been:

return put_user(ival, (int *) arg));

I assumed that -EINVAL would be returned, but the example shows the argument being changed, without the ioctl returning an error. I'm still not sure whether "harmony_set_format(HARMONY_DF_16BIT_LINEAR)" is needed when an unsupported format is requested. The specification is not clear to me. "Rejects" implies "does not set the format" to me.

I'm going to have to get a firm idea of what each driver currently does (since they tend to do different things anyway), what the apps want, and what the documentation states.

I've also added a SNDCTL_DSP_CHANNELS (needed for mikmod). Here is the revised patch:

Index: harmony.c
===================================================================
RCS file: /var/cvs/linux-2.4/drivers/sound/harmony.c,v retrieving revision 1.28

harmony.c +16 -7
diff -u -r1.28 harmony.c
@@ -641,18 +641,17 @@
 			switch (ival) {
 			case AFMT_MU_LAW:	new_format = HARMONY_DF_8BIT_ULAW; break;
 			case AFMT_A_LAW:	new_format = HARMONY_DF_8BIT_ALAW; break;
-			case AFMT_S16_LE:	/* fall through, but not really supported */
-			case AFMT_S16_BE:	new_format = HARMONY_DF_16BIT_LINEAR;
-						ival = AFMT_S16_BE;
-						break; 
+			case AFMT_S16_BE:	new_format = HARMONY_DF_16BIT_LINEAR; break;
 			default: {
 				DPRINTK(KERN_WARNING PFX 
 					"unsupported sound format 0x%04x requested.\n",
 					ival);
-				return -EINVAL;
+				ival = AFMT_S16_BE;
+				return put_user(ival, (int *) arg);
 			}
 			}
 			harmony_set_format(new_format);
+			return 0;
 		} else {
 			switch (harmony.data_format) {
 			case HARMONY_DF_8BIT_ULAW:	ival = AFMT_MU_LAW; break;
@@ -660,8 +659,8 @@
 			case HARMONY_DF_16BIT_LINEAR:	ival = AFMT_U16_BE; break;
 			default: ival = 0;
 			}
+			return put_user(ival, (int *) arg);
 		}
-		return put_user(ival, (int *) arg);
 
 	case SOUND_PCM_READ_RATE:
 		ival = harmony.dac_rate;
@@ -680,7 +679,17 @@
 		if (ival != 0 && ival != 1)
 			return -EINVAL;
 		harmony_set_stereo(ival);
-		return put_user(ival, (int *) arg);
+		return 0;
+
+	case SNDCTL_DSP_CHANNELS:
+		if (get_user(ival, (int *) arg))
+			return -EFAULT;
+		if (ival != 1 && ival != 2) {
+			ival = harmony.stereo_select == HARMONY_SS_MONO ? 1 : 2;
+			return put_user(ival, (int *) arg);
+		}
+		harmony_set_stereo(ival-1);
+		return 0;
 
 	case SNDCTL_DSP_GETBLKSIZE:
 		ival = HARMONY_BUF_SIZE;

Mikmod now plays audio, but it says "Loading" at the top and you can't do anything in the UI. I'll look into this after I've fixed the mixer. The internal speaker, headphones, and rear jack don't seem to fit well with the existing mixer channels in OSS. Any recommendations?

I'm not sure how SOUND_PCM_WRITE_CHANNELS and SOUND_PCM_READ_CHANNELS are handled. Looks like they're the same ioctl. How does that work?!

Signature · 1 line
Stuart Brady
#6 lore raw

Re: [parisc-linux] [PATCH] Harmony driver - Reject AFMT_S16_LE & Implement SNDCTL_DSP_CHANNELS

Quoted 6 lines · Stuart Brady

Review

Quoted 2 lines · Stuart Brady
(quoted diff) +2 -0
+				if (put_user(ival, (int *) arg))
+					return -EFAULT;

I've been looking at the specification, I think this should have been:

...

Hi Stuart,
Sorry for taking so long with this...I was expecting someone else to reply and maybe missed it. A couple of minor problems with your patch:

1) it's for 2.4 and I'm personally trying to focus on 2.6 tree now. 2.6 is ready for prime time. It's been my desktop/ftp/http server (C3600) for the past couple of monthes and has proven very stable.

2) OSS is "decprecated" according to the 2.6 sound/Kconfig "Help". Ie use ALSA instead. See SOUND_PRIME option.

3) if no one else wants to poke at the 2.4 harmony driver, I will just apply your patches on good faith that it builds/works for you. Have you tweaked it more since Feb?

In 2.6, ALSA harmony driver does NOT compile and that's disappointing.

thanks,
grant
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

#7 lore raw

Re: [parisc-linux] [PATCH] Harmony driver - Reject AFMT_S16_LE & Implement SNDCTL_DSP_CHANNELS

Quoted 4 lines · Grant Grundler

2) OSS is "decprecated" according to the 2.6 sound/Kconfig "Help". Ie use ALSA instead. See SOUND_PRIME option.

In 2.6, ALSA harmony driver does NOT compile and that's disappointing.

The basic problem here is that ALSA doesn't use the generic device model. It has its own crazy set of bus abstractions which would all need to be implemented for the parisc_device ... for the benefit of one driver. Small wonder nobody's been interested in doing the work yet.

I think we'll be sticking with OSS Harmony for a while.

Signature · 10 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
#8 lore raw

Re: [parisc-linux] [PATCH] Harmony driver - Reject AFMT_S16_LE &Implement SNDCTL_DSP_CHANNELS

Actually, ALSA Harmony driver is compiling at this point (2.6.6-rc3-pa9), with the caveat that you need the latest CVS of sound/parisc..
Takashi Iwai has been kind enough to work on this, with me doing some basic testing. Unfortunately, my testing hit a snag when a hard drive started releasing magic smoke ever so slowly.

Thanks
Paul

----- Original Message ----- From: "Matthew Wilcox" <[email protected]>
To: "Grant Grundler" <[email protected]>
Cc: <[email protected]>
Sent: Tuesday, May 11, 2004 8:07 AM
Subject: Re: [parisc-linux] [PATCH] Harmony driver - Reject AFMT_S16_LE &Implement SNDCTL_DSP_CHANNELS

Quoted 24 lines · Grant Grundler
Quoted 4 lines

2) OSS is "decprecated" according to the 2.6 sound/Kconfig "Help". Ie use ALSA instead. See SOUND_PRIME option.

In 2.6, ALSA harmony driver does NOT compile and that's disappointing.

The basic problem here is that ALSA doesn't use the generic device model. It has its own crazy set of bus abstractions which would all need to be implemented for the parisc_device ... for the benefit of one driver. Small wonder nobody's been interested in doing the work yet.

I think we'll be sticking with OSS Harmony for a while.

Signature · 10 lines
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux 

_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

#9 lore raw

Re: [parisc-linux] [PATCH] Harmony driver - Reject AFMT_S16_LE & Implement SNDCTL_DSP_CHANNELS

In reply to Grant Grundler

Hi Grant,

Quoted 3 lines

Sorry for taking so long with this...I was expecting someone else to reply and maybe missed it. A couple of minor problems with your patch:

Actually, I think I should have been more persistent.

Quoted 10 lines

1) it's for 2.4 and I'm personally trying to focus on 2.6 tree now. 2.6 is ready for prime time. It's been my desktop/ftp/http server (C3600) for the past couple of monthes and has proven very stable.

2) OSS is "decprecated" according to the 2.6 sound/Kconfig "Help". Ie use ALSA instead. See SOUND_PRIME option.

3) if no one else wants to poke at the 2.4 harmony driver, I will just apply your patches on good faith that it builds/works for you. Have you tweaked it more since Feb?

I've made some improvements to the mixer code, but OSS's mixer interface seems to be inadequate. I can post a patch that improves the mixer code slightly. Other than that, I haven't changed anything.

Under high load, I tend to get panics when using the Harmony driver. I've no idea why this is, and I wish I could understand the low-level code a bit better. This has nothing to do with my patches, though. (I.e. it crashes with them and it crashes without them.)

http://lists.parisc-linux.org/pipermail/parisc-linux/2004-February/022432.html
is probably more of an issue for lkml. (There's a slight mistake in that patch - I forgot the closing */ on line 42.)

Quoted 1 line

In 2.6, ALSA harmony driver does NOT compile and that's disappointing.

I would love to try and help with the 2.6 drivers instead. Unfortunatlely, I can't get 2.6 to boot. This is all that I see:

Command line for kernel: 'root=/dev/sda6 HOME=/ console=tty0 sti=1 sti_font=VGA8x16 TERM=linux palo_kernel=2/vmlinux-2.6' Selected kernel: /vmlinux-2.6 from partition 2
ELF32 executable
Entry 001001c0 first 00100000 n 3
Segment 0 load 00100000 size 2520296 mediaptr 0x1000
Segment 1 load 00368000 size 565504 mediaptr 0x269000 Segment 2 load 003f4000 size 581766 mediaptr 0x2f4000 Branching to kernel entry point 0x001001c0. If this is the last message you see, you may need to switch your console. This is a common symptom -- search the FAQ and mailing list at parisc-linux.org

I have another graphics card in this machine, so I'm wondering whether that has anything to do with this. FWIW, it's a 715/100. I'm using the prebuilt palinux-32-2.6.6-pa4_0-2_all.deb package from
http://cvs.parisc-linux.org/download/linux-2.6/

Thanks,

Signature · 5 lines
Stuart Brady
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
#10 lore raw

Re: [parisc-linux] [PATCH] Harmony driver - Reject AFMT_S16_LE & Implement SNDCTL_DSP_CHANNELS

Hello,

Quoted 3 lines · Stuart Brady
Quoted 3 lines

In 2.6, ALSA harmony driver does NOT compile and that's disappointing.

as I (and others) wrote in a former mail ALSA harmony driver from ALSA cvs does compile. Just copy harmony.c from ALSA cvs into kernel source, fix two typos (haromny -> harmony) and voilà... Boot log shows:

Advanced Linux Sound Architecture Driver Version 1.0.4. Lasi Harmony Audio driver h/w id 20, rev. 18 at 0xf0104000, IRQ 82
harmony: Successfully registered harmony pcm backend & mixer 0 ALSA device list:
#0: ALSA driver for LASI Harmony at h/w, id 20, rev. 18 hpa
0xf0104000, IRQ 82

There is just one problem ;). One can't unmute main volume. I can change volume settings using aumix, but they get canceled when closing aumix. But perhaps this can be fixed easily by s.o. who has the force... ;)

Quoted 4 lines

I would love to try and help with the 2.6 drivers instead. Unfortunatlely, I can't get 2.6 to boot. This is all that I see:

Quoted 13 lines

Command line for kernel: 'root=/dev/sda6 HOME=/ console=tty0 sti=1 sti_font=VGA8x16 TERM=linux palo_kernel=2/vmlinux-2.6' Selected kernel: /vmlinux-2.6 from partition 2
ELF32 executable
Entry 001001c0 first 00100000 n 3
Segment 0 load 00100000 size 2520296 mediaptr 0x1000
Segment 1 load 00368000 size 565504 mediaptr 0x269000 Segment 2 load 003f4000 size 581766 mediaptr 0x2f4000 Branching to kernel entry point 0x001001c0. If this is the last message you see, you may need to switch your console. This is a common symptom -- search the FAQ and mailing list at parisc-linux.org

I also have an 715/100 with two graphics card and I'm using current cvs kernel (2.6.7-rc1-pa1). It works for me. My command line for the kernel is:
'root=/dev/sdb4 HOME=/ console=tty0 sti=1 sti_font=VGA8x16 TERM=linux palo_kernel=2/vmlinux'. I can also give you my .config if you are interested in.

Quoted 8 lines

I have another graphics card in this machine, so I'm wondering whether that has anything to do with this. FWIW, it's a 715/100. I'm using the prebuilt palinux-32-2.6.6-pa4_0-2_all.deb package from
http://cvs.parisc-linux.org/download/linux-2.6/

Thanks,

Thanks,
Ruediger
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

A message that is not in the archive (sent off-list or not yet received): [email protected]

#12 lore raw

PATCH 1/1 Harmony - buffer underrun crash fix

Quoted 3 lines · Thibaut VARENE

There's been a suspected buffer overflow in the code for years, and it's a known "feature" that the driver may randomly panic the kernel. Patch welcome.

Thanks. I had assumed that this was black magic and I'd never be able to fix it, but thanks to your encouragement, I've taken a look and I now have a fix for the crash, as well as a sample skipping problem that I was previously unaware of:

Index: harmony.c
===================================================================
RCS file: /var/cvs/linux-2.4/drivers/sound/harmony.c,v retrieving revision 1.28

harmony.c +9 -4
diff -u -r1.28 harmony.c
@@ -537,6 +537,7 @@
 	int count = 0;
 	int frame_size;
 	int buf_to_fill;
+	int fresh_buffer;
 
 	if (!harmony.format_initialized) 
 	   harmony_format_auto_detect(buffer, total_count);
@@ -556,12 +557,16 @@
 		
 		
 		buf_to_fill = (harmony.first_filled_play+harmony.nb_filled_play); 
-		if (harmony.play_offset)
+		if (harmony.play_offset) {
 			buf_to_fill--;
+			buf_to_fill += MAX_BUFS;
+		}
 		buf_to_fill %= MAX_BUFS;
-
+		
+		fresh_buffer = (harmony.play_offset == 0);
+		
 		/* Figure out the size of the frame */
-		if ((total_count-count) > HARMONY_BUF_SIZE - harmony.play_offset) {
+		if ((total_count-count) >= HARMONY_BUF_SIZE - harmony.play_offset) {
 			frame_size = HARMONY_BUF_SIZE - harmony.play_offset;
 		} else {
 			frame_size = total_count - count;
@@ -578,7 +583,7 @@
 		CHECK_WBACK_INV_OFFSET(played_buf, (HARMONY_BUF_SIZE*buf_to_fill + harmony.play_offset), 
 				frame_size);
 	
-		if (!harmony.play_offset)
+		if (fresh_buffer)
 			harmony.nb_filled_play++;
 		
 		count += frame_size;

Lines 560-563 fix the crash, which was caused by buf_to_fill taking the value -1 (since -1 % MAX_BUFS == -1).

Lines 540, 566, 586/587 should fix a bug where samples were skipped.

The change to line 569 prevents harmony_silence from being called whenever the final frame that is to be copied completely fills a buffer.

I'm a bit of a newbie here... can a harmony_interrupt occur whilst we're in harmony_audio_write?

Signature · 5 lines
Stuart Brady
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux