[PATCH] CD-ROM improvements

Discussion of development and patch submission.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

[PATCH] CD-ROM improvements

Post by Battler »

The patch is attached. Here's what it does:
- Improves IDE loading - makes CD-ROM always the 1st slave, except for when no hard disks are present in which case it disables the CD-ROM so some BIOS'es don't freak out about it, and makes sure the hard disks fill the slots in order, which makes sure there's no slave without master;
- Changes the entire ATAPI implementation to the one from PCem-X which adds several ATAPI commands previously not emulated, as well as better disc change handling (code partially written by SA1988 too);
- IOCTL changes to accomodate the above;
- Ability to load ISO images (most of the code written by RichardG867).
Attachments
pcem_cdrom.rar
(18.11 KiB) Downloaded 217 times
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

New version attached. Differs only in that I removed the modification (added -g switch) to makefile.mingw.
Attachments
pcem_cdrom.rar
(18.12 KiB) Downloaded 211 times
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

.ISO support committed to rev 414.

Not too sure about the other changes though. I don't see the need for the hard drive loading changes, frankly if users want to load slave drives without masters and get the same issues they'd get on real hardware with the same setup, that's up to them. Forcing the CD-ROM to primary slave prevents the use of 2 hard drives plus CD-ROM on older BIOSes, which is a valid setup on real hardware (with a dual-channel IDE controller or a sound card with an IDE port) that is supported by OAKCDROM.SYS and probably some other drivers. I also don't see the need to replace the entire ATAPI implementation - new commands are nice, but you've changed a lot of the existing code for what seems like no good reason, without improving it.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

Without improving it? So disc changes being handled correctly and VIDE-CDD.SYS actually detecting the CD-ROM drive is not an improvement? And yes, the loading change is also needed because VIDE-CDD.SYS seems to fail to detect a CD-ROM drive if it's master. Maybe a better way would be to allow the user to specify what IDE channel to set the CD-ROM to instead of forcing a specific one. That way both configurations can be accomodated.
Also the only changes *I* did to the API part was redoing the disc change detection since the old one was bad enough that NT 3.x and VIDE-CDD.SYS both failed to detect disc change and kept causing old contents to be listed. SA1988 did everything else. I have no idea why he didn't just add some commands without changing other things, but I don't see the harm, either. In my opinion, the emulator would benefit more from accepting the changes than from not accepting them.
Edit: I also changed makefile.mingw64 to add several files into it that you forgot to add to it.
Edit #2: Your cdrom_ioctl.c is designed for outdated 32-bit MinGW/GCC, because already since 4.9.x., the path to the DDK CD-ROM include has been same in both 64-bit and 32-bit (and certainly not the same as in 32-bit 4.7.x/4.8.x), and they also added the TOC definition there. This is the only thing preventing compiling mainline PCem on GCC 4.9.x and later. Maybe a GCC version check would be in order there.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

New ATAPI patch is attached. It does this:
- Rewrites the disc changed / not ready handler because the old one misbehaved with several DOS CD-ROM drivers, as well as with NT 3.x;
- Makes the CD-ROM IDE channel fully customizable, and defaulting to 2 (secondary master);
- Adds several new ATAPI commands.

I tried my best to keep changes to those that were necessary. Hopefully this one will be OK.
Attachments
pcem_atapi.patch
(60.71 KiB) Downloaded 203 times
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

Battler wrote:Without improving it? So disc changes being handled correctly and VIDE-CDD.SYS actually detecting the CD-ROM drive is not an improvement?
Yes, that's an improvement, but changing all the #defines and playing with the indentation is not improving it, it's just changing code for the sake of it. It also makes it very difficult to see what actual improvements have been made as it shows that _everything_ has changed.

