hrydgard / ppsspp

A PSP emulator for Android, Windows, Mac and Linux, written in C++. Want to contribute? Join us on Discord at https://discord.gg/5NJB6dD or just send pull requests / issues. For discussion use the forums at forums.ppsspp.org.
https://www.ppsspp.org
Other
11.1k stars 2.16k forks source link

Phantom Kingdom Portable (NPJH50451) black screen on boot with Function Replacements enabled #6665

Closed Kalanyr closed 9 years ago

Kalanyr commented 10 years ago

First responsible commit (by bisection): https://github.com/hrydgard/ppsspp/commit/a7b9ce205b6c43b52c71ebaf95f9bf599b4ccc50

Last unaffected commit is: https://github.com/hrydgard/ppsspp/commit/986d54039c450faf8945c06ab1527b88db979cd7

The cause is actually FuncReplacement so https://github.com/hrydgard/ppsspp/commit/a7b9ce205b6c43b52c71ebaf95f9bf599b4ccc50 is only "responsible" because it's the first build where FuncReplacement is enabled by default.

Reproduction Method: 1) Boot Phantom Kingdom Portable with FuncReplacement enabled.

Temporary Workarounds: The issue is with FuncReplacement though, so even the latest build works if FunctionReplacement is set to False in ppsspp.ini.

sum2012 commented 10 years ago

v0.9.9-44-g7675037 info log https://gist.github.com/sum2012/8094e6ee001a14ad8fb3 debug log https://drive.google.com/file/d/0B3OaSdeV0L8kdmtZX2gtMEFBeWM/edit?usp=sharing

hrydgard commented 10 years ago

Would be interesting if you could narrow down which function replacement it is, by commenting them out in ReplaceTables.cpp lines around 550. Anyway, I'm going to disable a bunch of them as several seem to cause problems.

sum2012 commented 10 years ago

comment { "memset", &Replace_memset, 0, 0 }, work info log: https://gist.github.com/sum2012/13f168b9bd30e1a1e380 debug log: https://drive.google.com/file/d/0B3OaSdeV0L8kbzRpc2ppTVRjYVU/edit?usp=sharing

unknownbrackets commented 10 years ago

What if you log the memsets?

static int Replace_memset() {
    u32 destPtr = PARAM(0);
    u8 *dst = Memory::GetPointerUnchecked(destPtr);
    u8 value = PARAM(1);
    u32 bytes = PARAM(2);

    NOTICE_LOG(HLE, "memset(%08x, %02x, %08x)", destPtr, (u32)value, bytes);

What are the last ones before it dies?

-[Unknown]

sum2012 commented 10 years ago

48:42:000 user_main N[HLE]: HLE\ReplaceTables.cpp:229 memset(08fb10a8, 00, 00000010) 48:42:001 user_main E[G3D]: GPUCommon.cpp:490 DL PC = 00000000 WTF!!!! 48:42:003 user_main N[HLE]: HLE\ReplaceTables.cpp:229 memset(08fed000, 00, 00002000)

info log: https://gist.github.com/sum2012/a79c72ce7fd005125b6e debug log: https://drive.google.com/file/d/0B3OaSdeV0L8keGJac0NWT1YwbFk/edit?usp=sharing

unknownbrackets commented 10 years ago

Does it work if you keep the function, but replace:

{ "memset", &Replace_memset, 0, 0},

With:

{ "memset", &Replace_memset, 0, REPFLAG_HOOKENTER},

This will still zero out memory, but it will also execute the function and update registers, etc.

-[Unknown]

sum2012 commented 10 years ago

Yes it work info log: https://gist.github.com/sum2012/0492f001435d2c20ed17 debug log: https://gist.github.com/sum2012/e95a6aeb68cebe7c82b4

unknownbrackets commented 10 years ago

So, then, it's probably not what regions are being set to 0, but rather side effects like stack space or registers.

Apparently this is eabb9c1b4f83d2b4:52. Not sure which that's from... oh, Crisis Core uses the same one. It's short:

    t1 = size_a2;
    a3 = size_a2 - 1;
    t0 = dst_a0;
    a2 = a3;

    while (t1 != 0)
    {
        t1 = a3;
        sb     val_a1, 0x0(t0)
        a3 = a2 - 1;
        t0++;
        a2 = a3;
    }

    // a0 = dst
    // a1 = val
    // a2 = -1
    // a3 = -1
    // t0 = dst + size
    // t1 = 0

    // at, v1, t2+ -> undisturbed
    // no stack space

    return a0;

Want to replace memset for the GPU stuff. Worst case I guess we could just hook it.

-[Unknown]

unknownbrackets commented 10 years ago

What if you turn it back to a normal replacement (remove the hook flag we added), and then have it set the registers?

Specifically:

    if (!skip) {
        memset(dst, value, bytes);
    }
    RETURN(destPtr);

    currentMIPS->r[MIPS_REG_A2] = -1;
    currentMIPS->r[MIPS_REG_A3] = -1;
    currentMIPS->r[MIPS_REG_T0] = destPtr + bytes;
    currentMIPS->r[MIPS_REG_T1] = 0;

Does it work, or still not?

-[Unknown]

sum2012 commented 10 years ago

MIPS_REG_T0 and MIPS_REG_T1 are undefined

unknownbrackets commented 10 years ago

Well, I guess that enum still calls them A4/A5, sorry.

-[Unknown]

sum2012 commented 10 years ago

Still do not work,black screen edit:the log are the same with first one https://gist.github.com/sum2012/b2c8d08102eba098ce56

unknownbrackets commented 10 years ago

Okay, then I really think something is wrong with our replacement mechanics. Hmm. Or else, it has to do with rescheduling during the memset() or cycles, but neither sounds very likely.

-[Unknown]

unknownbrackets commented 10 years ago

Does it help to remove this line from MIPS/x86/Jit.cpp?

js.downcountAmount = 0;  // we just subtracted most of it

It appears twice, remove both times. I think it's incorrect. But, hmm, still doesn't seem enough to break this...

What if you increase the memset time:

return 10 + bytes / 4;  // approximation

Make it like:

return 8 + bytes * 5;  // approximation

-[Unknown]

sum2012 commented 10 years ago

keep this change: currentMIPS->r[MIPS_REG_A2] = -1; currentMIPS->r[MIPS_REG_A3] = -1; currentMIPS->r[MIPS_REG_A4] = destPtr + bytes; currentMIPS->r[MIPS_REG_A5] = 0;

and add above 2 changes

Now stop in "Now loading" screen info log https://gist.github.com/sum2012/1749a24ea0c57e1f4455 debug log https://gist.github.com/sum2012/728f01471d6782362761

sum2012 commented 10 years ago

return 8999; same result edit: 8998 black screen

unknownbrackets commented 9 years ago

It seems that return 5 + 6 * bytes + 2; makes it work, from a naive translation of the opcodes to 1 cycle each as jit/interp normally do. From a log, it does lots of very large memset()s (seems to do all of vram), and I guess it expects these and its other work to all add up to a full vblank. The one that works has an interrupt trigger right before the other one breaks.

We might just change it to that higher estimate. Unfortunately, it may penalize more efficient memset() implementations. Grr.

-[Unknown]

hrydgard commented 9 years ago

@unknownbrackets well, we can differentiate different memset implementations with different estimates as they will have different hashes.. like my memcpy_jak

daniel229 commented 9 years ago

These problematic games are using the same memcpy and memset as Jak and Daxter

unknownbrackets commented 9 years ago

Does this still happen now with the latest time estimate in those funcs, then? It should match non-replacements.

-[Unknown]

daniel229 commented 9 years ago

No more black screen at start up with the latest build.