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

pbkit: Set default PTIMER values without killing the system #621

Closed
wants to merge 1 commit into from

Conversation

GXTX
Copy link
Contributor

@GXTX GXTX commented Jan 5, 2023

2nd go at #585

Fixes #536

Edit: Playing around with this, it seems like we can remove it. The kernel and applications before should already be properly setting up these 2 regs and they aren't reset during a launch. Thoughts on removing it completely? I tried testing with abaire/nxdk_pgraph_tests but their nxdk is out of date and refuses to build on my system.

The kernel sets these registers up before ever launching an application, so we don't need to be calculating them. Verified this on 3944 and some scene kernels.

@GXTX GXTX changed the title pbkit: Support non-standard frequencies pbkit: Remove PTIMER calcuations Jan 6, 2023
@JayFoxRox
Copy link
Member

Rather than checking if the kernel does it, I think we should be doing what Direct3D is doing.
Yes, the kernel also initializes a bunch of other hardware, but the state isn't always ideal.

The GPU setup in the boot animation might be incomplete and there might even be scenarios where a dashboard messes up these registers. If D3D does it, then that dashboard would work for almost any XDK app, but nxdk apps would suddenly break.

@GXTX
Copy link
Contributor Author

GXTX commented Jan 7, 2023

Turns out the XDK just hard codes these values. Taken from LEGO Star Wars 1. So.. change to these values instead?

image

@JayFoxRox
Copy link
Member

That's essentially the code we already have, with a slight off-by-one.

I'd be fine with either:
A. keeping as-is.
B. modifying to XDK variant.
C. making clock adjustable in a high-level fashion (like #585) but ideally defaulting to the current values. This might also require code on cleanup to default to more standard-ish values if not done from a stay-resident app?).

Removing (this PR) is a bad idea in my opinion; there are probably reasons why the XDK code does it.
Behaving more similar to XDK reduces risk for errors which are hard to debug and avoids potentially dangerous hardware state.

However, I think it's a good idea to extend behaviour in nxdk for advanced / experimental users.
There migh be good arguments for skipping parts of init or modifying it (overclock/underclock) in some case, but the default option should drive the hardware similar to the XDK (in my opinion).

@GXTX
Copy link
Contributor Author

GXTX commented Jan 7, 2023

I don't really care how this turns out. I would like to stop using the dashboard I'm using and use a nxdk based one, this requires not killing the application when NVCLK is overclocked. So you or other org members need to decide the implementation. You suggested we do it the way XDK does it so I found that it's just static, can we remove this check and be done with it?....

@GXTX GXTX changed the title pbkit: Remove PTIMER calcuations pbkit: Set default PTIMER values without killing the system Jan 11, 2023
@GXTX
Copy link
Contributor Author

GXTX commented Jan 11, 2023

Went ahead and changed this to have the default values without killing the box and opened a 2nd PR as the alternative (setting it the way the kernel is). Please decide. Thanks.

@LoveMHz
Copy link
Contributor

LoveMHz commented May 6, 2023

@LoveMHz
Copy link
Contributor

LoveMHz commented May 6, 2023

Turns out the XDK just hard codes these values. Taken from LEGO Star Wars 1. So.. change to these values instead?

image

The sample provided here does not match the proposed implementation. In the sample NV_PTIMER_NUMERATOR is being set to 56966, whereas the current/proposed implementation differs.

Context: It's been suggested that an acceptable solution should follow existing retail software logic (XDK).

@LoveMHz
Copy link
Contributor

LoveMHz commented May 6, 2023

Removing (this PR) is a bad idea in my opinion; there are probably reasons why the XDK code does it.

I'm not sure I follow this comment (between the multiple PRs, force pushes, etc; it's all kinda hard to follow).

Are you referring to removing the PLL check as bad? I understand that other logic may implicitly be affected by the PLL value via assumption, but I'm not seeing it in the sample that I'm currently looking at LEGO Star Wars - The Video Game (USA).

With LEGO Star Wars - The Video Game (USA) the values of NV_PRAMDAC_MPLL_COEFF, NV_PRAMDAC_VPLL_COEFF, NV_PRAMDAC_NVPLL_COEFF are being read in at the function located at 0x0018441D, and stored in CMiniport, but I'm not seeing any readback references to those values.

@GXTX GXTX closed this May 6, 2023
@JayFoxRox
Copy link
Member

Removing (this PR) is a bad idea in my opinion; there are probably reasons why the XDK code does it.

I'm not sure I follow this comment (between the multiple PRs, force pushes, etc; it's all kinda hard to follow).

My comment was about the state at the time of commenting: 5f4c262.
That revision removed the core functionality of the XDK counterpart: initialization of the GPU registers (NV_PTIMER_NUMERATOR, NV_PTIMER_DENOMINATOR and NV_PTIMER_ALARM_0)

Are you referring to removing the PLL check as bad?

I don't have a strong opinion about the PLL check.

  • If the PLL is misconfigured, an assumption is broken, so it's fine if the code fails / checks it.
  • I'm also fine if PLL is not checked if it's intended to be overclocked.

Eitherway, the default code path should be safe and stable (= mimic XDK; in this case I'm not sure what XDK does).

I have a stronger opinion about setting GPU registers on startup.
It's probably part of the hardware init and skipping it might be unsafe (= mimic XDK; set registers by default).

I'm fine with configuring other values for the clocks or skipping certain parts (checks or even register writes) - but if it happens it should happen through its own API or compile-time options, so people don't use it accidentally (which might break hardware / software).
The default options should mimic the XDK closely, so we have a stable reference to work against (and avoid bricking hardware / causing hard-to-debug bugs).

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

Successfully merging this pull request may close these issues.

pbkit quits if the nvclk freq is not default
3 participants