Also, if you don't want me to assume they're all your changes then say who's responsible! I'm not psychic, and if you post patches without saying who made the changes (and you didn't originally say specifically what changes SA1988 made) then I'm going to assume you did.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

Going to upload a new version of the ATAPI patch soon, as I have since found out the above's handling of disc changes is a bit awkward, so I have redone it.
neozeed
Posts: 176
Joined: Tue 08 Jul, 2014 4:41 am
Location: Hong Kong SAR
Contact:

Re: [PATCH] CD-ROM improvements

Post by neozeed »

I really hate my cdrom code I'm using in my basilisk port, are these patches gpl able? I have no issues giving attribution, I'd love to just try them.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

- neozeed: They are intended for PCem which is GPL, so they are obviously GPL themselves.
neozeed
Posts: 176
Joined: Tue 08 Jul, 2014 4:41 am
Location: Hong Kong SAR
Contact:

Re: [PATCH] CD-ROM improvements

Post by neozeed »

cool, just wanted to make sure
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

New patach attached. Adds various improvements as well as bugfixes over the previous (and the disc change detection now actually *WORKS*, verified with several pieces of software). Also made IDE set signature on Reset ATAPI device and Drive diagnostics commands, per the spec, and writes to IDE data buffer are now discarded after packet command (A0) is issued and the 12 bytes (6 words) are written, unless a command specifically requests a write. This fixes problems with software that issues excess writes after issuing packet command. Also added the audio-related senses and made PLAY AUDIO correctly fail if either an ISO is inserted or the command was issued on a non-audio track.
Attachments
cdrom_patch_latest.patch
(98.6 KiB) Downloaded 161 times
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

And yet another version of the patch, with MODE SELECT-related fixes. Page 0x0E (CDROM AUDIO page) is now actually changeable (though the changes have no practical effect - but otherwise, LBA 2 hangs at the volume menu when attempting to change CD audio volume) and fixed some of the atastat and secount values used by MODE SELECT so it actually starts writing.
Attachments
cdrom_patch_latest_updated.patch
(102.89 KiB) Downloaded 163 times
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

Updated again. Now disc changes within a host drive while the drive remains mounted inside PCem are visible to the emulated machine.

Edit #2: Removed two pclog lines from the patch that I forgot to remove before.
Attachments
cdrom_patch_latest_updated_again.patch
(106.02 KiB) Downloaded 156 times
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

Another update. The ATAPI CDROM AUDIO mode page volume settings now actually affect CD Audio volume. Makes the CD Audio volume bar in LBA 2 work.
Attachments
cdrom_patch_latest_updated_again_2.patch
(107.66 KiB) Downloaded 165 times
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

I haven't forgotten these changes, I just haven't had much chance lately to look at them. I've attached the current state - mostly I've changed code formatting (tabs are 8 characters wide!) and tried to neaten it up a bit.

I noticed that changing CD in Windows 95 is broken, that will have to be fixed before this can be committed.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

- TomWalker: The last update of my patch broke CD changing in Windows 95, but I have discovered that only after I posted it here. I'm still investigating why it's broken.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

Fixed the Windows 95 disc change problem. New patch attached.
Attachments
cdrom_patch_win95fixed.patch
(108.61 KiB) Downloaded 165 times
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

Finally started getting this committed - CD-ROM channel selection committed at rev 449.
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

Quick question -

Code: Select all

		case GPCMD_GET_EVENT_STATUS_NOTIFICATION: /*0x4a*/
		if (idebufferb[1] != 0)
		{
			ide->atastat = READY_STAT | ERR_STAT;
			ide->error = (SENSE_ILLEGAL_REQUEST << 4) | ABRT_ERR;
			if (atapi_sense.sensekey == SENSE_UNIT_ATTENTION)
			{
				ide->error |= MCR_ERR;
			}
			atapi_sense.asc=ASC_INV_FIELD_IN_CMD_PACKET;
			ide->packetstatus = ATAPI_STATUS_ERROR;
			idecallback[ide_board]=50*IDE_TIME;
			break;
		}
			
		uint8_t events, class;
			
		events = 1 << GESN_MEDIA;
		class = 0;
			
		if (idebufferb[4] & (1 << GESN_MEDIA))
		{
			class |= GESN_MEDIA;
			len = atapi_event_status(ide, idebufferb);
		}
		else
		{
			class = 0x80;
			len = idebufferb[4];
		}
		len=(idebufferb[7]<<8)|idebufferb[8]; 
			
		ide->cylinder=len;
		ide->packetstatus = ATAPI_STATUS_DATA;
		ide->secount=2;
		ide->pos=0;
		idecallback[ide_board]=50*IDE_TIME;
		ide->packlen=len;
		break;
events and class are calculated but never used. Is this just dead code or did something go wrong when generating the patch?
SA1988
Posts: 268
Joined: Wed 30 Apr, 2014 9:38 am

Re: [PATCH] CD-ROM improvements

Post by SA1988 »

I tried porting it from QEMU, but yeah, it's still primitive about that command.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

- TomWalker: That's a part I asked SA1988 to port from QEMU but evidently, as per usual, he only ported like half of it and claimed he was done. I am going to have a look at it later and see what he did and what's missing.
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

Correct implementation of the GPCMD_GET_EVENT_STATUS_NOTIFICATION command:

Code: Select all

		case GPCMD_GET_EVENT_STATUS_NOTIFICATION: /*0x4a*/
				temp_command = idebufferb[0];
				alloc_length = len;

				const uint8_t *packet = idebufferb;
				
				struct {
					uint8_t opcode;
					uint8_t polled;
					uint8_t reserved2[2];
					uint8_t class;
					uint8_t reserved3[2];
					uint16_t len;
					uint8_t control;
				} *gesn_cdb;
				
				struct {
					uint16_t len;
					uint8_t notification_class;
					uint8_t supported_events;
				} *gesn_event_header;
				unsigned int max_len, used_len;
				
				gesn_cdb = (void *)packet;
				gesn_event_header = (void *)idebufferb;
				
				max_len = gesn_cdb->len;
				
				/* It is fine by the MMC spec to not support async mode operations */
				if (!(gesn_cdb->polled & 0x01))
				{	/* asynchronous mode */
					/* Only pollign is supported, asynchronous mode is not. */
                        ide->error = (SENSE_ILLEGAL_REQUEST << 4) | ABRT_ERR;
                        idecallback[ide_board]=50*IDE_TIME;
						atapi_cmd_error(ide, atapi_sense.sensekey, atapi_sense.asc);
						ide->atastat = 0x53;
                        ide->packetstatus=0x80;
						atapi_sense.sensekey = SENSE_ILLEGAL_REQUEST;
                        atapi_sense.asc = ASC_INV_FIELD_IN_CMD_PACKET;
						atapi_sense.ascq = 0;
                        return;
				}
				
				/* polling mode operation */
				
				/*
				 * These are the supported events.
				 *
				 * We currently only support requests of the 'media' type.
				 * Notification class requests and supported event classes are bitmasks,
				 * but they are built from the same values as the "notification class"
				 * field.
				 */
				gesn_event_header->supported_events = 1 << GESN_MEDIA;
				
				/*
				 * We use |= below to set the class field; other bits in this byte
				 * are reserved now but this is useful to do if we have to use the
				 * reserved fields later.
				 */
				gesn_event_header->notification_class = 0;
				
				/*
				 * Responses to requests are to be based on request priority.  The
				 * notification_class_request_type enum above specifies the
				 * priority: upper elements are higher prio than lower ones.
				 */
				if (gesn_cdb->class & (1 << GESN_MEDIA))
				{
					gesn_event_header->notification_class |= GESN_MEDIA;
					used_len = atapi_event_status(ide, idebufferb);
				}
				else
				{
					gesn_event_header->notification_class = 0x80; /* No event available */
					used_len = sizeof(*gesn_event_header);
				}
				gesn_event_header->len = used_len - sizeof(*gesn_event_header);

				atapi_command_send_init(ide, temp_command, len, alloc_length);

				atapi_command_ready(ide_board, len);
				break;
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

How much of this code has been taken from QEMU?
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

This that I posted tonight? Ported from QEMU in its entirety.
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

And in the main patch?
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

I admit, that last change has me slightly dubious. GPCMD_GET_EVENT_STATUS_NOTIFICATION was clearly never tested, as otherwise you'd have noticed the implementation was essentially broken. How much of the rest of the code is like that?
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

- TomWalker: Only that part wasn't tested, and that's simply because I don't have a single piece of software that used it. The rest was not only tested, but for a lot of it I even went through each command in an ATAPI reference I found to make sure it's implemented correctly.

Edit: And in the main patch, there is not much from QEMU, especially since most if not all of the new commands added other than GET EVENT STATUS NOTIFICATION were not even present in the QEMU code, and the disc change detection was originally ported from QEMU, but then I redid it completely after I found out just how inadequate the QEMU implementation of it was.
User avatar
SarahWalker
Site Admin
Posts: 1885
Joined: Thu 24 Apr, 2014 4:18 pm

Re: [PATCH] CD-ROM improvements

Post by SarahWalker »

Committed at rev 450.
User avatar
FredPJ
Posts: 31
Joined: Tue 15 Sep, 2015 1:55 pm

Re: [PATCH] CD-ROM improvements

Post by FredPJ »

Please forgive my ignorance, but does this solve the infamous "DOS drivers (GSCDROM.SYS) stalling then not detecting the drive when the drive is empty" bug?
Battler
Posts: 793
Joined: Sun 06 Jul, 2014 7:05 pm

Re: [PATCH] CD-ROM improvements

Post by Battler »

- FredPJ: It should, I have tested it with VIDE-CDD.SYS version 2.15 which had the same problem and it works now.
Post Reply