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: Implement atomic vertex upload (with Strict Rendering Mode) #12591

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented Sep 1, 2022

Implement RSX 128-byte atomic vertex fetching when "Strict Rendering Mode" is used. May fix 3D model or color flickering in some cases. It makes sense performance-wise for games to be able to update vertex coordinates at run-time of RSX if the developer actually knew it would fetch it atomically and the scene on screen is changing.

@kd-11
Copy link
Contributor

kd-11 commented Sep 1, 2022

Given that more than half of rendering cost is already wasted by upload, I am planning on ripping this out completely down the line. It is the main reason some games are still not playable due to performance. And I can assure you the performance hit is much greater than you think.
It is also not necessary. Games cannot know if we're still using vertex streams. The plan is to use buffer pointers on the gpu and allow passthrough access to system ram though I need a fallback plan on linux where that is not allowed for rpcs3.

@elad335
Copy link
Contributor Author

elad335 commented Sep 1, 2022

There is no performance hit if games aren't writing to vertex streams as RSX processes them or if the setting is unused, and games don't need to know if RSX uses it or not. That's the whole point as to why games would use it: The vertices of draw calls which repeat over more than 2 frames but need some adjustments as the scene changes can be updated asyncronously, it doesn't matter if the data is not reflected in the current frame but in the following one (or even not at all because a second update kicks in) because the performance gain of not stalling the RSX nor adding more 'adjusting' calculations in RSX VP shader code may be more important because RSX is slower than SPUs for this. This technique would currently result in flickering or other graphical bugs in RPCS3.

@kd-11
Copy link
Contributor

kd-11 commented Sep 1, 2022

We don't need more options that will go unused, it makes maintenance and extending functionality a pain in the ass. This is a theoretical fix with performance implications (yes, even if the option is false there will be a needless hit) with no evidence that it is needed. Sure, you could write a specially crafted test to break it, but do we really need this option? It is a case of effort being prioritized inefficiently imo. I won't block it from being merged, but having a ton of switches and tinker options doesn't help the end user beyond a certain point.

@elad335
Copy link
Contributor Author

elad335 commented Sep 1, 2022

Here are all the additional CPU instructions added by this pull request when the setting is off, I don't see anything here which results in a performance loss.
image
image

@kd-11
Copy link
Contributor

kd-11 commented Sep 1, 2022

I'm not going to argue about instruction count. 1 extra instruction is too many (remember when adding atomic fifo as an option degraded RDR performance)
My issue is the addition of new options all over the place that we now have to maintain. The emulator already has poor accessibility. Usually we would have you come in and say, we will have a <1% hit but it fixes XYZ games and the option would ideally be made the default. There should be no need for end users to know of all these different options.

I would prefer if this option is merged in with strict mode option rather than having its own setting. We have way too many of these single change options littered all over. Look at it from my pov - what do I do with this when I rewrite this functionality? I have to weigh if it is worth porting it over to the new mechanism which takes extra time and effort. Adding new behavioral changes should not be taken so lightly.

@elad335 elad335 changed the title rsx: Implement Atomic Vertex Upload (via setting) rsx: Implement atomic vertex upload (with Striuct Rendering Mode) Sep 1, 2022
@elad335 elad335 changed the title rsx: Implement atomic vertex upload (with Striuct Rendering Mode) rsx: Implement atomic vertex upload (with Strict Rendering Mode) Sep 1, 2022
@elad335
Copy link
Contributor Author

elad335 commented Sep 1, 2022

Merged with SRM.

@elad335 elad335 force-pushed the atomic-rsx-vertex-upload branch 2 times, most recently from 5c443e3 to 181b0df Compare September 1, 2022 14:58
Copy link
Contributor

@kd-11 kd-11 left a comment

Choose a reason for hiding this comment

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

lgtm

ch_out_mbox.set_value(data);
ch_in_mbox.try_pop(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

This to me looks like it needs some testing. Any ideas on what games could be affected by this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EAGAIN is a fake error code for savestates here, it means the syscall ia aborted because it cannot be completed because the thread-to-be-signalled has aborted execution in savestates.

@Nekotekina Nekotekina merged commit 7bad8f3 into RPCS3:master Sep 1, 2022
@elad335 elad335 deleted the atomic-rsx-vertex-upload branch September 2, 2022 00:43
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.

3 participants