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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix section reuse #5013

Merged
merged 1 commit into from Aug 22, 2018

Conversation

Projects
None yet
8 participants
@kd-11
Contributor

kd-11 commented Aug 20, 2018

Probable fix for #4298

  • Returns things to the old way of handling false positives in the texture cache.

Thanks to @ruipin for debugging the issue. Took us a whole week to find my silly mistakes 馃槤. Hopefully this fixes flickering in other games like #4857

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch 2 times, most recently from 5638f9f to fe3100d Aug 20, 2018

@kd-11 kd-11 referenced this pull request Aug 20, 2018

Open

Demon's Souls with v7077 #4857

@ruipin

This comment has been minimized.

Contributor

ruipin commented Aug 20, 2018

I'm curious if this actually fixes more games than P5!

Mistakes happen, don't worry. Thanks for all your help throughout this debugging effort - I'm starting to think it's a waste that most of our conversation happened in DMs, lots of useful knowledge there.

Once this gets merged I'll see about pushing some of the sanity checks/assertions I wrote to debug this, so we can avoid more texture cache issues in the future.

@Dani1977

This comment has been minimized.

Dani1977 commented Aug 20, 2018

Thanks,
I try this PR, white flickering are gone , but black flickering (black screen) still present this are much shorter and more rare than before .

I try Nexus and Boletaria 1-1, ask me if you need more information and tests .

Regards

RPCS3.log.gz

immagine

immagine

others setting are on default value .

I try to do more levels

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from fe3100d to 8533c3a Aug 20, 2018

@kd-11

This comment has been minimized.

Contributor

kd-11 commented Aug 20, 2018

You can try with async disabled just in case there are issues resulting from that. Its unlikely though, likely some other unrelated bug that will need separate investigation.

@Dani1977

This comment has been minimized.

Dani1977 commented Aug 20, 2018

immagine

immagine

RPCS3.log.gz

with disable async shaders (on already visited level so no caching shaders) looks like the same with enable; but I notice this thing (enable/disable async is the same):
when you dodge on a barrel or broke some environments, the screen flick (Boletaria).

Hope that can help you
Regards

@fifazalata

This comment has been minimized.

fifazalata commented Aug 20, 2018

Could this be related to WCB?
Since the flickering was happening with NieR too and it needs WCB.

@kd-11

This comment has been minimized.

Contributor

kd-11 commented Aug 20, 2018

Nier bug predates the memory mirror patches. Likely not related.

@Parthorisian

This comment has been minimized.

Parthorisian commented Aug 21, 2018

Non TSX CPU. Shaders cache has been cleared. No any flickering noticed+large performance boost(ASMJIT Recompiler) similar to pr4935.
Settings: standard except WCB+Use GPUTexture scaling - are on. Played around 1-1,5 hours (almost 600 pipeline objects compiled)
P.S. a)May be something like that sometimes for Nexus+World4-2, but hard to notice and i think it was just popups of uncompiled shaders. Could'nt notice anything on 2nd boot
b)I hope infinite loading screen (for ASMJIT Recompiler) which finished my tests, unrelated to this PR
DemonsSouls_RPCS3.zip

@kamer1337

This comment has been minimized.

kamer1337 commented Aug 21, 2018

Tony Hawk's Project 8

F {PPU[0x1000000] Thread (main_thread) [0x009f1edc]} class std::runtime_error thrown: Unreachable
(in file c:\projects\rpcs3\rpcs3\emu\rsx\vk../Common/texture_cache.h:614)

RPCS3.log.gz

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from 5c30cbd to 4c9fcc7 Aug 21, 2018

@kd-11

This comment has been minimized.

Contributor

kd-11 commented Aug 21, 2018

Ignore the assertions for now, I'll update when its ready for testing again. Currently restructuring some internals so things are still buggy.

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch 2 times, most recently from f82561a to c98e2bd Aug 21, 2018

@kd-11

This comment has been minimized.

Contributor

kd-11 commented Aug 21, 2018

Should be safe to test again.

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from c98e2bd to b5d6418 Aug 21, 2018

@dukenukemx

This comment has been minimized.

