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: Rewrite IO mappings (again) #7262

Merged
merged 3 commits into from Feb 10, 2020
Merged

rsx: Rewrite IO mappings (again) #7262

merged 3 commits into from Feb 10, 2020

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented Jan 16, 2020

  • Move io table to rsx::thread, no longer a global variable.
  • Minor optimizations in io translations: remove shift left of base mapping 1m block, obsolete validation check and a compiler hint about it.
  • Fix HLE cellGcmGetTimeStampLocation (io offset passed to translation was wrong, see cellGcmGetReportDataAddressLocation).
  • Make HLE gcm internal translations rely on HLE table.
  • Minor bugfix of rsx::thread::pause(). (wait for previous pause request)
  • Fix rsx capture re-capturing frame after a crash occured or emulation was stopped in the middle of it.
  • Make cellGcm HLE thread-safe.

@elad335 elad335 requested a review from kd-11 January 16, 2020 21:05
@Megamouse
Copy link
Contributor

I thought rsx was frozen XD

@Margen67
Copy link
Contributor

@Megamouse Just let it go :^)

@elad335 elad335 force-pushed the rsx-verify branch 2 times, most recently from 7b9ff4f to 2c972ea Compare January 16, 2020 21:44
@Xcedf
Copy link

Xcedf commented Jan 16, 2020

Uncharted 2, right before intro fmv
E {RSX [0x0010008]} RSX: FIFO error: possible desync event (last cmd = 0xffffffff)
F {RSX [0x006d060]} RSX: class std::runtime_error thrown: GetAddress(offset=0xffffff, location=0xfeed0001): RSXIO memory not mapped
(in file C:\projects\rpcs3\rpcs3\Emu\RSX\RSXThread.cpp:73)

Uncharted 3
E {RSX [0x0012008]} RSX: FIFO error: possible desync event (last cmd = 0xffffffff) x2
E {RSX [0x006e454]} RSX: FIFO error: possible desync event (last cmd = 0xffffffff) x2
F {PPU[0x1000000] Thread (main_thread)} class std::runtime_error thrown: Trap! (0x731b44)

@kd-11
Copy link
Contributor

kd-11 commented Jan 17, 2020

Rsx is still in maintenance mode so this will be up for a while.

@elad335
Copy link
Contributor Author

elad335 commented Jan 17, 2020

Possibly fixed.

@Megamouse
Copy link
Contributor

We can merge the error code earlier if you seperate it

@Xcedf
Copy link

Xcedf commented Jan 17, 2020

@elad335 yes, fixed

rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
@elad335 elad335 force-pushed the rsx-verify branch 2 times, most recently from d5763c8 to 5048286 Compare January 17, 2020 11:55
@elad335 elad335 requested a review from kd-11 January 17, 2020 11:57
@elad335 elad335 force-pushed the rsx-verify branch 2 times, most recently from 1ff51d3 to 6c33aeb Compare January 28, 2020 13:23
@elad335
Copy link
Contributor Author

elad335 commented Jan 28, 2020

hmm can it be merged already? Honestly if it may regress anything it would be fixed at lightspeed once it reaches master due to its triviality.

rpcs3/Emu/RSX/RSXFIFO.cpp Outdated Show resolved Hide resolved
@elad335 elad335 closed this Feb 7, 2020
@elad335
Copy link
Contributor Author

elad335 commented Feb 7, 2020

oops

@elad335 elad335 reopened this Feb 7, 2020
@rajkosto
Copy link
Contributor

rajkosto commented Feb 8, 2020

well LIKELY and UNLIKELY arent macros anymore so it would have to be transformed into [[unlikely] on the side, thats why i asked

@elad335
Copy link
Contributor Author

elad335 commented Feb 8, 2020

Conflicts solved.

@elad335 elad335 force-pushed the rsx-verify branch 2 times, most recently from 750dbef to 2fe4911 Compare February 8, 2020 19:33
@elad335 elad335 requested review from kd-11 and removed request for kd-11 February 8, 2020 19:46
@@ -68,9 +67,9 @@ namespace rsx
case CELL_GCM_CONTEXT_DMA_MEMORY_HOST_BUFFER:
case CELL_GCM_LOCATION_MAIN:
{
if (u32 result = RSXIOMem.RealAddr(offset))
if (const u32 ea = render->iomap_table.get_addr(offset); ea + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just check ea == 0xffffffff? It's more readable that way. Also, the decision to have ea = 0 as legal is dubious, I do not think that is legal for anything but the lv1 kernel itself. I don't mind either way but this add is pointless here for the comparison bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 isn't compatoble with the opposite operation (ea->io), but io->ea can still use 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also -1 has a cool trick, notice rsx_io_table::get_addr no longer checks for unmapped memory.
Thats because OR doesnt change -1, unlike 0.

Copy link
Contributor

@kd-11 kd-11 Feb 9, 2020

Choose a reason for hiding this comment

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

That's fine, but the comparison for invalid still irks me. It relies on arithmetic to infer a hard-coded constant which is messy. Can this be done in the table class with proper named constants? I think a static constexpr in the map class may work for this. The reason 0 comparison gets a pass is because it is a universal 'false' since forever, hence the special handling even in the language itself.
Alternatively get_addr may return a pair, but that is overkill.

@AniLeo AniLeo requested a review from kd-11 February 10, 2020 18:53
@AniLeo AniLeo merged commit dcb30df into RPCS3:master Feb 10, 2020
@elad335 elad335 deleted the rsx-verify branch February 10, 2020 21:40
@elad335 elad335 changed the title rsx: Rewrite io mappings (again) rsx: Rewrite IO mappings (again) Aug 16, 2021
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.

None yet

8 participants