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

PPU Page Faults #3309

Merged
merged 7 commits into from Oct 8, 2017
Merged

PPU Page Faults #3309

merged 7 commits into from Oct 8, 2017

Conversation

@flash-fire
Copy link
Contributor

@flash-fire flash-fire commented Aug 24, 2017

I feel like Neko is going to have a field day reviewing this... or rewriting it

@mention-bot
Copy link

@mention-bot mention-bot commented Aug 24, 2017

@flash-fire, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Nekotekina, @robxu9 and @jbeich to be potential reviewers.

@flash-fire flash-fire force-pushed the flash-fire:PageFaultImplement branch from 06769c4 to 7eb236e Aug 24, 2017
{
be_t<u64> gpr[32];
be_t<u32> cr;
be_t<u32> rsv1;

This comment has been minimized.

@danilaml

danilaml Aug 24, 2017
Contributor

why use 2 spaces?

auto pf_events = fxm::get_always<page_fault_event_entries>();

bool found = false;
for (auto ev : pf_events->events)

This comment has been minimized.

@danilaml

danilaml Aug 24, 2017
Contributor

const auto &?

This comment has been minimized.

@flash-fire

flash-fire Aug 24, 2017
Author Contributor

Yeah. We don't need a local copy so const auto & would be better. I'll do same in mmapper.

Copy link
Contributor

@danilaml danilaml left a comment

btw, have you considered using some other collection other than std list? Maybe something like unordered (multi)map or just forward_list. Also, a lot of the loops could probably be replaced by std:find_if.

if (cpu)
{
if (fxm::check<page_fault_notification_entries>())
{
for (auto entry : fxm::get<page_fault_notification_entries>()->entries)

This comment has been minimized.

@danilaml

danilaml Aug 24, 2017
Contributor

const auto &

auto pf_entries = fxm::get_always<page_fault_notification_entries>();

// We're not allowed to have the same queue registered more than once for page faults.
for (auto entry : pf_entries->entries)

This comment has been minimized.

@danilaml

danilaml Aug 24, 2017
Contributor

const auto &

null_func,//BIND_FUNC(sys_ppu_thread_recover_page_fault)//57 (0x039)
null_func,//BIND_FUNC(sys_ppu_thread_get_page_fault_context),//58 (0x03A)
BIND_FUNC(sys_ppu_thread_recover_page_fault), //57 (0x039)
BIND_FUNC(sys_ppu_thread_get_page_fault_context), //58 (0x03A)

This comment has been minimized.

@danilaml

danilaml Aug 24, 2017
Contributor

tabs before //

@flash-fire
Copy link
Contributor Author

@flash-fire flash-fire commented Aug 24, 2017

Map/set wouldn't add much efficiency since page_fault_notification_entries is indexed by event_queue_id in sys_mmapper_enable_page_fault_notification and by address elsewhere. So, I don't think there's an O(1) solution everywhere without multiple maps. Forward_lists would probably be better for how I'm using it. But, again, so far all games tested only page fault from one thread and have only one thing being notified-- there isn't much point to optimizing data structures while n=1.

I didn't know about std::find_if, but it isn't being used anywhere else in rpcs3's codebase. For readability, would it be worth starting to use it if it's used no where else?

@flash-fire flash-fire force-pushed the flash-fire:PageFaultImplement branch from 2b1b23b to 2d78207 Aug 24, 2017
@flash-fire flash-fire force-pushed the flash-fire:PageFaultImplement branch 2 times, most recently from d04f35e to 718b46b Sep 1, 2017
@AniLeo AniLeo requested a review from Nekotekina Sep 6, 2017
@flash-fire flash-fire changed the title Implement page faults for PPUThreads [WIP] Implement page faults for PPUThreads Sep 8, 2017
@flash-fire
Copy link
Contributor Author

@flash-fire flash-fire commented Sep 8, 2017

BACK to WIP. I discovered something particularly nefarious that will require a rewrite.

@flash-fire flash-fire changed the title [WIP] Implement page faults for PPUThreads [WIP!] [Needs Testing] Implement Page Faults and Make VM Compliant to Not Zeroing Sep 11, 2017
@flash-fire
Copy link
Contributor Author

@flash-fire flash-fire commented Sep 11, 2017

This is very much a work in progress. As, Linux isn't done (is mostly done ;) and it doesn't improve unity games as much as my hacked build. However, this should start seeing improvement for basically all unity games!

@Nekotekina
Copy link
Member

@Nekotekina Nekotekina commented Sep 12, 2017

This derailed really hard.

@flash-fire
Copy link
Contributor Author

@flash-fire flash-fire commented Sep 12, 2017

Cleaned up everything. Less multitrack drifting.

@flash-fire flash-fire force-pushed the flash-fire:PageFaultImplement branch from cc6b152 to f3ebad5 Sep 12, 2017
@flash-fire flash-fire force-pushed the flash-fire:PageFaultImplement branch from f3ebad5 to 573111a Sep 12, 2017
@flash-fire flash-fire changed the title [WIP!] [Needs Testing] Implement Page Faults and Make VM Compliant to Not Zeroing PPU Page Faults Sep 12, 2017
@woj1993
Copy link

@woj1993 woj1993 commented Oct 6, 2017

@flash-fire Is this ready?

@AniLeo
Copy link
Member

@AniLeo AniLeo commented Oct 6, 2017

No, otherwise it would have been merged already

@AniLeo
Copy link
Member

@AniLeo AniLeo commented Oct 6, 2017

Really, copy pasta gigantic console output?

I removed the spam, if you want to include a log file, attach an actual log file.

flash-fire and others added 2 commits Oct 6, 2017
@Nekotekina Nekotekina merged commit 47b121a into RPCS3:master Oct 8, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@flash-fire flash-fire deleted the flash-fire:PageFaultImplement branch Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants