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

Kernel/IPC: Add a small delay after each SyncRequest to prevent thread starvation. #3091

Merged
merged 2 commits into from Dec 10, 2017

Conversation

Projects
None yet
@Subv
Copy link
Member

Subv commented Nov 8, 2017

In a real 3DS, threads that call svcSyncRequest are put to sleep until the server responds via svcReplyAndReceive. Our HLE services don't implement this mechanism and are effectively immediate from the 3DS's point of view. This commit makes it so that we at least simulate the IPC delay.

Specific HLE handlers might need to put their callers to sleep for a longer period of time to simulate IO timings. This is their responsibility but doing so is currently not implemented.

See https://gist.github.com/ds84182/4a7690c5376e045cab9129ca4185bbeb for a test that was not passing before this commit.

Thanks @ds84182 for pointing this out!

Closes #2794 #1105 #3054 #2647 #2802 #2608 #2749 #2118 #2526 #2749


This change is Reviewable

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Nov 8, 2017

looks like fixes Animal Crossing #1105

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Nov 8, 2017

don't think each SyncRequest needs own delay, because games works well on differ variants of (new/old)3ds/2ds and firmwares

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Nov 8, 2017

also fixes FIFA (at least 12), but first we need to signal y2r::completion_event ASYNC

well, not really fix, can't pass extdata creation window, but this is not related bug

unknown / unimplemented function 'CreateExtSaveData': port=fs:USER

2017-11-08

@Dragios

This comment has been minimized.

Copy link
Contributor

Dragios commented Nov 8, 2017

Does it also fix the the missing models on the character / hand in ACNL?

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Nov 8, 2017

@Dragios, yes. i don't play enough yet, but at start missing chars visible
2017-11-08 1

@dampih

This comment has been minimized.

Copy link

dampih commented Nov 8, 2017

This PR also fixes #3054 .My friend said it also fixes Dragon Quest XI stuck after choosing New Game and became playable, but unfortunately it also make Attack of the Friday Monster! A Tokyo tale stuck at intro logo. Sorry I don't have those game so I can't test it out.
citra-qt 2017-11-08 16-07-06-14

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 8, 2017

IMO the default delay time should be the minimal one a SendSyncRequest can take. i.e. it is the time taken by a no-op service function. Is this measurable or calculable?

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 8, 2017

@acnleditor2 no other hack talk is allowed here, thanks.

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 8, 2017

@wwylele Agreed. We could write a custom service that has a single function that takes an input ticks value, calculates the difference with the tick value when it is executed, and returns that in the command response. Then we just call that a lot of times and average it out.

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Nov 8, 2017

just check random softlock game: Moco Moco Friends - softlock on chapter screen #2794
it works.
looks like maaaany issues will be closed

@dampih

This comment has been minimized.

Copy link

dampih commented Nov 9, 2017

Changing initial value 100 to 90000 give additional bonus fixes. Fixes #2446 #2802 #2745

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 9, 2017

That delay really has to be measured on real hardware, but I believe an IPC overhead of 0.09ms (90000ns) isn't out of the realm of possibility.

@Subv Subv force-pushed the Subv:hle_request_delat branch from 6bf5d78 to fe4e647 Nov 9, 2017

@Dragios

This comment has been minimized.

Copy link
Contributor

Dragios commented Nov 9, 2017

Fixes #3061 as well. I have set the delay to 0.1ms. Still fixed with the new value of 54.14 µs.
Pokédex 3D Pro now boots up instantly rather than stuck at black screen.


// TODO(Subv): Figure out a good value for this.
static constexpr u64 IPCDelayNanoseconds = 100;
thread->WakeAfterDelay(IPCDelayNanoseconds);

This comment has been minimized.

@Subv

Subv Nov 9, 2017

Member

I just noticed that due to the way WakeAfterDelay is implemented, we can't have delays of less than a microsecond, this is essentially the same as SleepThread(0)

This comment has been minimized.

@tokumeiwokiboushimasu

tokumeiwokiboushimasu Dec 10, 2017

Is this really 51140 instead of 54140?

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 9, 2017

I have updated the delay to 54.14 microseconds as measured by @jroweboy. Could you all please re-test everything and report here what was fixed / what was broken by this?

@bakugo

This comment has been minimized.

Copy link

