Skip to content

accel/amdxdna: add query DPM level and DPM frequency table support#1465

Open
donwalkarsoham wants to merge 1 commit into
amd:mainfrom
donwalkarsoham:xrt-smi-validate-dpm
Open

accel/amdxdna: add query DPM level and DPM frequency table support#1465
donwalkarsoham wants to merge 1 commit into
amd:mainfrom
donwalkarsoham:xrt-smi-validate-dpm

Conversation

@donwalkarsoham

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves clock/TOPS reporting for the amdxdna AIE4/NPU3 path by adding firmware queries for the current DPM level and the firmware-programmed (per-silicon) DPM frequency tables, then using those to report live domain frequencies.

Changes:

  • Add new AIE4 management message opcodes and ABI structs for querying DPM frequency tables and current DPM levels.
  • Cache firmware-authoritative DPM frequency tables at probe and use them (plus a live DPM level query) to resolve current AIE/NPU-H frequencies.
  • Update clock metadata reporting to rely on refreshed live clock values from aie_update_counters().

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
drivers/accel/amdxdna/npu3_regs.c Update counters to prefer sensor telemetry when available, otherwise query FW DPM level and map to frequency (FW table preferred, static table fallback).
drivers/accel/amdxdna/aie4_pci.h Extend device handle with cached FW DPM tables and add prototypes for new DPM query/init helpers.
drivers/accel/amdxdna/aie4_pci.c Initialize DPM frequency table cache during AIE4 device query and clarify clock metadata reporting.
drivers/accel/amdxdna/aie4_msg_priv.h Define new AIE4 message opcodes and request/response structs for DPM table/level queries.
drivers/accel/amdxdna/aie4_message.c Implement querying/caching of FW DPM frequency tables and querying current FW DPM levels.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread drivers/accel/amdxdna/aie4_message.c
Comment thread drivers/accel/amdxdna/npu3_regs.c Outdated
Comment thread drivers/accel/amdxdna/npu3_regs.c Outdated

aie->npuclk_freq = npu_metrics.mpnpuclk_freq;
aie->hclk_freq = npu_metrics.npuclk_freq;
if (ndev->dpm_aie_levels && ndev->dpm_npuh_levels) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donwalkarsoham this code... generated by AI?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @xdavidz,
As discussed, this is an initial copy for giving an idea of the changes and this needs further review, changes and testing. As suggested, using some AI tools for workflow.

@donwalkarsoham
donwalkarsoham force-pushed the xrt-smi-validate-dpm branch 2 times, most recently from c38174c to 78678a8 Compare July 1, 2026 23:30
@donwalkarsoham
donwalkarsoham requested a review from xdavidz July 1, 2026 23:49
Comment thread drivers/accel/amdxdna/aie4_pci.h Outdated
u32 total_col;
u32 max_dpm_level;

/* FW-provided DPM frequency tables (MHz); *_levels == 0 if unavailable */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a dpm_clk_tbl in amdxdna_dev_priv, but you add another in the amdxdna_dev_hdl.

I understand this is a cache of current dpm level, thus the other one in the priv is obsolated?

I think the logic is not.

We do have 2 dpm_clk_freq arrrays, one is hardcoded, one is dynamically quired from the fw. if dynamic query failed, then use the hardcode one.

you might consider adding it to the amdxdna_dev_priv for dpm_clk_tbl, and only update it when query is successful. So the logic is a little different than windows driver.

We always query from the dpm_clk_tbl, but we do update the dpm_clk_tbl once by querying the fw when aie4_pci init.

Do this make better sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok got it, makes sense. So, we query the FW table at aie4_pci initialization and update the dpm_clk_tbl. After this, we only query the dpm_clk_tbl during run time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok got it, makes sense. So, we query the FW table at aie4_pci initialization and update the dpm_clk_tbl. After this, we only query the dpm_clk_tbl during run time.

  1. We query dpm freq from FW during initializaioon and update the dpm_clk_tbl, if query is failed, use the default freq.
  2. We query the dpm_clk_tbl freq during run time.
    2.1) During the run time query, we also call update_dpm from pmf, if pmf is not ready, we skip updating the final freq.

In this way, we have good dpm_clk_tbl and also accurate freq when pmf is working. I think current code structure support this flow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that flow makes sense.
One implementation detail: Since priv->dpm_clk_tbl is const and shared across devices, we can't update it in place. We can leave it as the read-only default template and add a small writable array in the per-device amdxdna_dev_hdl. At init, we can copy the defaults into that per-device array, then overwrite it with the FW values if the query succeeds (if it fails, it just keeps the defaults). At runtime we will read frequencies from this per-device table if PMF is not available. That way priv stays immutable, each device has its own correct table, and there's a single table to index.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the changes.

I will review this today.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Comment thread drivers/accel/amdxdna/aie4_message.c
Comment thread drivers/accel/amdxdna/aie4_message.c Outdated
Comment on lines +151 to +158
n = min_t(u32, resp.aieclk_table.num_levels, resp.npuhclk_table.num_levels);
n = min_t(u32, n, AIE4_MAX_DPM_LEVEL_COUNT);
for (i = 0; i < n; i++) {
ndev->dpm_clk_tbl[i].npuclk = resp.aieclk_table.values[i];
ndev->dpm_clk_tbl[i].hclk = resp.npuhclk_table.values[i];
}

return 0;
Comment thread drivers/accel/amdxdna/aie4_message.c
Comment thread drivers/accel/amdxdna/aie4_pci.c Outdated
@donwalkarsoham
donwalkarsoham force-pushed the xrt-smi-validate-dpm branch from 98cd198 to 0504cfc Compare July 8, 2026 18:49
Comment thread drivers/accel/amdxdna/aie4_message.c
Comment thread drivers/accel/amdxdna/npu3_regs.c Outdated
Comment thread drivers/accel/amdxdna/npu3_regs.c Outdated
if (!ret) {
aie->npuclk_freq = npu_metrics.mpnpuclk_freq;
aie->hclk_freq = npu_metrics.npuclk_freq;
} else if (!aie4_query_dpm_level(ndev, &aieclk_level, &npuhclk_level)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this logic is right, let's sync up offline.

the logic here is:

  1. during fw boot (load fw), query dpm level and dpm freq
  2. each time query will call_get_sensors to get most accurate data
    2.0) if query success, use the pmf sensor data
    2.1) if the query failed, maybe not support this etc, use the dpm level data. Given the dpm level data is either hardcoded or set correctly by No.1 step, we don't need to redo the query, just get what ever date from the dpm level.

Thus, I think you don't have to use windows driver's example (if 0 then not query).

I think you should:

  1. still use the priv->dpm_level to update the dpm_table
  2. if dpm_query failed, warn but skip updating the dpm_table

That's it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@donwalkarsoham
donwalkarsoham force-pushed the xrt-smi-validate-dpm branch 3 times, most recently from 0ba087e to 42c0e80 Compare July 15, 2026 19:45
@donwalkarsoham
donwalkarsoham requested a review from xdavidz July 15, 2026 23:45
Signed-off-by: Soham Donwalkar <Soham.Donwalkar@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants