Fix: [gpu-info] The gpu info not show in special platform.#540
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/eaglefrom Oct 9, 2025
Merged
Conversation
-- adjust the code logic to get gpu info. Log: fix issue Bug: https://pms.uniontech.com/bug-view-333969.html
Reviewer's GuideCentralize and refactor GPU information retrieval by introducing a cached utility in CommonTools that leverages D-Bus or glxinfo, simplifying existing interfaces and integrating the new flow in generator and startup logic. Sequence diagram for the new GPU info retrieval flowsequenceDiagram
participant App
participant CommonTools
participant D-Bus
participant DeviceInfoService
participant glxinfo
App->>CommonTools: getGpuInfoCommandFromDConfig()
App->>CommonTools: preGenerateGpuInfo()
alt D-Bus available
CommonTools->>D-Bus: getGpuInfoByCustom(arguments, gpuInfo)
D-Bus->>DeviceInfoService: getGpuInfoByCustom(cmd, arguments)
DeviceInfoService->>glxinfo: execute GPU info command
glxinfo-->>DeviceInfoService: GPU info output
DeviceInfoService-->>D-Bus: GPU info
D-Bus-->>CommonTools: GPU info
else D-Bus not available
CommonTools->>glxinfo: execute glxinfo -B
glxinfo-->>CommonTools: GPU info output
end
CommonTools-->>App: GPU info
Class diagram for updated CommonTools GPU info methodsclassDiagram
class CommonTools {
+static QString getGpuInfoCommandFromDConfig()
+static QString preGenerateGpuInfo()
-static bool getGpuBaseInfo(QMap<QString, QString> &mapInfo)
}
Class diagram for updated DeviceInterface GPU info methodclassDiagram
class DeviceInterface {
+QString getGpuInfoByCustom(const QString &cmd, const QStringList &arguments)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In preGenerateGpuInfo(), return immediately after logging if DISPLAY or XAUTHORITY is missing to avoid making a DBus call with invalid arguments.
- Move the QRegularExpression instance outside the loop (e.g. as a static) to avoid recompiling it for every line in getGpuBaseInfo.
- The static gpuInfo cache in preGenerateGpuInfo isn’t thread-safe—consider protecting it with a mutex or refactoring to avoid data races in multi-threaded contexts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In preGenerateGpuInfo(), return immediately after logging if DISPLAY or XAUTHORITY is missing to avoid making a DBus call with invalid arguments.
- Move the QRegularExpression instance outside the loop (e.g. as a static) to avoid recompiling it for every line in getGpuBaseInfo.
- The static gpuInfo cache in preGenerateGpuInfo isn’t thread-safe—consider protecting it with a mutex or refactoring to avoid data races in multi-threaded contexts.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/Tool/commontools.cpp:269` </location>
<code_context>
return cmd;
}
+
+QString CommonTools::preGenerateGpuInfo()
+{
+ static QString gpuInfo { "" };
</code_context>
<issue_to_address>
**issue (bug_risk):** Static caching of GPU info may cause stale data if environment changes.
Caching with a static QString prevents updates if DISPLAY or XAUTHORITY change. Please review if caching is required, or implement invalidation when these environment variables are modified.
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager/src/Tool/commontools.cpp:278-282` </location>
<code_context>
- QProcessEnvironment env = QProcessEnvironment::systemEnvironment();
- QString display = env.value("DISPLAY");
- QString xauthority = env.value("XAUTHORITY");
- if (display.isEmpty() || xauthority.isEmpty()) {
- qWarning() << "DISPLAY or XAUTHORITY is not set!";
- } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Function continues execution even if DISPLAY or XAUTHORITY is missing.
Recommend returning immediately after logging the error to prevent further execution when DISPLAY or XAUTHORITY is missing.
```suggestion
if (display.isEmpty() || xauthority.isEmpty()) {
qCritical() << "DISPLAY or XAUTHORITY is not set!";
return gpuInfo;
} else {
arguments << display << xauthority;
}
```
</issue_to_address>
### Comment 3
<location> `deepin-devicemanager/src/Tool/commontools.cpp:289` </location>
<code_context>
+ "org.deepin.DeviceInfo",
+ QDBusConnection::systemBus());
+ if (iface.isValid()) {
+ QDBusReply<QString> replyList = iface.call("getGpuInfoByCustom", arguments, gpuInfo);
+ if (replyList.isValid()) {
+ gpuInfo = replyList.value();
</code_context>
<issue_to_address>
**question:** Passing gpuInfo as an argument to DBus call may be unnecessary.
Please verify whether gpuInfo should be passed as an input to the DBus method, as it seems to be intended as an output parameter.
</issue_to_address>
### Comment 4
<location> `deepin-devicemanager/src/Tool/commontools.cpp:328-329` </location>
<code_context>
+ if (match.hasMatch()) {
+ QString key = match.captured(1).trimmed();
+ QString value = match.captured(2).trimmed();
+ if (key == "OpenGL renderer string") {
+ mapInfo.insert(kName, value);
+ mapInfo.insert(kModel, value);
+ } else if (key == "OpenGL vendor string") {
</code_context>
<issue_to_address>
**suggestion:** Assigning both kName and kModel to the same value may reduce clarity.
Verify that assigning the same value to both fields matches their intended use and does not cause confusion for downstream consumers.
Suggested implementation:
```cpp
if (key == "OpenGL renderer string") {
// Assign renderer string to kModel, not kName, for clarity
mapInfo.insert(kModel, value);
} else if (key == "OpenGL vendor string") {
```
If you want to provide a more descriptive name for `kName`, you could use a default value or combine vendor and renderer strings after parsing both. For now, this change ensures that `kModel` is set to the renderer string, which is typically more appropriate.
</issue_to_address>
### Comment 5
<location> `deepin-devicemanager-server/customgpuinfo/main.cpp:101-102` </location>
<code_context>
int main(int argc, char *argv[])
{
QMap<QString, QString> mapInfo;
- if (getGpuBaseInfo(mapInfo)) {
- getGpuMemInfoForFTDTM(mapInfo);
+ if (getGpuMemInfoForFTDTM(mapInfo)) {
for (auto it = mapInfo.begin(); it != mapInfo.end(); ++it)
</code_context>
<issue_to_address>
**suggestion:** getGpuMemInfoForFTDTM return value is now used for conditional logic.
Please verify that error handling and fallback logic remain correct if getGpuMemInfoForFTDTM fails.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return cmd; | ||
| } | ||
|
|
||
| QString CommonTools::preGenerateGpuInfo() |
There was a problem hiding this comment.
issue (bug_risk): Static caching of GPU info may cause stale data if environment changes.
Caching with a static QString prevents updates if DISPLAY or XAUTHORITY change. Please review if caching is required, or implement invalidation when these environment variables are modified.
| "org.deepin.DeviceInfo", | ||
| QDBusConnection::systemBus()); | ||
| if (iface.isValid()) { | ||
| QDBusReply<QString> replyList = iface.call("getGpuInfoByCustom", arguments, gpuInfo); |
There was a problem hiding this comment.
question: Passing gpuInfo as an argument to DBus call may be unnecessary.
Please verify whether gpuInfo should be passed as an input to the DBus method, as it seems to be intended as an output parameter.
Comment on lines
+328
to
+329
| if (key == "OpenGL renderer string") { | ||
| mapInfo.insert(kName, value); |
There was a problem hiding this comment.
suggestion: Assigning both kName and kModel to the same value may reduce clarity.
Verify that assigning the same value to both fields matches their intended use and does not cause confusion for downstream consumers.
Suggested implementation:
if (key == "OpenGL renderer string") {
// Assign renderer string to kModel, not kName, for clarity
mapInfo.insert(kModel, value);
} else if (key == "OpenGL vendor string") {
If you want to provide a more descriptive name for kName, you could use a default value or combine vendor and renderer strings after parsing both. For now, this change ensures that kModel is set to the renderer string, which is typically more appropriate.
Comment on lines
-101
to
-102
| if (getGpuBaseInfo(mapInfo)) { | ||
| getGpuMemInfoForFTDTM(mapInfo); |
There was a problem hiding this comment.
suggestion: getGpuMemInfoForFTDTM return value is now used for conditional logic.
Please verify that error handling and fallback logic remain correct if getGpuMemInfoForFTDTM fails.
max-lvs
approved these changes
Oct 7, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GongHeng2017, max-lvs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/forcemege |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
deepin-bot
bot
merged commit 85e94c5
into
linuxdeepin:develop/eagle
16 of 18 checks passed
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
3 participants
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.
-- adjust the code logic to get gpu info.
Log: fix issue
Bug: https://pms.uniontech.com/bug-view-333969.html
Summary by Sourcery
Fix GPU info retrieval and display by centralizing fetching logic in CommonTools and updating usage across the application
New Features:
Bug Fixes:
Enhancements: