Linux: read GPU usage from drm-cycles when there is no ns counter - #2069
Linux: read GPU usage from drm-cycles when there is no ns counter#2069bogdanr wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughGPU accounting parses DRM timestamp, busy-cycle, and total-cycle metrics through shared helpers. It filters duplicate client sections and persists raw counters between scans. Poem
Merge Risk: 🟡 Moderate · up to This change enables GPU utilization reporting from DRM cycle counters, but malformed or rejected counters can still produce incorrect one-sample GPU percentages, and processes using multiple DRM devices may receive an inaccurate combined ratio. Merge should wait for these bounded reporting-correctness issues to be fixed or explicitly accepted. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8dc9927e-4b62-4b2f-bfd4-9886da69682f
📒 Files selected for processing (5)
linux/GPU.clinux/LinuxMachine.hlinux/LinuxProcess.hlinux/LinuxProcessTable.clinux/Platform.c
bfad1a2 to
d1ba2f3
Compare
|
Force pushed one fixup after getting hold of an AMD machine (Strix Halo, It found a bug in the first version. Marking a process active whenever it has a counter at all, as on master, keeps Numbers before and after are in the description, the two builds now agree sample |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
linux/GPU.c (2)
48-54: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep device identity when aggregating total-cycle counters.
get_machine_gpu_engine()keys counters byengineonly. The maximum rule is valid only for reports from the samepdev. If two devices expose the same engine name, such asrcs0, Lines 95-96 merge their device-wide totals and discard one denominator.Retain
pdevin the aggregation identity. Calculate each device ratio separately before combining device activity. The PR objective defines total-cycle counters as device-wide.Also applies to: 92-96, 275-277
48-135: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse module-prefixed names for the new helpers.
Rename
get_machine_gpu_engine,update_machine_gpu,update_machine_gpu_cycles,update_machine_gpu_total_cycles,count_section, andparse_engine_valueto theGPU_functionName()convention.As per coding guidelines:
**/*.c: “Use ModuleName_functionName() naming convention for functions.”Source: Coding guidelines
d1ba2f3 to
a9b5ab0
Compare
|
Two of the three comments are worth fixing, so I did both in Raw counters on the error path. Consolidating the three branches. - Leaving this one. They look alike, but the |
| } | ||
|
|
||
| static void update_machine_gpu(LinuxProcessTable* lpt, unsigned long long int time, const char* engine, size_t engine_len) { | ||
| static GPUEngineData* get_machine_gpu_engine(LinuxProcessTable* lpt, const char* engine, size_t engine_len) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required GPU_ function naming convention.
The new helper names use snake_case. Rename them to GPU_<functionName> names and update their local call sites. For example, use GPU_getMachineGpuEngine() instead of get_machine_gpu_engine().
As per coding guidelines: "**/*.c: Use ModuleName_functionName() naming convention for functions."
Also applies to: 79-79, 86-86, 92-92, 99-99, 114-115
Source: Coding guidelines
Drivers may report engine usage either as a busy time in nanoseconds (drm-engine-<engine>), or as busy cycles plus the cycles elapsed on that engine (drm-cycles-<engine> and drm-total-cycles-<engine>). We only ever handled the first form. Intel's Xe driver, which took over from i915 on recent hardware, only implements the second one. On a Lunar Lake laptop the GPU meter and the GPU_PERCENT/GPU_TIME columns therefore sit at 0.0% while nvtop and gputop happily show the GPU pegged. So take the ratio of busy to elapsed cycles and scale it with the sampling interval, which lets it be accounted just like the nanosecond values. Nice side effect: the ratio doesn't depend on the wall clock, and the GPU frequency moves around a lot. drm-total-cycles is a device wide counter that every client repeats, so take the maximum of it instead of summing it up. While here, compute the meter residue from the per engine deltas instead of keeping a second absolute counter around, and skip the very first sample, where there is no interval to divide by yet - it used to report nonsense such as "20679d" of total GPU time in the first frame.
a9b5ab0 to
85873cb
Compare
|
Braces added in Not doing the rename to |
|
The AMD machine came back, so the |
| const char* delim = strchr(engineStart, ':'); | ||
| if (!delim) | ||
| return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an empty engine name.
parse_engine_value accepts drm-engine-: 1 ns because delim == engineStart is valid. The caller then creates and updates an engine with an empty key. Reject this malformed metric.
Proposed fix
- if (!delim)
+ if (!delim || delim == engineStart)
return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const char* delim = strchr(engineStart, ':'); | |
| if (!delim) | |
| return false; | |
| const char* delim = strchr(engineStart, ':'); | |
| if (!delim || delim == engineStart) | |
| return false; |
| if (parse_engine_value(line, "engine-", " ns", &engine, &engine_len, &value)) { | ||
| if (count_section(&sstate, client_id, pdev, parsed_ids)) { | ||
| new_gpu_time += value; | ||
| update_machine_gpu(lpt, value, engine, engine_len); | ||
| } | ||
| } | ||
| } else if (line[0] == 'c' && String_startsWith(line, "cycles-")) { | ||
| /* Drivers that cannot provide a nanosecond resolution timestamp | ||
| * (e.g. Intel Xe) export the busy cycles of an engine together with | ||
| * the cycles elapsed on that engine. */ | ||
| if (sstate == SECST_DUPLICATE) | ||
| continue; | ||
|
|
||
| char* endptr; | ||
| errno = 0; | ||
| unsigned long long int value = strtoull(delim + 1, &endptr, 10); | ||
| if (errno == 0 && String_startsWith(endptr, " ns")) { | ||
| if (sstate == SECST_UNKNOWN) { | ||
| if (client_id != INVALID_CLIENT_ID && !is_duplicate_client(parsed_ids, client_id, pdev)) | ||
| sstate = SECST_NEW; | ||
| else | ||
| sstate = SECST_DUPLICATE; | ||
| const char* engine; | ||
| size_t engine_len; | ||
| unsigned long long int value; | ||
| if (parse_engine_value(line, "cycles-", "", &engine, &engine_len, &value)) { | ||
| if (count_section(&sstate, client_id, pdev, parsed_ids)) { | ||
| new_gpu_cycles += value; | ||
| update_machine_gpu_cycles(lpt, value, engine, engine_len); | ||
| } | ||
| } | ||
| } else if (line[0] == 't' && String_startsWith(line, "total-cycles-")) { | ||
| if (sstate == SECST_DUPLICATE) | ||
| continue; | ||
|
|
||
| if (sstate == SECST_NEW) { | ||
| new_gpu_time += value; | ||
| update_machine_gpu(lpt, value, engineStart, delim - engineStart); | ||
| const char* engine; | ||
| size_t engine_len; | ||
| unsigned long long int value; | ||
| if (parse_engine_value(line, "total-cycles-", "", &engine, &engine_len, &value)) { | ||
| if (count_section(&sstate, client_id, pdev, parsed_ids)) { | ||
| /* The same free running counter is reported for all engines | ||
| * of a device, so don't accumulate it. */ | ||
| if (value > new_gpu_totalCycles) | ||
| new_gpu_totalCycles = value; | ||
| update_machine_gpu_total_cycles(lpt, value, engine, engine_len); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep raw baselines after a rejected metric.
If parse_engine_value rejects a matching metric, its accumulator remains zero or partial. Lines 332-334 then replace the prior raw baseline. The next valid absolute counter can be reported as a full-uptime delta in one scan.
Track parse success per counter family. If a matching metric is rejected, retain that family’s previous raw counter and clear gpu_percent for the scan.
Also applies to: 332-334
| uint64_t monotonicTimeDelta = host->monotonicMs - host->prevMonotonicMs; | ||
| unsigned long long int gputimeDelta = saturatingSub(new_gpu_time, lp->gpu_timeRaw); | ||
|
|
||
| /* Cycle based accounting only yields a ratio of busy to elapsed cycles, | ||
| * which is turned into a busy time using the sampling interval. */ | ||
| unsigned long long int cyclesDelta = saturatingSub(new_gpu_cycles, lp->gpu_cycles); | ||
| unsigned long long int totalCyclesDelta = lp->gpu_totalCycles ? saturatingSub(new_gpu_totalCycles, lp->gpu_totalCycles) : 0; | ||
| if (cyclesDelta > 0 && totalCyclesDelta > 0) | ||
| gputimeDelta += (unsigned long long int)((double)cyclesDelta / totalCyclesDelta * monotonicTimeDelta * (1000 * 1000)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the raw GPU counter fields and process initialization paths.
ast-grep outline linux/LinuxProcess.h --items all --view expanded
fd -t f '^LinuxProcess\.(c|h)$' linux | xargs -r rg -n -C 4 \
'\bgpu_timeRaw\b|\bgpu_cycles\b|\bgpu_totalCycles\b|GPU_readProcessData'Repository: htop-dev/htop
Length of output: 3142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace LinuxProcess allocation and all assignments to the GPU raw fields.
fd -t f . linux | sort | xargs -r rg -n -C 5 \
'LinuxProcess_new|gpu_timeRaw|gpu_cycles|gpu_totalCycles|xCalloc|calloc|memset'Repository: htop-dev/htop
Length of output: 14396
Skip delta accounting on the first GPU sample.
LinuxProcess_new() zero-initializes gpu_timeRaw, so the first sample subtracts zero from the absolute new_gpu_time counter and reports lifetime GPU time as interval usage. Add explicit baseline state and skip delta calculation until the raw counters are initialized.
ravi-arnan
left a comment
ravi-arnan
left a comment
There was a problem hiding this comment.
You have covered xe and amdgpu. I have an Alder Lake-P / Iris Xe laptop on
i915, which is a third driver on the drm-engine-* nanosecond path, so I
built the branch and exercised that half.
No regression there. Clean build, no new warnings. With vblank_mode=0 glxgears
under load, both upstream main and this branch report non-zero,
similar-magnitude GPU% and a monotonically increasing GPU_TIME for the same
process:
main 18.1 17.9 28.2 36.8 21.1 17.8 ...
pr2069 15.7 21.1 19.8 21.5 16.0 ...
(Samples are not aligned and the load is not steady, so read that as shape, not
as a numeric comparison.)
One thing worth recording for anyone else trying to check the numbers from
outside htop: a naive sum over /proc/<pid>/fdinfo/* multiplies the result. My
first cross-check script read roughly four times the real utilisation, because
glxgears holds four fds that are all the same DRM client:
/proc/522302/fdinfo/{4,5,6,7}
drm-client-id: 135 (identical in all four)
drm-pdev: 0000:00:02.0
drm-engine-render: 33641346752 ns / 33641937680 / 33642722984 / 33643895844
That is exactly what is_duplicate_client() already handles, so htop is right
and the script was wrong, but it means any external verification has to dedupe
on (drm-pdev, drm-client-id) first.
Two things in the diff looked risky enough to check, and both are fine. Noting
them so nobody else has to re-derive them:
parse_engine_value()with an empty unit requires the number to be the last
thing on the line (*endptr != '\0'). That holds only because lines come from
strsep(&buf, "\n")and therefore carry no trailing newline. It is correct as
written, but it is a non-local assumption; a comment on the empty-unit case
would keep a future change to the line splitting from breaking the cycles path
silently.lp->gpu_timechanges meaning, from "last raw counter value" to "accumulated
busy time". The displayed GPU_TIME survives that, because on the first sample
saturatingSub(new_gpu_time, 0)is the whole lifetime counter, so the
accumulator starts from the same place the old raw assignment did.
One actual question, on the cycles branch:
gputimeDelta += (unsigned long long int)((double)cyclesDelta / totalCyclesDelta * monotonicTimeDelta * (1000 * 1000));monotonicTimeDelta is the global sampling interval, but a process can have
been skipped for up to 5 seconds by the activity check at the top of the
function. On the ns path that skew is visible: the delta covers 5 seconds of
busy time and is divided by one interval, so the percentage overshoots. On the
cycles path it is silently smoothed instead, since the ratio is already
normalised over whatever period the counters covered, and it is then scaled by a
single interval. So the two paths disagree in that corner. Is that intended? I
can see the argument that the cycles behaviour is the more correct of the two,
in which case the ns path is the one worth revisiting later.
I cannot test the drm-cycles-* half here, since i915 only exports
drm-engine-*.
2 participants
On my Lunar Lake laptop (Arc 140V,
xedriver) the GPU meter and the GPU columnsare stuck at
0.0%no matter what the GPU is doing, while nvtop and gputop showit pegged.
The reason is that drm-usage-stats
allows two ways of reporting engine utilisation:
drm-engine-<engine>: <uint> ns- busy time in nanosecondsdrm-cycles-<engine>plusdrm-total-cycles-<engine>- busy cycles andelapsed cycles, whose ratio is the utilisation
We only parse the first one, and the Xe driver only implements the second one.
There is not a single
drm-engine-*line to be found:Since Xe is what recent Intel hardware gets by default, this will affect more
and more machines.
intel_gpu_tophas the same problem, it refuses to start andtells you to use
gputopinstead.What the patch does
GPU.cnow also parsesdrm-cycles-*anddrm-total-cycles-*, reusing theexisting client-id/pdev deduplication. Utilisation is
Δcycles / Δtotal-cycles,scaled with the sampling interval so it can be added up together with the
nanosecond values everywhere else. Doing it as a ratio means there is no need to
know the GPU clock, which is good, because on this machine it swings between
400 MHz and 1950 MHz while sampling.
drm-total-cyclesis a device wide free running counter that every clientrepeats, so it is aggregated with max instead of sum, both per process and per
engine.
Because the raw counters are no longer always nanoseconds,
LinuxProcesskeepsthem separately (
gpu_timeRaw,gpu_cycles,gpu_totalCycles) andgpu_timeaccumulates the observed busy time. For
drm-engine-*drivers the accumulatedvalue comes out the same as the old absolute one, except it no longer goes
backwards when a client closes its fds.
Two things in
Platform.cthat got in the way and are fixed here as well: themeter residue is now derived from the per engine deltas instead of a second
absolute counter, and the first sample is skipped because there is no interval
to divide by yet. That first sample used to display things like
20679dof GPUtime.
I also pulled the line parsing and the duplicate client bookkeeping into two
small helpers, they were about to be written a third time.
Testing
One machine per reporting style.
Arc 140V,
xe, cycle counters, compared againstgputopfromigt-gpu-tools:
With glxgears running, the meter total of 85.6-86.4 % matches gputop adding up
its clients for the same period (Xorg 40.3 % + glxgears 45.3 % + picom 1.7 %).
Radeon 8060S (Strix Halo),
amdgpu, nanosecond counters, to check that theexisting path did not move. Two glmark2 processes rendering off-screen at
1920x1080,
gpu_busy_percentpinned at 100 %, one sample per second, thisbranch next to the same tree without the patch:
Per process both glmark2 clients read ~49 % in either build, while an
independent aggregation of the fdinfo counters says 50.08 % and 49.92 %. Killing
the load gives a flat 0.00 % for as long as I watched it.
Builds clean with
-Wall -Wextraon both machines, and with--enable-debug,where no assertion fired during the runs. Still untested on NVIDIA.
Two things I noticed but did not touch
GPUMeter_engineDataholds four engines while Xe exposes five (rcs,vcs,vecs,bcs,ccs), so the fifth one always lands in the residue. On thislaptop compute work goes to
ccs, which means the named bars stay empty whilethe total is right. Bumping the array is easy, but it needs a fifth colour and I
did not want to mix that in here.
A process with fds on two devices whose
drm-total-cyclescounters differ willbe slightly off, as only one reference counter is kept per process. That is the
same kind of conflation as #1657.