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

rsx: Fix index vertex array range with modulo calculation #14505

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented Aug 18, 2023

In #9055, modulo op with frequency was used on vertex indices wiuth some values already surpoassing frequency divider value. We incorrectly assumed that this means that the upper bound of vertex data memory data is the frequency divider in this case, but actually we still need to check for each element max_index = max(index % frequency, max_index). This is also true for min_index.
This re-iteration over all indices can be quite expensive for this exceptionally rare case, so do it if an access violation is guarenteed due to unallocated memory access. I think this can be optimized in the future for single frequency across the entire vertex arrays but I won't do it in this pr.

Fixes #9055

rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Show resolved Hide resolved
{
if (std::find(std::begin(frequencies), std::begin(frequencies) + freq_count, attrib.frequency) == std::begin(frequencies) + freq_count)
{
frequencies[freq_count++] = attrib.frequency;
Copy link
Contributor

Choose a reason for hiding this comment

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

how is ensured that freq_count is not >= 16 ?

rpcs3/Emu/RSX/RSXThread.cpp Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
@elad335 elad335 force-pushed the modulo branch 2 times, most recently from 027f866 to d2cb038 Compare August 18, 2023 18:15

if (index_size == 4)
{
re_evaluate(u32{});
Copy link
Contributor

Choose a reason for hiding this comment

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

weird right?
I stumbled upon the same issue lol

Copy link
Contributor

@Megamouse Megamouse Aug 18, 2023

Choose a reason for hiding this comment

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

Maybe just pass vm::get_super_ptr<u32>(address) here and use auto in the lambda.
Then there would be no such useless param.

@elad335 elad335 force-pushed the modulo branch 6 times, most recently from 8ce204d to 35e10d1 Compare August 18, 2023 19:13
// Force aligned indices as realhw
const u32 address = (0 - index_size) & get_address(rsx::method_registers.index_array_address(), rsx::method_registers.index_array_location());

auto re_evaluate = [&](auto ptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surpised that this works without the template.
Are you sure it's not implicitly casting to another type for one of the calls?

Copy link
Contributor

@kd-11 kd-11 Aug 19, 2023

Choose a reason for hiding this comment

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

Recursively iterating over the IB is way too slow for this to be a real fix.
A better solution is to simply handle this whole function as a line intersection problem. Every attribute is simply a 1D vector and usually only a handful will be in use anyway. I may have to think about it a bit more, it may require some more work elsewhere.
This function exists solely to accelerate vertex uploads over going one attr at a time, which this solution is actually slower than.

@elad335
Copy link
Contributor Author

elad335 commented Aug 19, 2023

I added a debug message for when this fallback is hit named "This sucks" in red error message, please test many games and upload logs if you see it spammed frequently or even logged at all.
In the game in the issue it is about once per second and after entering the stage it nearly vanishes.

@elad335 elad335 force-pushed the modulo branch 2 times, most recently from 588b225 to 7dd90e9 Compare August 19, 2023 08:43
@Ordinary205
Copy link
Contributor

I've tested most of the games and haven't noticed a single error named "This sucks".
So it seems fine to me.

@ReLyf47
Copy link

ReLyf47 commented Aug 21, 2023

No other game except specifically the DW series spits out "This Sucks"
so it doesnt affect other game
but it makes DW games works

@elad335 elad335 merged commit a26b8df into RPCS3:master Aug 22, 2023
5 checks passed
@arcadee1977
Copy link

GT6 have broken graphic in this PR

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

Successfully merging this pull request may close these issues.

{RSX [0x02a6238]} VM: Access violation reading location 0xcf900000 (unmapped memory) (Dynasty Warriors 8 XL)
6 participants