bakugo commented Nov 9, 2017

Can confirm that #2802 is fixed with this value.

@valentinvanelslande

This comment has been minimized.

Copy link
Contributor

valentinvanelslande commented Nov 9, 2017

Mario Maker: Pigeon stops here
super mario maker 3ds

@Hexagon12

This comment has been minimized.

Copy link
Contributor

Hexagon12 commented Nov 9, 2017

Can confirm that #2608 is also fixed.

@RavenHome1

This comment has been minimized.

Copy link

RavenHome1 commented Nov 9, 2017

causes pokemon moon to show a black screen instead of the intro

@Chrisatsinnoh

This comment has been minimized.

Copy link

Chrisatsinnoh commented Nov 10, 2017

Where can I download the fixes, though?

@bakugo

This comment has been minimized.

Copy link

bakugo commented Nov 10, 2017

Update on #2802: The freeze that always happens when starting the game OR loading a save is indeed fixed with IPCDelayNanoseconds = 51140, however it still happens in other situations: if you create a character, let it save, then quit and start again, it will happen after the intro cutscene. Increasing the value further (to 851140) makes the game not freeze but get stuck on a white screen instead with sound playing.

Seems like this is a pretty complex issue.

@einstein95

This comment has been minimized.

Copy link
Contributor

einstein95 commented Nov 10, 2017

This fixes #2749. Star Fox 64 3D now properly initialises save data without needing blank files or premade save files.

@j-selby

This comment has been minimized.

Copy link
Contributor

j-selby commented Nov 11, 2017

Fixes #2118. Woop!

@valentinvanelslande

This comment has been minimized.

Copy link
Contributor

valentinvanelslande commented Nov 18, 2017

#3119 breaks ever oasis fix

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 18, 2017

I am going to temporarily pull this from canary and push #3119 first. We need to make sure it doesn't break anything from master. After everything settle down, we will revisit this fix

@einstein95

This comment has been minimized.

Copy link
Contributor

einstein95 commented Nov 22, 2017

@SigmaVirus No, it doesn't

@bunnei

bunnei approved these changes Dec 4, 2017

@bunnei

This comment has been minimized.

Copy link
Member

bunnei commented Dec 6, 2017

Can this be merged? (I have hesitation since I don't see any more approvals...)

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Dec 6, 2017

This is waiting on someone actually measuring the delay properly

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Dec 6, 2017

hmm... i suppose New 3ds faster, than 2ds, sorry for trolling

@B3n30

This comment has been minimized.

Copy link
Contributor

B3n30 commented Dec 7, 2017

@mailwl nope 2ds is faster.

HW tests with https://gist.github.com/ds84182/ecdbbd25b56a29bd4e5b32a7544b8e92 compared with testing in citra showed that a delay of 39ms will give the same results as the average on new3ds. Those were also values observed on 2ds. Thus a delay of 39ms is a good start. But I also would add a comment on how we obtained that value. Besides changing the delay: LGTM

Kernel/IPC: Add a small delay after each SyncRequest to prevent threa…
…d starvation.

In a real 3DS, threads that call svcSyncRequest are put to sleep until the server responds via svcReplyAndReceive. Our HLE services don't implement this mechanism and are effectively immediate from the 3DS's point of view. This commit makes it so that we at least simulate the IPC delay.

Specific HLE handlers might need to put their callers to sleep for a longer period of time to simulate IO timings. This is their responsibility but doing so is currently not implemented.

See https://gist.github.com/ds84182/4a7690c5376e045cab9129ca4185bbeb for a test that was not passing before this commit.

@Subv Subv force-pushed the Subv:hle_request_delat branch from df7808c to 00ea8c2 Dec 8, 2017

Kernel/IPC: Use 39 microseconds for the SendSyncRequest delay approxi…
…mation.

As measured by the time it takes for to return when performing the SetLcdForceBlack IPC request to the GSP:GPU service in a n3DS with firmware 11.6

See https://gist.github.com/ds84182/ecdbbd25b56a29bd4e5b32a7544b8e92 for the source code of the test.

@Subv Subv force-pushed the Subv:hle_request_delat branch from 00ea8c2 to 98e3872 Dec 10, 2017

@B3n30 B3n30 merged commit 2146311 into citra-emu:master Dec 10, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment