Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle async IO more correctly (testing/review) #939

Merged
merged 17 commits into from
Mar 11, 2013

Conversation

unknownbrackets
Copy link
Collaborator

This is a larger change and probably could use more testing. I recommend merging only after 0.7, but it'd be great to know if this helps or hurts any games I don't have / haven't tested yet.

It's still not entirely accurate, but it's much better. This is sorta a first step, not wanting to make it too huge. I still need to finish writing better tests also, and there's always more to do, but I'm quite optimistic so far.

Practical affects:

  • Can't find any games this breaks (but there is one "hack", see below.)
  • Tales of Destiny 2 -> menu
  • Unchained Blades -> ingame
  • Patapon 2 -> intro
  • Patapon -> menu (but black)
  • Final Fantasy 4 -> less errors, tiny bit farther (still nowhere)
  • Shadow of Destiny -> now loading (slightly farther)
  • Likely others?
  • Less error messages in console even for games that do work.

Changes:

  • sceIo funcs now delay/resched like on the PSP, but I kept all the delays low to be on the safe side (many were 10x or more, but often varied by a wide degree in my testing.)
  • The async functions (including sceIoOpen now) call the callback with a proper delay.
  • The async wait functions now actually wait.
  • The async result is now properly sign extended (e.g. 0xffffffff80020325 not 0x0000000080020325)
    • Unfortunately, this change caused regressions. Some games were sceIoReadAsync()'ing into a NULL pointer, but not failing because they thought it worked. Afterward, they crashed.
    • For now, I'm making it return 0, and everything seems okay with that.
    • Ultimately this is caused by some other HLE func not returning an address, etc., or else timing/scheduling issues.
  • I made hleDelayResult() support 64-bit. As a side-effect, it overwrites v1 even when returning 32-bit, but I think this is safe within the ABI?
  • It now closes files properly under async, which fixes a lot of spurious logged errors.
  • Fixes and adds some sceIoDevctl methods I found along the way (these match tests.) Some were used in the games that got farther with async.
  • Avoids overwriting the async result for non-async calls (real firmware doesn't seem to.)

There's still more - for example errors when you try to do an async twice, or wait on no async, etc. But I wanted to create this so others can see my code and report any games it breaks.

-[Unknown]

@daniel229
Copy link
Collaborator

great.this makes last ranker playable.
02

@daniel229
Copy link
Collaborator

also zero kiseki can pass this scene.
04

@dbz400
Copy link
Contributor

dbz400 commented Mar 11, 2013

It is good to hear that Last Ranker can start new game now as it previously stuck at loading :)

Just wondering any other games benefit from this amazing aync IO changes ?

@unknownbrackets
Copy link
Collaborator Author

Basically all games use sceIo and this makes some major changes to how it schedules, so the only way to answer that question is testing. So far all known games improved are logged within this issue.

-[Unknown]

@dbz400
Copy link
Contributor

dbz400 commented Mar 11, 2013

Excellent . BTW , previously mentioned will prepare to release 0.8 soon ? or 0.7 ?

@unknownbrackets
Copy link
Collaborator Author

Sorry, 0.8 was a typo - meant 0.7. I think the plan is soon, yes.

-[Unknown]

@dbz400
Copy link
Contributor

dbz400 commented Mar 11, 2013

I see

@Xele02
Copy link
Collaborator

Xele02 commented Mar 11, 2013

Black Rock Shooter now go ingame. Load thread no longer blocked :)

@@ -65,7 +65,7 @@ void hleDelayResultFinish(u64 userdata, int cycleslate)
u32 error;
SceUID threadID = (SceUID) userdata;
SceUID verify = __KernelGetWaitID(threadID, WAITTYPE_DELAY, error);
SceUID result = __KernelGetWaitValue(threadID, error);
u64 result = (userdata ^ threadID) | __KernelGetWaitValue(threadID, error);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think xoring the threadID here needs a comment, it's a bit hard to tell what's going on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, heh, I see how it looks a bit odd now, it made sense when I wrote it.

Should I just replace it with userdata & 0xFFFFFFFF00000000, rather than a comment? Should be the same deal, and that's probably a clearer way to do it.

-[Unknown]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that looks clearer. To make the compiler happy you should write it as 0xFFFFFFFF00000000ULL I think.

@hrydgard
Copy link
Owner

Setting v1 always should be ABI safe but I don't know of a way to be 100% sure of that...

Seems very unlikely to be a problem though.

@unknownbrackets
Copy link
Collaborator Author

Well, I guess I could just create two timers one for each, that would be the simplest way to not overwrite v1 if we're worried.

-[Unknown]

@hrydgard
Copy link
Owner

I'm not sure we are worried though :)
Up to you.

Now it actually fires the callback as necessary.  Some games care.
Errors become fffffff80010002, etc.
And remove defAction, it was the wrong way.
< 0 means error, and 0x0000000080000000 is not < 0.
Tried to estimate some rough timing.  Fixes Unchained Blades.
No need to have this logic duplicated.
Also make it so we can return u64s easily in places...
A low estimate based on tests.
With help from jpcsp and testing.
Final Fantasy 4 seemed to want this.
Actual delays for these seem much higher, but not sure.
@mikusp
Copy link
Contributor

mikusp commented Mar 11, 2013

GTA: LCS goes a little bit further. Patapon 2 & 3 also, but they require adding a stub for scePsmfSpecifyStream.
Zrzut ekranu z 2013-03-11 16:37:33

@livisor
Copy link
Collaborator

livisor commented Mar 11, 2013

How can i test an issue that wasn't yet merged in a build?Anyway silent hill shaterred memories will also benefit from this because it shows sceio async unimpl
http://pastebin.com/wp6FWJ9x

@hrydgard
Copy link
Owner

Okay, 0.7 was released.

Ready to merge or want to add something more?

@unknownbrackets
Copy link
Collaborator Author

Yeah, let's merge.

-[Unknown]

hrydgard added a commit that referenced this pull request Mar 11, 2013
Handle async IO more correctly (testing/review)
@hrydgard hrydgard merged commit 3efb748 into hrydgard:master Mar 11, 2013
unknownbrackets added a commit to unknownbrackets/ppsspp that referenced this pull request Aug 27, 2016
This doesn't seem to reschedule, must've measured wrong in hrydgard#939.

Fixes hrydgard#8749.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants