Repository navigation
Conversation
Every gpu_metrics v2.x table carries GFX and MM activity in hundredths of a percent. The SMU headers mark GfxActivity and UvdActivity "[centi]" for each ASIC that publishes one -- smu_v13_0_4, smu_v13_0_5 and yellow_carp for v2.1, renoir and vangogh for v2.2 to v2.4 -- and amdgpu stores the value unscaled, dividing by 100 only on the gpu_busy_percent path. amd-smi copied it straight into the public whole-percent fields, so amdsmi_get_gpu_activity() and amd-smi metric --usage reported a GPU at 45% busy as 4567. Convert on the way to the public fields, leaving the not-applicable sentinel alone so a value the device never answered for does not become 655. The APU sub-struct keeps the units the firmware published, as its power fields already do. A v3.0 table reports whole percent to begin with and is untouched. Two further fixes to the same path: - A per-metric query turned an unreadable metrics revision into unexpected data, contradicting the unsupported status the whole-table query reports for the same device. Keep the reason setup gave. - readDevInfoBinary() passed every errno from a refused read through to the caller, where EINVAL became INVALID_ARGS and blamed the caller for a fault on the driver's side. Pass on only the errnos that name a condition the caller can act on -- EBUSY, EPERM, EACCES, EINTR, and EOPNOTSUPP, which is what the SMU answers when the firmware does not know the metrics command at all -- and report the rest as the read failure they are. The APU metrics pointer handed back with the table pointed into the device's own metrics object, which a concurrent call on the same device could replace while the caller was still reading it. Copy it into thread-local storage while the device lock is still held, which is the lifetime the headers already promise for that pointer.
- The oversized-header case declared 4 KiB more than the layout we model but wrote only a table-sized file, so dropping the read clamp ended in a short read rather than an overrun: the case never reached past the buffer and ASAN saw nothing. The blob builder now always writes at least what the header declares, so the dangerous configuration is no longer reachable by leaving an argument out, and removing the clamp reports a heap-buffer-overflow. - Nothing covered the branch that tells a read the driver refused from a node that is simply empty -- both return zero bytes, and only the stream's error flag separates them. A directory in place of the node reproduces the refusal. The test checks that premise before drawing any conclusion from the status, so a platform where a directory stops behaving that way says so instead of passing for the wrong reason. - Nothing covered the activity unit conversion. One case walks three readings chosen to separate truncation, rounding away from zero, and rounding half to even, and gives the two fields different values so a crossed-over conversion cannot hide. Another checks the not-applicable sentinel is not scaled into a plausible-looking 655; it carries a live reading beside the sentinel, because the destination is pre-filled with that same sentinel and the check would otherwise also pass if the read had populated nothing at all. - The APU field-table test built its expectation with the same expression as the code it tested, so it held for whatever that table happened to contain. Spell the v2.4 layout out instead. - The empty-clock-file skip is gated on the driver's APU bit, which MI300A also carries. That is intentional, since those parts power-gate domains too, so say so rather than leaving a comment that reads as small-APU-only.
- The return documentation for both metrics getters listed only success, an unsupported revision and an unexpected size, while the call can also report a busy device or a permission problem. List what a caller can see. - Both getters carried the same APU-metrics paragraph, but a partition metrics table resolves only the XCP revisions and never carries APU metrics, so the pointer that paragraph describes is always null for that call. Say so, and keep the part that still applies: the call invalidates a pointer an earlier whole-table query handed out on the same thread. - Record the unit the APU sub-struct reports activity in, beside the existing note that its power fields stay in milliwatts. It follows the firmware, so it is hundredths of a percent on a v2.x table and whole percent on v3.0, while the top-level fields are whole percent for every revision. Say that the not-applicable sentinel is never scaled, since callers test for it by value. - A comment described the shorter APU revisions as byte-prefix supersets of the v2.4 struct, which is backwards and contradicted the paragraph stacked under it. They are prefixes of it. Fold the two into one and note that v3.0 is a separate layout rather than a prefix. - Restore three logging lines that were re-wrapped by hand while unrelated to the change that touched them. The pinned clang-format leaves both forms alone, so the churn was never required. - The changelog entry named status constants and described the change as APU-only. Say what a user sees instead, and cover the discrete-GPU PLX change it left out.
The APU sub-struct kept v2.x GFX and MM activity in hundredths of a percent, while v3.0 reports whole percent and the Python interface and the CLI print both with a % sign. On a v2.1 APU more than 1% busy, the APU activity rows were dropped as N/A; below that they read up to 100 times too high. Convert them the same way as the top-level fields. Drop the per-metric query status change: another open pull request changes the same lines in a different way, so that change is left to it. Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Leave v2.3/v2.4 GFX/MM activity as reported: only Van Gogh publishes them, its current firmware interface already reports whole percent, and dividing turned 48% GFX busy into 0% - Fold the 100x CHANGELOG entry into the integrated GPU metrics fix, since no release reported those APUs' activity before Old Van Gogh firmware can still pair v2.2 with whole percent or v2.3 with hundredths; the table carries nothing else to tell them apart. Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Divide v3.0 average_vcn_activity by 100 with the rounding helper and keep N/A: both v3.0 kernel producers report VCN in hundredths and divide it by 100 for vcn_busy_percent - Document the field as whole percent in amdsmi.h - Add a Resolved Issues entry, since v3.0 APU metrics shipped in 7.13.0 Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Move the empty pp_dpm_* clock file case on Phoenix and Hawk Point to amdsmitst.exclude instead of a FUSION skip that also covered MI300A - Restore the gfx1103 entry in detect_asic_filter.sh Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Route setup-failure logging through log_by_metrics_status() instead of a second copy of its level switch - Log the fixed UNEXPECTED_DATA in run_internal_gpu_metrics_query() with LOG_ERROR directly - Cut multi-paragraph comments in the metrics and device read paths down to the reason behind the code - Move the APU rebind helper and legacy note next to the code they describe - Revert formatting-only changes to unrelated lines - Describe max-value readings as N/A in comments Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Tag each amdsmi_apu_metrics_t field with the first revision that carries it instead of v2_4 - Add AMDSMI_STATUS_INTERRUPT to both metrics calls' return codes and note that the driver's EPERM is temporary - Add a CHANGELOG entry for the new metrics read failure codes - Describe max-value readings as N/A in comments Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Build the fake sysfs root with mkdtemp() instead of a predictable /tmp name that is removed and recreated - Move test comments onto the tests they describe - Describe max-value readings as N/A in test names and messages Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Return 0xFFFF for every field of an APU core or L3 slot whose clock reads 0, in v2.x and v3.0 tables. Firmware zero-fills the slots of cores the part does not have, such as slots 0 and 4 on 6-core Phoenix and 6, 7, 14 and 15 on Strix Halo - Keep real 0 mW and 0 % C0 readings from present cores - Describe the rule in both public headers and the CHANGELOG - Add unit tests using real Phoenix and Strix Halo readings Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Review follow-ups for #11075 and #13540. One is a user-visible fix: on APUs with fewer CPU cores than the metrics table has slots, the missing cores read 0 °C, 0 MHz and 0 W instead of N/A. The rest don't change behavior: test exclusion, log routing, return-code docs, comment length, and fake sysfs temp dirs.
Depends on #13540. This branch is stacked on it, so #13540's 6 commits also show here until it merges and this branch is rebased. The follow-up is the last 5 commits.
Technical Details
Absent APU cores (
rocm_smi_gpu_metrics.cc,amdsmi.h,rocm_smi.h,CHANGELOG.md):Test exclusion (
amdsmitst.exclude,detect_asic_filter.sh,frequencies_read_write_test.cc):pp_dpm_*clock-file case on Phoenix/Hawk Point (gfx1103) inamdsmitst.excludeinstead of a FUSION skip that also skipped MI300Adetect_asic_filter.shMetrics read path (
rocm_smi_gpu_metrics.cc,rocm_smi_device.cc,amd_smi.cc,amd_smi_utils.cc/.h):log_by_metrics_status()instead of a second copy of its level switchUNEXPECTED_DATAinrun_internal_gpu_metrics_query()withLOG_ERRORHeader / docs (
amdsmi.h,rocm_smi.h,CHANGELOG.md):amdsmi_apu_metrics_tfield with the first revision that carries it instead ofv2_4AMDSMI_STATUS_INTERRUPTto both metrics calls' return codes; note that the driver's EPERM is temporaryUnit tests (
dynamic_metrics_test.cc,apu_metrics_v30_test.cc,gpu_metrics_read_test.cc,temp_read_test.cc):mkdtemp()instead of a predictable/tmpname that is removed and recreatedNo API, wrapper, Python or CLI code changes.
Issue Tracking
JIRA ID: ROCM-29548
Test Plan
amdsmitst --gtest_filter='*Unit*'check_test_conventions.py,check_license_headers.py,git clang-format, pre-commitamdsmi_get_gpu_metrics_info()andamd-smi metric --power --clock --temperatureon Phoenix (v2.1), Strix Halo (v3.0) and Van Gogh (v2.4)detect_asic_filter.shby reading onlyTest Result
0 MHz,0.0 °C,0.0 WN/A0 MHz,0.0 W, 0 % C0N/AN/AN/A(unchanged)Present cores keep their real readings on all three, including 0 W and 0 % C0.
Submission Checklist