telemetry: Enhance HDA IO telemetry#10675
telemetry: Enhance HDA IO telemetry#10675wjablon1 wants to merge 3 commits intothesofproject:mainfrom
Conversation
wjablon1
requested review from
abonislawski,
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
pblaszko and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
This PR enhances I/O performance telemetry for HDA by improving counter instance identification on the DAI side and adding host-side byte counters, while also fixing a bitmap/lock handling leak in the telemetry slot allocator.
Changes:
- Use
dai_indexas the IO perf “instance” identifier for DAI-side counters (instead ofcpu_get_id()), and rename the DAI-side counter pointer for clarity. - Add host-side HDA IO perf byte counters (init in
params(), update on data copy, release on free) in both Zephyr and legacy host implementations. - Fix error-path handling in performance monitor slot allocation to avoid bitmap slot leaks and spinlock leaks.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/include/sof/lib/dai-zephyr.h | Renames DAI IO perf counter pointer member to reflect DAI-specific usage. |
| src/debug/telemetry/performance_monitor.c | Fixes allocation error paths to free bitmap slots and unlock correctly. |
| src/audio/host-zephyr.c | Adds host-side HDA IO byte counter init/update/release (Zephyr host). |
| src/audio/host-legacy.c | Adds host-side HDA IO byte counter init/update/release (legacy host). |
| src/audio/dai-zephyr.c | Switches IO perf instance ID to dai_index and updates renamed member usage. |
| src/audio/copier/host_copier.h | Adds host data pointer to IO perf slot (behind config) plus forward decl. |
| src/audio/copier/copier_host.c | Trivial formatting-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
LGTM, but pls resolve any copilot comments.
CPU ID is meant to be used as a measurement identifier only for IO measurements related to IPC/IDC whereas for measuring DAIs IO, a DMA index /DAI index shall be used instead. This commit contains this small correction as well as cosmetic changes related to the DAI IO measurement. Signed-off-by: Wojciech Jablonski <wojciech.jablonski@intel.com>
Currently SOF can collect IO performance data only for HAD Link interface. The corresponding counter on Host side is missing. This commit adds the missing counter with DMA index used as the counter instance Signed-off-by: Wojciech Jablonski <wojciech.jablonski@intel.com>
During allocation of a counter slot, spinlock isn't released in case of failure. This could lead to deadlock in case of failed allocation. This change fixes that by adding the missing release operation. Signed-off-by: Wojciech Jablonski <wojciech.jablonski@intel.com>
| params->direction, | ||
| IO_PERF_POWERED_UP_ENABLED, | ||
| IO_PERF_D0IX_POWER_MODE, | ||
| 0, 0, 0 |
There was a problem hiding this comment.
can we please use named initialisations like .id = IO_PERF_HDA_ID, etc.? These are too many to just rely on the order. Also no need to explicitly initialise to 0.
There was a problem hiding this comment.
I could do this, but all the occurrences of io_perf_data_item initialization follow the same pattern. If I mixed the two initialization methods, it would make reading (comparing the occurrences) even more complicated, especially because your proposal permits reordering. IMO we should either change all the occurrences or none.
abonislawski
left a comment
abonislawski
left a comment
There was a problem hiding this comment.
Interestingly, it's added even for host-legacy. It will probably never be used, but theoretically there is no such limitation in telemetry.
Some enhancements to HDA IO telemetry