Bus mastering support for IDE CDROM audio

Reading audio from IDE CDROMs always uses PIO. This patch teaches the kernel to use DMA for the CDROMREADAUDIO ioctl.

Total time to read a CD using cdparanoia improves by up to 20%. But sometimes there is no change.

Total CPU load decreases greatly. On a 700 MHz VIA C3 with 82C6xx Southbridge the CPU load during the rip falls from 85% to 9%.

On an 850MHz P3 with piix chipset CPU load falls from 76% to 8%.

These loads cannot be observed with top' or ps' - it all happens at interrupt time. I measure with `cyclesoak'. It's googlable.

Recovery from media errors has been tested, works OK.

Recovery from DMA errors has been simulated. It falls back to PIO mode OK.

This code has not been tested for its effects upon SCSI-based CDROM readers. It needs to be.

I'd be interested in feedback from testers, please.

(Thanks, Jens)

@@ -1910,6 +1910,76 @@ static int cdrom_do_cmd(struct cdrom_dev
 	return ret;
 }
 
+/*
+ * CDROM audio read, with DMA support.  Added in 2.4.18-pre4, akpm.
+ */
+static int cdda_read_audio(int cmd,
+			struct cdrom_device_info *cdi,
+			struct cdrom_generic_command *cgc,
+			struct cdrom_read_audio *ra)
+{
+	int lba;
+	int frames_todo;
+	int ret;
+	void *xferbuf = 0;
+	char *useraddr;
+
+	ret = -EINVAL;
+	if (ra->addr_format == CDROM_MSF) {
+		lba = msf_to_lba(ra->addr.msf.minute,
+				 ra->addr.msf.second,
+				 ra->addr.msf.frame);
+	} else if (ra->addr_format == CDROM_LBA) {
+		lba = ra->addr.lba;
+	} else {
+		goto out;
+	}
+
+	if (lba < 0 || ra->nframes <= 0)
+		goto out;
+
+	/*
+	 * We just allocate a single frame for the transfer buffer.  It
+	 * would be nice to allocate a page vector, but that would require
+	 * s/g support for PIO mode in cdrom_pc_intr().  Later.
+	 *
+	 * Also, I cannot persuade my CDROM to DMA transfer more than a single
+	 * frame before it drops DRQ and stops.
+	 *
+	 * This code used to kmalloc a large transfer buffer.  Be aware that
+	 * in DMA mode we later wrap a single buffer_head around this memory,
+	 * and b_size is unsigned short.  64k limit.
+	 *
+	 * Andre says the disk can transfer too much data sometimes, hence
+	 * the extra 100 bytes.
+	 */
+	xferbuf = kmalloc(CD_FRAMESIZE_RAW + 100, GFP_KERNEL);
+	ret = -ENOMEM;
+	if (!xferbuf)
+		goto out;
+
+	cgc->buffer = xferbuf;
+	cgc->data_direction = CGC_DATA_READ;
+	cgc->do_dma = 1;
+	frames_todo = ra->nframes;
+	useraddr = ra->buf;
+	while (frames_todo) {
+		ret = cdrom_read_block(cdi, cgc, lba, 1, 1, CD_FRAMESIZE_RAW);
+		if (ret)
+			goto out;
+		ret = -EFAULT;
+		if (copy_to_user(useraddr, cgc->buffer, CD_FRAMESIZE_RAW))
+			goto out;
+		useraddr += CD_FRAMESIZE_RAW;
+		frames_todo--;
+		lba++;
+	}
+	ret = 0;
+out:
+	kfree(xferbuf);
+	return ret;
+}
+
 static int mmc_ioctl(struct cdrom_device_info *cdi, unsigned int cmd,
 		     unsigned long arg)
 {		

@@ -1973,57 +2043,9 @@ static int mmc_ioctl(struct cdrom_device
 		}
 	case CDROMREADAUDIO: {
 		struct cdrom_read_audio ra;
-		int lba, nr;
 
 		IOCTL_IN(arg, struct cdrom_read_audio, ra);
-
-		if (ra.addr_format == CDROM_MSF)
-			lba = msf_to_lba(ra.addr.msf.minute,
-					 ra.addr.msf.second,
-					 ra.addr.msf.frame);
-		else if (ra.addr_format == CDROM_LBA)
-			lba = ra.addr.lba;
-		else
-			return -EINVAL;
-
-		/* FIXME: we need upper bound checking, too!! */
-		if (lba < 0 || ra.nframes <= 0)
-			return -EINVAL;
-
-		/*
-		 * start with will ra.nframes size, back down if alloc fails
-		 */
-		nr = ra.nframes;
-		do {
-			cgc.buffer = kmalloc(CD_FRAMESIZE_RAW * nr, GFP_KERNEL);
-			if (cgc.buffer)
-				break;
-
-			nr >>= 1;
-		} while (nr);
-
-		if (!nr)
-			return -ENOMEM;
-
-		if (!access_ok(VERIFY_WRITE, ra.buf, ra.nframes*CD_FRAMESIZE_RAW)) {
-			kfree(cgc.buffer);
-			return -EFAULT;
-		}
-		cgc.data_direction = CGC_DATA_READ;
-		while (ra.nframes > 0) {
-			if (nr > ra.nframes)
-				nr = ra.nframes;
-
-			ret = cdrom_read_block(cdi, &cgc, lba, nr, 1, CD_FRAMESIZE_RAW);
-			if (ret)
-				break;
-			__copy_to_user(ra.buf, cgc.buffer, CD_FRAMESIZE_RAW*nr);
-			ra.buf += CD_FRAMESIZE_RAW * nr;
-			ra.nframes -= nr;
-			lba += nr;
-		}
-		kfree(cgc.buffer);
-		return ret;
+		return cdda_read_audio(cmd, cdi, &cgc, &ra);
 		}
 	case CDROMSUBCHNL: {
 		struct cdrom_subchnl q;

@@ -109,6 +109,7 @@ struct packet_command {
 	int quiet;
 	int timeout;
 	struct request_sense *sense;
+	int do_dma;
 	unsigned char c[12];
 };
 

@@ -287,6 +287,7 @@ struct cdrom_generic_command
 	unsigned char		data_direction;
 	int			quiet;
 	int			timeout;
+	int			do_dma;
 	void			*reserved[1];
 };

_Wonderful_ patch. I've been testing for the past hour or so on my Yamaha CRW2200E (hooked up to hdc, ide1, on VIA vt82c686b (rev 40)), and the cpu load has rarely peaked 7-9% with cdparanoia.

I have not had a chance to test with a SCSI reader yet because my local patchset has some unresolved symbols in a scsi module, but I expect to iron those out shortly.

Thanks again, Andrew.

Dan Chen                 [email protected]
GPG key:   www.unc.edu/~crimsun/pubkey.gpg.asc

Hello.

Works very fine here. I applied your patch on 2.4.18-pre3-ac2. System feels little smoother overall. Especially when I grab from 2 CD's simultaneously.

Will it be included in 2.4.19 ?

*Kristian

:... [snd.science] ...:
::
:: http://www.korseby.net
:: http://gsmp.sf.net
:.........................:: ~/$ [email protected] :

Initial testing on my system shows some good, some bad :-)

Asus A7V, 1.0GHz Athlon Thunderbird, 512M PC-133 CAS3 SDRAM VIA VT82C686A (rev 22) Ultra ATA 66
Promise PDC20265 Ultra ATA 100

There are two CD-ROM drives, master and slave on the secondary channel of the VIA controller. There are also four hard drives, spread across the rest of the channels (three of them on the Promise controller), configured as a software raid-5 array, with ext3 volumes on top of that. Kernel is 2.4.18-pre7, plus this patch, plus Ingo's O(1)-J6 scheduler patch.

The CD-ROM drives are both Creative CD-ROM Blaster 52X, but different vintages (so probably different manufacturers). One reports as "CREATIVE CD5233E-N", the other as "CREATIVE CD5233E-CF". The N drive is ATA Master, the CF drive is Slave. Both reported "using DMA" previously, even though reading audio never used DMA.

I am using cdda2wav to read the audio tracks, one track at a time, using native IDE mode (no ide-scsi emulation). When reading from the CF drive, everything works quite well, seems to use less CPU time (MP3 encoding running simultaneously gets more work done :-). When reading from the N drive, get lots of "cdrom_pc_intr: read too little data 0 < 2352", and it takes forever to read tracks (I actually never let it complete). Using hdparm to turn off the "using DMA" flag for the N drive results in normal read activity, just like before the patch went in.

Performance seems really good using the patch and DMA on the working drive; the WAV data is stored onto an ext3 volume, then read back from that volume and encoded into MP3, stored back onto the same volume. Encoding speed (while ripping) nearly doubled! Previously, just using Ingo's new scheduler had improved encoding speed a little (less than 10%), but this has made a huge improvement.

I don't have any spare IDE channels in this box, otherwise I'd split these drives onto separate channels. I may solve that problem later today, and be able to provide more useful debugging. If there's anything else you'd like to know, just ask.

I should note that I have only devices marked "master" on both IDE channels (1 HD on ide0, 1 ATAPI cdrw on ide1).

Dan Chen                 [email protected]
GPG key:   www.unc.edu/~crimsun/pubkey.gpg.asc

OK, thanks Kevin (Dan, Kristian, Grant..)

Seems that some devices simply terminate their DMA in a normal manner, report no errors and don't tell us how much data they transferred. From my reading of the ATA spec, they're allowed to do that - they only need to report the transfer byte count in PIO mode.

Could you please change the code in drivers/ide/ide-cd.c:cdrom_pc_intr() to:

                if ((stat & DRQ_STAT) == 0 && len < pc->buflen) {
                        printk(__FUNCTION__ ": read too little data! %d < %d\n",
                                        len, pc->buflen);
+                       len = pc->buflen;
                }
                pc->buflen -= len;
                pc->buffer += len;

and let me know if the thing actually reads audio correctly?

Also, please tell me whether that particular drive reads normal
ISO filesystems correctly in DMA mode? Thanks.

-

There's an updated patch at

	http://www.zip.com.au/~akpm/linux/2.4/2.4.18-pre7/ide-akpm.patch

It now supports multi-frame transfers and should fix the problem which you observed.

-

Your first patch behaves much better as the second. It seems that my drives are running with PIO only again. I see no problems with your first patch.

cdparanoia with ide-akpm-1:

/dev/scd0: (CPU ~40%)

	real    1m10.194s
	user    0m6.560s
	sys     0m2.880s
/dev/scd1: (CPU ~40%)
	real    2m8.283s
	user    0m7.230s
	sys     0m3.890s

cdparanoia with ide-akpm-2:

/dev/scd0: (CPU ~80%)

	real    1m37.472s
	user    0m7.340s
	sys     0m3.290s
/dev/scd1: (CPU ~50%)
	real    2m40.879s
	user    0m7.650s
	sys     0m3.620s

dd if=/dev/scd0 of=test.iso:

And dd consumes about 5% CPU with your first patch and almost 90% with your second patch as I would disable dma on it (and the system gets really slow responding).

My specs:

hdc: LTN526S, ATAPI CD/DVD-ROM drive
hdd: Hewlett-Packard CD-Writer Plus 9100b, ATAPI CD/DVD-ROM drive

I'm using ide-scsi:
scsi0 : SCSI host adapter emulation for IDE ATAPI devices

  Vendor: LITEON    Model: CD-ROM LTN526S    Rev: YS0A
  Type:   CD-ROM                             ANSI SCSI revision: 02
  Vendor: HP        Model: CD-Writer+ 9100b  Rev: 1.06
  Type:   CD-ROM                             ANSI SCSI revision: 02

I'm using 2.4.18-pre3-ac2. Maybe there're some collisions with Andre's IDE patches ?

*Kristian

:... [snd.science] ...:
::
:: http://www.korseby.net
:: http://gsmp.sf.net
:.........................:: ~/$ [email protected] :

Hello again.

Ok. Sorry. My fault. The second patch produces the same throughput... I didn't realized that the kernel disabled DMA during rebooting. My drives only went to DMA again after a cold boot. Don't know what's going on here. But after a normal reboot, my drives are in PIO only and don't support DMA.

cdparanoia on /dev/scd0 now gives the same result as with the first patch.

real    1m8.055s
user    0m6.740s
sys     0m2.850s

*Kristian

:... [snd.science] ...:
::
:: http://www.korseby.net
:: http://gsmp.sf.net
:.........................:: ~/$ [email protected] :

You might want to try sending your cdroms into sleep when dma mode wont
work anymore. That usually fixes things for me. hdparm -Y ....the
kernel should wake them up immediately and things should work again.

OK, Gents. Version three is at

	http://www.zip.com.au/~akpm/linux/2.4/2.4.18-pre7/ide-akpm-3.patch

This attempts to overcome the situation where a drive/controller doesn't want to perform multiframe DMA, but is happy performing single-frame DMA.

So:

- We start out trying to perform multiframe DMA. If we get a DMA error, we fall back to single-frame DMA.

- If we get a DMA error in single frame mode, we fall back to multi-frame PIO.

At no stage does a packet-mode DMA error turn off drive-level DMA. This is because some devices seem to perform ordinary ATA DMA OK, but screw up packet DMA.

The kernel internally retries the requests when it performs fallback, so userspace shouldn't see any disruption as the kernel works out what to do.

Once the drive has fallen back to single-frame (or PIO mode) for packet reads, the only way to get it back to a higher level is a reboot.

-

Doesn that mean that a bad media (typically a scratched CDROM) will cause the drive to revert to PIO until next reboot ?

Ben.

Nope. This error handling is specifically for busmastering errors, not for media errors.

I've tested media errors (whiteboard marker scribblings on the CD do this nicely). DMA errors (bad return value from HWIF->dmaproc) I can only simulate.

-

I've always been able to get it back to dma for packet by forcing the
drive to sleep mode and then letting the kernel wake it. I guess I'll
try this 3rd version patch when I get back from class today and see if that still works.

hdparm -Y /dev/cdrom

then go and set dma again with hdparm.

Although this could just be fickleness of my cdrom.

It works for the second one (the HP writer). But the kernel completely hangs for 2 seconds trying to wake up the drive (with 2.4.18-pre3-ac2).

My log shows:
hdd: DMA disabled
hdd: drive not ready for command
hdd: ATAPI reset complete

But preemption does help in that case.

The first cd-rom drive does not support sleep or standby commands. ;-(

*Kristian

:... [snd.science] ...:
::
:: http://www.korseby.net
:: http://gsmp.sf.net
:.........................:: ~/$ [email protected] :

The third patch works fine.

When I'm doing "hdparm -c0 /dev/hdc" (16bit I/O) the system respond-ratio gets very low. (with all ide-cd-patches) I can see when gtk draws it's icons... But when I switch I/O back to 32 bit linux gets responsive again and cpu load decreases (~20%).

Tested with 2.4.18-pre3-ac2 and 2.4.17-lowlat-ide.

*Kristian

:... [snd.science] ...:
::
:: http://www.korseby.net
:: http://gsmp.sf.net
:.........................:: ~/$ [email protected] :

Andrew,

I wanted to confirm success of testing the patch with a SCSI CD-ROM (Plextor UltraPlex Wide on aic7xxx). I used your updated patch off your website.

Audio rip completed without error. Performance seems the same, which I assume is to be expected with SCSI readers.

Robert Love

sr already uses DMA for all transfers, so no performance gain was to be expected there. problem is ide-cd using pio for all packet command data transfers currently (modulo fs read write requests, of course)

not a whole lot of pio aic7xxx adapters out there :-)

Jens Axboe

Right. The patch touches the generic cdrom driver, however, so Andrew asked for testing with SCSI readers. Thankfully it works; of course performance didn't go up.

Robert Love

Yup. Rob was looking for regression - I'm not set up to test SCSI CDROMs here.

The second patch goes back to reading a bunch of frames all inside the same request, rather than one frame at a time. This is because the cdparanoia guys tell me that it can prevent single-request overruns and underruns and other data loss which occurs around the start and end of the request. So with a walking-window read algorithm from userspace they can pick up data which would otherwise be lost.

Also it seems that some devices aren't happy with the larger transfers, so it looks like the algorithm needs to become:

- Try multiple frames, DMA
- If that fails, try single frames, DMA
- If that fails, fall back to PIO

While all the time not altering the DMA status of the drive for block-based filesystem I/O.

Thank heavens for that (I _knew_ I shouldn't have stuck my nose in drivers/ide/).

-

It works very well on my system; I'm now able to rip audio from both drives in DMA mode (one in UDMA-2, the other in MDMA-2) simultaneously. Even though they are on the same IDE cable, rip performance increased slightly (average of 12x per drive, instead of 11x), and CPU usage dropped drastically (less then 10% usage, even with the WAV files going onto a software RAID-5 array with ext3 filesystem).

Great job Andrew!

----- Original Message -----
From: "Andrew Morton" <[email protected]>
To: "Kevin P. Fleming" <[email protected]>; "lkml" <[email protected]>; "Grant" <[email protected]>
Sent: Saturday, January 26, 2002 11:41 PM
Subject: Re: [CFT] Bus mastering support for IDE CDROM audio