S3 render fix patch

[Battler]

This patch fixes an acceleration-caused rendering problem for both 8bpp and 32bpp modes. The problem is that icon rendering in some OS'es was incorrect (purple background, etc.) in 8bpp but correct in 32bpp. Then an update to the code reversed the problem, making the rendering correct in 8bpp but incorrect in 32bpp. What this does is basically using the original two lines if the mode is 32bpp, and the new ones if 8bpp. The 4bpp and 16bpp modes are not affected as they always render correctly.

I came up with the solution and my friend SA1988 uploaded it to his site in .patch format: http://towg.tk/Emulators/video.patch.

[SarahWalker]

Under what OSes does this occur? What drivers?

[SA1988]

this mainly occurs on NT 3.1 with PCI S3 Trio64 drivers from 1995 (the only ones that enable 1024x768x32bpp under that OS), and I've tested this patch earlier, makes the background render correctly.

[SarahWalker]

I won't accept the patch as-is, as it's a hack rather than a proper fix (the S3 blitter has no knowledge whatsoever of the current display depth - that's a property of the RAMDAC, and isn't even on the same chip on 864 boards). I will look into the issue though.

[Battler]

I think I just found a better way to fix it:

Code: Select all

                                else if ((s3->accel.cmd & 0x600) == 0x200)
                                {
					if ((s3->accel.cmd & 0x700) == 0x300)
					{
                                        	s3_accel_start(2, 1, 0xffffffff, val >> 16, s3);
                                        	s3_accel_start(2, 1, 0xffffffff, val,       s3);
					}
					else
					{
                                        	s3_accel_start(2, 1, 0xffffffff, val,       s3);
                                        	s3_accel_start(2, 1, 0xffffffff, val >> 16, s3);
					}
                                }[/quote]
I tested, and it works correctly.
Edit: Windows 3.1 didn't have problems in 256 colors anyway. I need an install of Windows NT 3.1 to check. So disregard the above code until I investigate further.

[Battler]

So I logged some values (addr, val, accel.cmd, and all of accel.multifunc):

Windows 3.1 32bpp 640x80 (where writing the value's 16-bit components in reverse renders correctly):

Code: Select all

Write S3 accel 000A007C 00000000 53B1 [001F] [0000] [0000] [0665] [027F] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0240] [0007]

Windows NT 3.1 8bpp 640x480 (where writing the value's 16-bit components in forward renders correctly):

Code: Select all

Write S3 accel 000A001C 00000000 53B1 [001F] [0000] [0000] [0998] [027F] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0040] [0000]

Edit: Windows NT 3.1 32bpp 640x480:

Code: Select all

Write S3 accel 000A0424 00C0C0C0 53B1 [0060] [0000] [0000] [0665] [027F] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0000] [0040] [0000]

The are the values in order:

Code: Select all

addr val accel.cmd accel.multifunc[0] accel.multifunc[1] accel.multifunc[2] accel.multifunc[3] accel.multifunc[4] accel.multifunc[5] accel.multifunc[6] accel.multifunc[7] accel.multifunc[8] accel.multifunc[9] accel.multifunc[0χa] accel.multifunc[0χb] accel.multifunc[0xc] accel.multifunc[0xd] accel.multifunc[0xe] accel.multifunc[0xf]

These is my attempt at an exaplanation of them:

Code: Select all

Address Value Command [Unknown 0] [Zero] [Zero] [Unknown 1] [H. Resolution] [Zero] [Zero] [Zero] [Zero] [Zero] [Zero] [Zero] [Zero] [Zero] [Unknown 2] [Unknown 3]

Unknown 0 is the same (0x001F) in both cases, so it's irrelevant here.
Unknown 1 is 0x0665 in the former case, and 0x0998 in the latter - 0x0665 in NT 3.1 at 32bpp. 0x3FF in Windows 3.1 at 32bpp if set to 1024x768.
Unknown 2 is 0x0240 in the former case, and 0x0040 in the latter, and in NT 3.1 at 32bpp.
Unknown 3 is 0x0007 in the former case, and 0x0000 in the latter, and in NT 3.1 at 32bpp.

