Adapt GPU label support to debugfs DRM entry changes #802
Conversation
Codecov Report
@@ Coverage Diff @@
## main #802 +/- ##
==========================================
+ Coverage 53.70% 53.94% +0.23%
==========================================
Files 39 39
Lines 3545 3572 +27
==========================================
+ Hits 1904 1927 +23
- Misses 1531 1533 +2
- Partials 110 112 +2
Continue to review full report at Codecov.
|
eero-t
force-pushed
the
nfd-gen-label
branch
from
ce38351
to
449be9f
Compare
cmd/gpu_nfdhook/labeler.go
Outdated
| for prefix, action := range searchStringActionMap { | ||
| line := scanner.Text() | ||
| if !strings.HasPrefix(line, prefix) { | ||
| continue | ||
| } | ||
| fields := strings.Split(line, ": ") | ||
| if len(fields) == 2 { | ||
| action(fields[1]) | ||
| } else { | ||
| klog.Warningf("invalid '%s' line format: '%s'", file.Name(), line) | ||
| } | ||
| delete(searchStringActionMap, prefix) | ||
| if len(searchStringActionMap) == 0 { | ||
| return | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
Do I understand it right that we take a first entry in searchStringActionMap (which can be any) and look for it in the first line. If not found we take another entry and look for it in the second line (but not in the first line). If we are lucky and the second entry of the unordered map coincided with the second line then we either stop iterating or return from the function at all.
There was a problem hiding this comment.
Admittedly the code is a bit convoluted, partly because the original was also, before Eero touched it. My folt.
The code does indeed take the first entry in the searchStringActionMap and looks for the prefix from THE line. If the prefix is NOT found, another string from the map is attempted (the continue -path) with the same line from the scanner. The scanner does not progress forward with a call to Text(). Perhaps one could take the "line := scanner.Text()" out of the loop? Probably.
If the string IS found, well eventually either the whole function returns in case there are no more strings left in the map, or we proceed with the break-path which means the for loop exits, and another line is read from the scanner. That means, another line from the capability file gets investigated with the same reducing search map.
|
I wondered why tests did not catch the bug that I had retained "return" [1] for the case when "searchStringActionMap" goes empty, until I realized that it does not get empty any more. There is either "gen" or *_version strings in the capability file, but not both i.e. with new kernels the whole capability file will be scanned (even without this PR), until we can remove check for "gen"... (And "gen" support can be dropped only when even LTS kernels have upgraded from "gen" to *_version fields.) [1] Check should break from both loops using name break (or goto), or by being moved one level up (i.e. would be done for every line). Any preferences? |
| if len(searchStringActionMap) == 0 { | ||
| break scanning | ||
| } |
There was a problem hiding this comment.
I guess now this check can be moved right after the loop. Then there would be no need for the loop label.
There was a problem hiding this comment.
Then it would be run for every capabilities line, instead of only when one of the prefixes matches. With drm-tip on DG1:
# wc -l /sys/kernel/debug/dri/*/i915_capabilities
104 /sys/kernel/debug/dri/0/i915_capabilities
i.e. 104 instead of 2-3 times.
There was a problem hiding this comment.
The internal loop breaks right after the check unconditionally. I don't expect it to keep iterating.
There was a problem hiding this comment.
It keeps iterating because it never becomes empty with current kernels, and while both "gen" and *_version fields are supported. All the kernel capability files I have seen, have either "gen" or *_version fields, not both.
There was a problem hiding this comment.
I missed something. What never becomes empty? searchStringActionMap?
If one of the prefixes matches then the internal loop breaks and doesn't iterate regardless of the map's emptiness.
There was a problem hiding this comment.
Yes, currently searchStringActionMap never becomes empty for the reason I stated, therefore all lines in the capability file will be parsed (even before this PR, in case of newer kernels). Along with the number of map keys increasing, that's why this PR optimizes the map key comparisons.
There was a problem hiding this comment.
Ok. Now I see what you mean. Thanks! Let's keep it this way too.
| if len(searchStringActionMap) == 0 { | ||
| break scanning | ||
| } |
There was a problem hiding this comment.
Ok. Now I see what you mean. Thanks! Let's keep it this way too.
|
@eero-t can you squash the commits still, thanks! |
GPU generation "gen" number is replaced in the capability files of latest kernels with separate display, graphics, and media versions. For compatibility with newer kernels, provide "gen" based on the new labels (but without decimals), and for older kernel compatibility, new labels based on the "gen". Because different kernels match different items from the action map, whole capability file will get parsed. Capability file parsing is optimized by using prefix check instead of scanf. "platform_gen" label is deprecated, and can be dropped whenever it becomes inconvenient (lint complains about line count etc).
eero-t
force-pushed
the
nfd-gen-label
branch
from
6b116e5
to
bcc737b
Compare
|
Rebased to latest main, squashed to single commit, and updated the commit description. |
"gen" number is in latest kernels capabilities files replaced with separate display, graphics, and media versions.
For compatibility with newer kernels, provide "gen" based on the new labels (but without decimals), and for older kernel compatibility, new labels based on "gen".
Optimize capabilities file parsing by using prefix check instead of scanf.