(feat) Support AMD GPU for Insights#1147
Merged
Merged
Conversation
github-actions
Bot
added
rust
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
Rust Backend Coverage ReportCoverage Details |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the GPU monitoring system to support AMD and Intel GPUs on both Windows and Linux, moving away from NVIDIA-only support to a vendor-agnostic architecture. The changes enable per-GPU metrics collection and archival for multiple vendors, improve data availability feedback in the insights UI, and establish a scalable foundation for cross-vendor GPU monitoring.
Changes:
- Refactored GPU history storage from NVIDIA-specific (
nv_gpu_*_histories) to vendor-agnostic (gpu_*_histories) across backend models and services - Implemented AMD GPU monitoring via ADL on Windows with new per-adapter async functions for usage and temperature metrics
- Added Linux GPU metrics collection for AMD (via DRM/sysfs/hwmon) and Intel (via intel_gpu_top)
- Updated archive service to calculate per-GPU statistics by device name instead of aggregating across all GPUs
- Enhanced frontend InsightChart component to detect and display "no data available" messages for empty data periods
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/lang/en.json | Added "noDataForPeriod" translation key for empty data message |
| src/lang/ja.json | Added Japanese translation for "noDataForPeriod" message |
| src/features/hardware/insights/hooks/useInsightChart.ts | Added hasData field to return value for empty data detection |
| src/features/hardware/insights/components/InsightChart.tsx | Added no-data UI fallback, useEffect for data availability callback, and onDataAvailabilityChange prop |
| src-tauri/src/workers/system_monitor.rs | Changed sample_gpu calls to await async invocations on Windows and Linux |
| src-tauri/src/services/monitoring_service.rs | Introduced GpuSample type, refactored sample_gpu to async, added AMD sampling on Windows, implemented Linux GPU collection, updated update_gpu_histories to handle optional metrics |
| src-tauri/src/services/archive_service.rs | Updated GpuMetricsCollector to use vendor-agnostic history fields and calculate per-GPU stats by name |
| src-tauri/src/models/hardware_archive.rs | Renamed nv_gpu_* fields to gpu_* for vendor-agnostic support |
| src-tauri/src/models/hardware.rs | Renamed nv_gpu_* fields to gpu_* in HardwareMonitorState |
| src-tauri/src/lib.rs | Updated initialization to use renamed gpu_* history fields |
| src-tauri/src/infrastructure/providers/windows/adl_provider.rs | Added get_amd_gpu_usage_per_adapter and get_amd_gpu_temperatures_per_adapter async functions for per-adapter metrics |
Comments suppressed due to low confidence (5)
src/features/hardware/insights/components/InsightChart.tsx:157
- The check
chartData.every((v) => v == null)at line 149 is redundant because it duplicates thehasDatacomputation fromuseInsightChart(line 331:hasData = filledChartData.some((v) => v != null)). The condition at line 149 is checking if all values are null, whilehasDataalready tracks whether any non-null values exist. Replace this check withif (!hasData)to maintain consistency and avoid duplicate logic.
if (chartData.every((v) => v == null)) {
return (
<div className="flex h-full w-full items-center justify-center">
<span className="text-muted-foreground">
{t("pages.insights.noDataForPeriod")}
</span>
</div>
);
}
src/features/hardware/insights/hooks/useInsightChart.ts:333
- The new
hasDatafield returned byuseInsightChartlacks test coverage. The existing tests verifylabelsandchartDatabut don't assert on thehasDataboolean, which is critical for determining when to display the "no data" message in the UI. Add test cases that verifyhasDatais true when data exists and false when data is empty.
const hasData = filledChartData.some((v) => v != null);
return { labels: filledLabels, chartData: filledChartData, hasData };
src-tauri/src/services/monitoring_service.rs:168
- The
get_gpu_name_from_lspci_by_vendor_idfunction is called once with vendor ID "1002" (AMD), but is used for all AMD cards in the loop. If there are multiple AMD GPUs, all will receive the same name from the first match found by lspci, making them indistinguishable in the UI and history storage. Consider usingcard_idto query lspci for each specific card or include the card ID in the name to ensure unique identification.
let name =
crate::infrastructure::providers::lspci::get_gpu_name_from_lspci_by_vendor_id(
"1002",
)
.unwrap_or_else(|| format!("AMD GPU (card{})", card_id));
src-tauri/src/services/monitoring_service.rs:182
- The
get_intel_gpu_usage()function does not accept acard_idparameter, so if there are multiple Intel GPUs, all cards will report the same usage value returned byintel_gpu_top. This function should be modified to accept a card_id parameter and query the specific card's usage, or multiple Intel GPUs will show identical metrics.
let usage = drm_sys::get_intel_gpu_usage()
.await
.map(|u| (u * 100.0) as f32)
.ok();
src/features/hardware/insights/components/InsightChart.tsx:100
- The
onDataAvailabilityChangecallback is defined as an optional parameter but is never passed whenGpuInsightChartis invoked in the parent component (Insights.tsx). This means the callback will never be triggered, making theuseEffecthook that calls it ineffective. Either pass a handler from the parent component or remove the unused callback parameter and effect.
useEffect(() => {
onDataAvailabilityChange?.(hasData);
}, [hasData, onDataAvailabilityChange]);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
src-tauri/src/services/monitoring_service.rs:184
- The
get_intel_gpu_usage()function is called for each Intel card in the loop, but it doesn't accept a card_id parameter and appears to return global Intel GPU usage (usingintel_gpu_topwhich typically reports aggregate usage). When multiple Intel GPUs are present, each card will receive the same usage value. This should either be refactored to support per-card usage or documented as a known limitation.
drm_sys::GpuVendor::Intel => {
let usage = drm_sys::get_intel_gpu_usage()
.await
.map(|u| (u * 100.0) as f32)
.ok();
(format!("Intel GPU (card{})", card_id), usage, None)
src/features/hardware/insights/components/InsightChart.tsx:100
- The
onDataAvailabilityChangecallback prop is defined and called whenhasDatachanges, but it's not being passed or used by the parent component inInsights.tsx(line 458-464). The prop was added to allow the parent to respond to data availability changes, but without wiring it up in the parent, this functionality remains unused. Consider either removing this prop if it's not needed yet, or implementing the callback in the parent component to handle the no-data state appropriately.
useEffect(() => {
onDataAvailabilityChange?.(hasData);
}, [hasData, onDataAvailabilityChange]);
src/features/hardware/insights/components/InsightChart.tsx:149
- The condition
chartData.every((v) => v == null)on line 149 is redundant since thehasDatavariable already performs this check on line 331 ofuseInsightChart.tsusingfilledChartData.some((v) => v != null). The condition here is logically equivalent but inverted. Consider using!hasDatadirectly instead to avoid duplication and improve maintainability.
if (chartData.every((v) => v == null)) {
src/features/hardware/insights/hooks/useInsightChart.ts:334
- The new
hasDatafield returned byuseInsightChartis not tested. The test on line 137 validates empty data handling but doesn't check thehasDatareturn value. Consider adding assertions forresult.current.hasDatain existing tests (e.g.,expect(result.current.hasData).toBe(true)when data is present andexpect(result.current.hasData).toBe(false)in the empty data test on line 137) to ensure the new field works correctly.
const hasData = filledChartData.some((v) => v != null);
return { labels: filledLabels, chartData: filledChartData, hasData };
};
src-tauri/src/services/monitoring_service.rs:169
- The
get_gpu_name_from_lspci_by_vendor_idfunction returns only the first matching GPU name found (line 15 returns immediately on first match). When multiple AMD GPUs are present, all AMD cards will receive the same name in the loop. This will cause incorrect GPU identification and potential data aggregation issues. Consider passing the card_id to the lspci function to match the specific card, or use a different naming strategy that includes the card ID to ensure unique names.
let name =
crate::infrastructure::providers::lspci::get_gpu_name_from_lspci_by_vendor_id(
"1002",
)
.unwrap_or_else(|| format!("AMD GPU (card{})", card_id));
github-actions Bot
pushed a commit
that referenced
this pull request
Feb 24, 2026
- Features table: replace "GPU Insight (non-NVIDIA)⚠️ " with separate rows for AMD/Linux (✅) and macOS (✅) - Insight screenshot section: update note from NVIDIA-only to reflect NVIDIA, AMD (Windows/Linux), and Apple GPU (macOS via IOKit) support - FAQ: replace NVIDIA-only question with accurate multi-vendor answer - Roadmap: mark AMD GPU (Insights) and macOS GPU Monitoring as done - README.ja.md: apply same changes in Japanese; fix macOS OS table status from 🚧 to ✅ Triggered by: - #1147 (feat) Support AMD GPU for Insights - #1161 feat: add macOS GPU sampling functionality to monitoring service Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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.
This pull request introduces cross-vendor GPU monitoring support, refactoring the GPU metrics collection and storage system to be vendor-agnostic and adding AMD and Linux GPU metrics collection. It also updates the data pipeline and UI to handle the new structure and improves data availability feedback in the insights chart.
GPU Monitoring Refactor and Vendor-Agnostic Support:
nv_gpu_*_histories) with genericgpu_*_historiesin state and resource models, and updates all usages throughout the backend to use the new generic fields. [1] [2] [3] [4] [5] [6] [7]GpuSampletype. [1] [2] [3]AMD and Linux GPU Metrics Collection:
get_amd_gpu_usage_per_adapterandget_amd_gpu_temperatures_per_adapter.Data Pipeline and Stats Calculation Updates:
Frontend Improvements:
onDataAvailabilityChangecallback and a fallback UI message. [1] [2] [3] [4]Async Monitoring Pipeline:
These changes lay the groundwork for unified GPU monitoring across vendors and platforms, improve code maintainability, and enhance the user experience when data is unavailable.