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 zcull time to not time travel to the future #8048

Merged
merged 1 commit into from Apr 28, 2020

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented Apr 16, 2020

  • When timestamp_subvalue was equal to 1000 the reported time is the next microsecond, which means in comparison to other time reporting features with microsecond accuracy such current flip or vblank time (they use get_system_time()), rsx::thread::timestamp() has reported time ahead of the time flip/vblank reported.
    Even comparisons of two or more values from rsx::thread::timestamp() could have indicated such "time traveling". e.g if the first call had x value reported from get_system_time() and timestamp sub value that was greater than 1000, so if in the next call to rsx::thread::timestamp() the value reported by get_system_time() is greater than x but less than x + subvalue then this is illegal.
    To fix this, forcefully wait in rsx::thread::timestamp() until the next time value is reported.

  • Use the method with highest accuracy ps3 has to get the current time which is get_timebased_time(), it is used by PPU mftb and SPU decrementer read. It's fequancy is about 80 times higher than get_system_time().
    This ensures there won't be "time traveling" in comparison to PPU/SPU time as well.

@kd-11
Copy link
Contributor

kd-11 commented Apr 16, 2020

Is there no way to return real time in nanoseconds? I know clock_gettime returns time with ns precision. These workarounds we keep adding are starting to become a burden. My original reason to use subvalue was a quick hack and building off of that was a bad idea in the first place.

@elad335
Copy link
Contributor Author

elad335 commented Apr 16, 2020

  • That timestamp_subvalue ensures that everytime the function is called, it returns a different and unique value which what we want.
  • Even on linux, clock_gettime returns a value which is measured in nanoseconds but the real resolution is not guarenteed to be 1ns anywhere irrc. The actual resolution can be retrieved from clock_getres.
  • The only way to actually get ~1ns resolution is to use RDTSC, but unless we require all processors to support invarient TSC while also have TSX frequancy of 1ghz+ there's not much we can do.

@kd-11
Copy link
Contributor

kd-11 commented Apr 16, 2020

I am aware of the limitations of the subvalue, I'm the one who introduced it. We may not need to get 1ns precision for rdtsc to work, it should just make it so that we do not rely on the subvalue too much. Incrementing at a fixed rate was a lazy solution that I did in a few seconds with hopes of fixing it later.
As for rdtsc, afaik there has been some advancements in that front, could be worth considering as a second path instead of this workaround. We can also base all RSX clocks around rsx timestamp. Note that RSX timestamp != Cell time, so it is fine for RSX clock to drift as long as it is all RSX time.

@Nekotekina
Copy link
Member

Invariant TSC doesn't have nanosecond resolution in reality - its "remainder part" is approximated by the CPU after multiplying the value from the stable tick source, which is somewhere 10-100 MHz (so-called "crystal frequency", possibly located on the motherboard). It's multiplied to approach the CPU base clock (for historical reasons), and only looks like it has high resolution, because there is no way to verify its accuracy anyway (unless you have an atomic clock lol).

@elad335
Copy link
Contributor Author

elad335 commented Apr 17, 2020

It may be true that there might a better fix for this in theory, but right now the bug needs fixing.

@kd-11
Copy link
Contributor

kd-11 commented Apr 20, 2020

This isn't an urgent fix, we can be more careful with implementation instead of spending time redoing it full later on. This is actually ok-ish, but now there is a wait which is not accounted for which is what bothers me the most. get_timebased_time() is also not cheap, this will cause slowdown if the condition is hit multiple times, which is why I was suggesting trying to use TSC if possible.
That being said, it shouldn't be too common to hit this condition in normal runtime, so it should be fine.

@AniLeo AniLeo requested a review from kd-11 April 25, 2020 14:09
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.

I'm not satisfied with this solution, but doing this correctly is going to be a lot more work. As such, this can be merged as is, I'll implement it correctly later.

@Nekotekina Nekotekina merged commit 833ace1 into RPCS3:master Apr 28, 2020
@elad335 elad335 deleted the rsx-time branch June 13, 2020 05:25
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

3 participants