It seems now that it's bit 0 Unknown 1 (multifunc[3]) that controls the order of the data sent. If it's set, it's sent in reverse, otherwise, it's sent normally.

Edit: This is how s3_accel_write_l in vid_s3.c is supposed to look like if you take in account what I discovered here:

Code: Select all

void s3_accel_write_l(uint32_t addr, uint32_t val, void *p)
{
        s3_t *s3 = (s3_t *)p;
//        pclog("Write S3 accel l %08X %08X\n", addr, val);
        if (addr & 0x8000)
        {
                s3_accel_write(addr,     val,        p);
                s3_accel_write(addr + 1, val >> 8,   p);
                s3_accel_write(addr + 2, val >> 16,  p);
                s3_accel_write(addr + 3, val >> 24,  p);
        }
        else
        {
                if (s3->accel.cmd & 0x100)
                {
                        if ((s3->accel.multifunc[0xa] & 0xc0) == 0x80)
                        {
                                if (s3->accel.cmd & 0x1000)
                                        val = ((val & 0xff000000) >> 24) | ((val & 0x00ff0000) >> 8) | ((val & 0x0000ff00) << 8) | ((val & 0x000000ff) << 24);
                                if ((s3->accel.cmd & 0x600) == 0x400)
                                        s3_accel_start(32, 1, val, 0, s3);
                                else if ((s3->accel.cmd & 0x600) == 0x200)
                                {
                                        s3_accel_start(16, 1, val >> 16, 0, s3);
                                        s3_accel_start(16, 1, val, 0,       s3);
                                }
                                else if (!(s3->accel.cmd & 0x600))
                                {
                                        s3_accel_start(8, 1, val >> 24, 0, s3);
                                        s3_accel_start(8, 1, val >> 16, 0, s3);
                                        s3_accel_start(8, 1, val >> 8,  0, s3);
                                        s3_accel_start(8, 1, val,       0, s3);
                                }
                        }
                        else
                        {
                                if ((s3->accel.cmd & 0x600) == 0x400)
                                        s3_accel_start(4, 1, 0xffffffff, val, s3);
                                else if ((s3->accel.cmd & 0x600) == 0x200)
				{
                                	if (s3->accel.multifunc[3] & 0x1)
                                        	val = ((val & 0xffff0000) >> 16) | ((val & 0x0000ffff) << 16);
                                       	s3_accel_start(2, 1, 0xffffffff, val,       s3);
                                       	s3_accel_start(2, 1, 0xffffffff, val >> 16, s3);
				}
                                else if (!(s3->accel.cmd & 0x600))
                                {
                                	if (s3->accel.multifunc[3] & 0x1)
                                        	val = ((val & 0xff000000) >> 24) | ((val & 0x00ff0000) >> 8) | ((val & 0x0000ff00) << 8) | ((val & 0x000000ff) << 24);
                                        s3_accel_start(1, 1, 0xffffffff, val,       s3);
                                        s3_accel_start(1, 1, 0xffffffff, val >> 8,  s3);
                                        s3_accel_start(1, 1, 0xffffffff, val >> 16, s3);
                                        s3_accel_start(1, 1, 0xffffffff, val >> 24, s3);
                                }
                        }
                }
        }
}

Tested, and Windows 3.1 and Windows NT 3.1 now render fine in all supported modes.

[Battler]

Alright, please disregard my previous post, I just found what seems to be the real fix.
In s3_accel_start in vid_s3.c, find this:

Code: Select all

cpu_dat = (cpu_dat & 0xffff) | (s3->accel.dat_buf << 16);

And replace it with:

Code: Select all

cpu_dat = ((cpu_dat & 0xffff) << 16) | s3->accel.dat_buf;

For some reason, this code, as it originally was, caused the accelerator to expect all the words being sent in reverse order when in 32bpp. This fixes it, and makes everything render correctly in 32bpp. I even tested the game The Greens, for Windows 3.1, that TheCollector1995 reported rendered incorrectly with all the previous fixes to this render problem, and now it renders 100% correctly.

Edit: Just to make it clear, all other fixes mention in this thread should be disregarded, and if you had applied them already, remove them and only apply the fix mentioned in this post.