dukenukemx commented Aug 21, 2018

Been using #5013 and it still freezes but it will unfreeze if I give it some time. It has done this twice. I still get the flicker once in a while too. In both cases the RPCS3 main window is not accessible. It isn't usable cause once this happens it freezes again in seconds.

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch 2 times, most recently from 6430c23 to 6f96f8b Aug 21, 2018

std::vector<std::pair<u32, u32>> subtractive_intersect(std::vector<section_storage_type*> marked_sections, std::vector<section_storage_type*> sections_to_exclude)
{
std::vector<std::pair<u32, u32>> result;
bool intersection_exists = false;

This comment has been minimized.

@ruipin

ruipin Aug 21, 2018

Contributor

Unused variable

// At least one intersection exists
intersection_exists = true;
// Split the range if possible

This comment has been minimized.

@ruipin

ruipin Aug 21, 2018

Contributor

Is this comment in the right place? Only the else branch actually "splits the range"

// Cannot be salvaged
this_range = { 0, 0 };
}
else if (memory_end > range_start && memory_end < range_end)

This comment has been minimized.

@ruipin

ruipin Aug 21, 2018

Contributor

This condition makes no sense. We want to check if {memory_base,memory_end} overlaps range_start, so why does range_end come into play?
Should probably be range_start >= memory_start && memory_end > range_start.
(Suggestion: Refactor these checks into a constexpr address_overlaps to avoid these sort of errors in the future and improve readability)

This comment has been minimized.

@kd-11

kd-11 Aug 22, 2018

Contributor

its inverse of your test, its checking if memory_end overlaps {range_start, range_end}. Will refactor for readability.

@ruipin

This comment has been minimized.

Contributor

ruipin commented Aug 22, 2018

I have tested this commit (or to be exact ruipin@4e44fbd which has a bug fix applied and a ton of debug checks and assertions) on both P5 and DeS for about 1 hour each, and could not reproduce any issues (other than the random black flickering in DeS which predates this PR and is probably a separate issue). I want to test it in Uncharted or something but sadly won't have much time to do so for the rest of the week.

Just make sure to fix the bug I reported in one of the review comments and I think this commit is ready to be merged.

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from 6f96f8b to 2cf3c4b Aug 22, 2018

std::vector<section_storage_type*> sections_to_unprotect; //These sections are to be unpotected and discarded by caller
std::vector<section_storage_type*> sections_to_flush; // Sections to be flushed
std::vector<section_storage_type*> sections_to_unprotect; // These sections are to be unpotected and discarded by caller
std::vector<section_storage_type*> sections_to_exclude; // These sections are to be repotected and discarded by caller (false positives)

This comment has been minimized.

@Margen67

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch 2 times, most recently from a4623dc to f9ef6db Aug 22, 2018

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from f9ef6db to a3f5d7b Aug 22, 2018

@kd-11 kd-11 changed the title from [WIP] Fix section reuse to Fix section reuse Aug 22, 2018

@ruipin

Two small comments, but everything looks good to me. I'll try to find time later today to test this and let you know. Though I think it could be merged without any issues at this point.

for (const auto &dirty : sections_to_exclude)
{
const auto intersect_test = dirty->get_protected_range();

This comment has been minimized.

@ruipin

ruipin Aug 22, 2018

Contributor

intersect_test should probably be renamed to exclude_range or something of the sort, this name is not very obvious and looks to me like it was copy-pasted from a method below that uses the same variable name.
See above for dirty.

result.push_back(section->get_protected_range());
}
for (const auto &dirty : sections_to_exclude)

This comment has been minimized.

@ruipin

ruipin Aug 22, 2018

Contributor

dirty? These sections aren't dirty - actually the opposite, right? (otherwise why wouldn't we unprotect them?)
Maybe use exclude or exclusion or something similar.

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from a3f5d7b to b740a81 Aug 22, 2018

@kd-11 kd-11 force-pushed the kd-11:rsx_volatile branch from b740a81 to b5f2e9b Aug 22, 2018

@kd-11 kd-11 merged commit f3d3a1a into RPCS3:master Aug 22, 2018

2 checks passed

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