fix(amdsmi): report APU utilization as whole percent

projects/amdsmi/CHANGELOG.md

@@ -193,7 +193,13 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr
 ### Resolved Issues
 
 - **Fixed GPU metrics returning no data on integrated GPUs**.  
-  - `amdsmi_get_gpu_metrics_info()` answered `AMDSMI_STATUS_UNEXPECTED_DATA` on APUs such as gfx1103, which also left PLX temperature and the `amd-smi metric` output empty. These devices now return the values they provide with everything else marked N/A, and a metrics version amd-smi cannot read reports `AMDSMI_STATUS_NOT_SUPPORTED`.
+  - `amdsmi_get_gpu_metrics_info()` failed on APUs such as gfx1103, leaving PLX temperature and `amd-smi metric` empty. Those devices now report the values they provide with the rest marked N/A, and any GPU without a PLX sensor now reports N/A there instead of a placeholder reading.
+
+- **Fixed GPU and multimedia utilization reading 100 times too high on integrated GPUs**.  
+  - `amdsmi_get_gpu_activity()` and `amd-smi metric --usage` passed on the hundredths of a percent the APU firmware reports as if it were whole percent, so a GPU at 45% busy read as 4567. Discrete GPUs were not affected.
+
+- **Fixed per-metric queries reporting an unreadable metrics version as bad data**.  
+  - A device whose metrics revision amd-smi cannot read now reports it as unsupported from every metrics query, matching what the whole-table query already reported.
 
 - **Fixed `amd-smi` printing a Python traceback when an unknown NIC or switch is selected**.  
   - `amd-smi static --nic 999` and `--switch 999` failed while building the "device not found" error, so the command exited `1` with a traceback and no readable message. `--json` and `--csv` produced no parseable output.

projects/amdsmi/include/amd_smi/amdsmi.h

@@ -2259,6 +2259,14 @@ typedef struct {
 
   /**
    * @brief Utilization
+   *
+   * On a v2.x table the GFX and MM activity below are in hundredths of a percent,
+   * the unit the APU firmware reports, and are not converted here -- the same
+   * arrangement as the power fields further down. The whole-percent values are in
+   * ::amdsmi_gpu_metrics_t.average_gfx_activity and
+   * ::amdsmi_gpu_metrics_t.average_mm_activity. A v3.0 table reports GFX
+   * activity in whole percent to begin with, so both forms match there; it
+   * carries no MM activity, which reads back as not applicable.
    */
   uint16_t average_gfx_activity;                            //!< v2_4, v3_0
   uint16_t average_mm_activity;                             //!< v2_4
@@ -5136,11 +5144,24 @@ amdsmi_status_t amdsmi_get_gpu_metrics_header_info(amdsmi_processor_handle proce
  *  including a GPU metrics table (v1.x) reported by an APU: the pointer
  *  tracks the table the device publishes, not whether the device is an APU.
  *
- *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on
- *  fail. Returns ::AMDSMI_STATUS_NOT_SUPPORTED when the device reports a
- *  metrics revision this library does not model, and
- *  ::AMDSMI_STATUS_UNEXPECTED_SIZE when the table it hands back does not match
- *  the size its header declares.
+ *  The APU sub-struct reports GFX and MM activity in the raw unit its firmware
+ *  uses, as its power fields stay in milliwatts: hundredths of a percent on a
+ *  v2.x table, whole percent on v3.0. @p pgpu_metrics->average_gfx_activity and
+ *  @p pgpu_metrics->average_mm_activity are whole percent for every revision.
+ *
+ *  A field the reported revision does not carry reads back as the not-applicable
+ *  sentinel, the maximum value of its type. The sentinel is never scaled, so a
+ *  field marked not applicable stays recognizable as one at both levels.
+ *
+ *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail.
+ *  @retval ::AMDSMI_STATUS_NOT_SUPPORTED the device reports a metrics revision
+ *  this library does not model, or publishes an empty metrics node.
+ *  @retval ::AMDSMI_STATUS_BUSY the device is resetting or suspended, so the
+ *  driver declined the read.
+ *  @retval ::AMDSMI_STATUS_NO_PERM the metrics node cannot be read with the
+ *  caller's privileges.
+ *  @retval ::AMDSMI_STATUS_UNEXPECTED_SIZE the read came up short of the size the
+ *  header declares, or the driver failed it without naming a device state.
  */
 amdsmi_status_t amdsmi_get_gpu_metrics_info(amdsmi_processor_handle processor_handle,
                                             amdsmi_gpu_metrics_t* pgpu_metrics);
@@ -5165,22 +5186,22 @@ amdsmi_status_t amdsmi_get_gpu_metrics_info(amdsmi_processor_handle processor_ha
  *  provided arguments.
  *
  *  **APU Metrics:**
- *  When APU-specific metrics are available (APU metrics table v2.1-v2.4 or
- *  v3.0; v2.0 is not supported),
- *  @p pgpu_metrics->apu_metrics will point to thread-local library-owned storage.
- *  This pointer is invalidated by ANY subsequent call to
- *  ::amdsmi_get_gpu_metrics_info or ::amdsmi_get_gpu_partition_metrics_info on
- *  the same thread, even for different devices. Callers must copy the entire
- *  ::amdsmi_apu_metrics_t structure immediately after the call to preserve the data.
- *  Every other table leaves @p pgpu_metrics->apu_metrics as nullptr,
- *  including a GPU metrics table (v1.x) reported by an APU: the pointer
- *  tracks the table the device publishes, not whether the device is an APU.
+ *  A partition metrics table never carries them. This call resolves only the XCP
+ *  partition revisions, so @p pgpu_metrics->apu_metrics is always nullptr here;
+ *  use ::amdsmi_get_gpu_metrics_info for a device that publishes an APU table.
+ *  This call does still invalidate an ::amdsmi_apu_metrics_t pointer obtained
+ *  earlier from that function on the same thread, so copy what it returned before
+ *  calling this one.
  *
- *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on
- *  fail. Returns ::AMDSMI_STATUS_NOT_SUPPORTED when the device reports a
- *  metrics revision this library does not model, and
- *  ::AMDSMI_STATUS_UNEXPECTED_SIZE when the table it hands back does not match
- *  the size its header declares.
+ *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail.
+ *  @retval ::AMDSMI_STATUS_NOT_SUPPORTED the device reports a metrics revision
+ *  this library does not model, or publishes an empty metrics node.
+ *  @retval ::AMDSMI_STATUS_BUSY the device is resetting or suspended, so the
+ *  driver declined the read.
+ *  @retval ::AMDSMI_STATUS_NO_PERM the metrics node cannot be read with the
+ *  caller's privileges.
+ *  @retval ::AMDSMI_STATUS_UNEXPECTED_SIZE the read came up short of the size the
+ *  header declares, or the driver failed it without naming a device state.
  */
 amdsmi_status_t amdsmi_get_gpu_partition_metrics_info(amdsmi_processor_handle processor_handle,
                                                       amdsmi_gpu_metrics_t* pgpu_metrics);

projects/amdsmi/rocm_smi/src/rocm_smi_device.cc

@@ -1197,9 +1197,28 @@ int Device::readDevInfoBinary(DevInfoTypes type, std::size_t b_size, void* p_bin
     auto result = EIO;
     const auto* cause = "the node came up short of the size it declared";
     if (read_failed) {
-      // Suspended or resetting answers EBUSY, runtime-inactive EPERM: pass
-      // it on, so each keeps reaching the caller as the state it is.
-      result = (read_errno != 0) ? read_errno : EIO;
+      // Pass on only the errnos that name a device state the caller can act on:
+      // suspended or resetting answers EBUSY, runtime-inactive EPERM, a real
+      // access problem EACCES, and a signal EINTR. Every other errno would be
+      // re-mapped into a status that misdescribes the failure -- EINVAL becomes
+      // INVALID_ARGS and blames the caller for a fault on the driver's side --
+      // so the rest are reported as the plain read failure they are.
+      switch (read_errno) {
+        // EOPNOTSUPP is what the SMU layer answers when the firmware does not
+        // know the metrics command at all (SMU_RESP_CMD_UNKNOWN), so it is the
+        // one errno here that genuinely means unsupported. Listing ENOTSUP
+        // beside it would not compile: glibc gives both the same value.
+        case EOPNOTSUPP:
+        case EBUSY:
+        case EPERM:
+        case EACCES:
+        case EINTR:
+          result = read_errno;
+          break;
+        default:
+          result = EIO;
+          break;
+      }
       cause = "the driver refused the read";
     } else if (bytes_read == 0) {
       // A driver with no metrics callback publishes an empty node: nothing

projects/amdsmi/rocm_smi/src/rocm_smi_gpu_metrics.cc

@@ -350,25 +350,27 @@ const AMDGpuMetricsUnitTypeTranslationTbl_t amdgpu_metrics_unit_type_translation
 // is_gpu_metrics_version_supported(), so the version a caller is told about and
 // the object built for it always come from the same decision.
 //
-// Resolve a metrics header to its version flag without logging. Minor revisions
-// are byte-prefix supersets of the newest struct we model, so an APU exposing
-// gpu_metrics v2.1-v2.3 is read with the v2.4 layout (its extra trailing fields
-// report as not-applicable). v2.0 is excluded: it stores system_clock_counter
-// right after the header instead of after the temperature/utilization block, so
-// it is not a byte-prefix of v2.1+ and is left unmodeled. Returns kGpuMetricNone
-// when the version cannot be modeled.
+// Resolve a metrics header to its version flag without logging.
+//
 // Format revisions and the content-revision bounds each one covers. The GPU and
-// partition families switch to a kernel-described layout from a given revision
-// on, and the APU family is modeled by one struct whose older revisions are
-// byte-prefixes of it; v2.0 is excluded because it orders its fields differently.
+// partition families switch to a kernel-described layout from a given revision on.
+// The APU family is modeled by the single v2.4 struct, and the older revisions are
+// byte prefixes of it: v2.1-v2.3 stop short of its tail, so an APU exposing one of
+// them is read with the v2.4 layout and the trailing fields its own revision does
+// not carry report as not-applicable. v2.0 is excluded: it stores
+// system_clock_counter right after the header instead of after the
+// temperature/utilization block, so it is not a prefix of v2.1+ and is left
+// unmodeled. v3.0 is a separate layout rather than a prefix of v2.4, so it is
+// matched by the explicit table above and read through its own struct. Returns
+// kGpuMetricNone when the version cannot be modeled.
 constexpr auto kGpuMetricsFormatRevision = std::uint8_t(1);
 constexpr auto kApuMetricsFormatRevision = std::uint8_t(2);
 constexpr auto kGpuMetricsFirstDynamicContentRevision = std::uint8_t(9);
 constexpr auto kPartitionMetricsFirstDynamicContentRevision = std::uint8_t(2);
 constexpr auto kApuMetricsFirstPrefixContentRevision = std::uint8_t(1);
 constexpr auto kApuMetricsLastPrefixContentRevision = std::uint8_t(4);
 
-AMDGpuMetricVersionFlags_t lookup_header_flag_version(
+static AMDGpuMetricVersionFlags_t lookup_header_flag_version(
     const AMDGpuMetricsHeader_v1_t& metrics_header, bool is_partition_metrics) {
   const auto flag_version = join_metrics_version(metrics_header);
   if (!is_partition_metrics) {
@@ -2779,6 +2781,20 @@ AMGpuMetricsPublicLatestTupl_t ApuMetricsBase_v30_t::copy_internal_to_external_m
     }
     return static_cast<PublicPowerT>(std::min<uint32_t>(rounded_watts, max_power));
   };
+  // Every v2.x table reports GFX/MM activity in hundredths of a percent: the SMU headers
+  // mark GfxActivity/UvdActivity "[centi]" for each ASIC that publishes one (smu_v13_0_4,
+  // smu_v13_0_5 and yellow_carp for v2.1, renoir and vangogh for v2.2-v2.4), and amdgpu
+  // stores it unscaled -- it only divides by 100 on the gpu_busy_percent path. The public
+  // field is a whole percent, so convert here. v3.0 firmware already reports [0-100], so
+  // the branch below leaves it alone.
+  auto convert_apu_activity_centi_to_public_percent = [](auto activity_centi) {
+    using InputT = decltype(activity_centi);
+    // The not-applicable sentinel is not a reading; scaling it would turn it into 655.
+    if (activity_centi == std::numeric_limits<InputT>::max()) {
+      return activity_centi;
+    }
+    return static_cast<InputT>((activity_centi + 50) / 100);
+  };
 
   if (get_gpu_metrics_version_used() == AMDGpuMetricVersionFlags_t::kApuMetricV24) {
     const auto& metrics = m_apu_metrics_v24_tbl;
@@ -2795,6 +2811,8 @@ AMGpuMetricsPublicLatestTupl_t ApuMetricsBase_v30_t::copy_internal_to_external_m
                 std::min(static_cast<uint32_t>(RSMI_APU_MAX_L3),
                          static_cast<uint32_t>(std::size(apu.temperature_l3))),
                 apu.temperature_l3);
+    // Raw firmware units here, as with the power fields: hundredths of a percent. The
+    // whole-percent conversion belongs to the top-level fields assigned further down.
     apu.average_gfx_activity = metrics.m_average_gfx_activity;
     apu.average_mm_activity = metrics.m_average_mm_activity;
     apu.average_socket_power = metrics.m_average_socket_power;
@@ -2845,8 +2863,10 @@ AMGpuMetricsPublicLatestTupl_t ApuMetricsBase_v30_t::copy_internal_to_external_m
     apu.average_soc_current = metrics.m_average_soc_current;
     apu.average_gfx_current = metrics.m_average_gfx_current;
 
-    metrics_public_init.average_gfx_activity = metrics.m_average_gfx_activity;
-    metrics_public_init.average_mm_activity = metrics.m_average_mm_activity;
+    metrics_public_init.average_gfx_activity =
+        convert_apu_activity_centi_to_public_percent(metrics.m_average_gfx_activity);
+    metrics_public_init.average_mm_activity =
+        convert_apu_activity_centi_to_public_percent(metrics.m_average_mm_activity);
     metrics_public_init.average_socket_power =
         convert_apu_power_mw_to_public_w(metrics.m_average_socket_power);
     metrics_public_init.system_clock_counter = metrics.m_system_clock_counter;
@@ -5092,7 +5112,7 @@ auto Device::dev_read_gpu_metrics_all_data(DevInfoTypes type) -> rsmi_status_t {
     }
 
   } else {
-    // Metric tables are versioned supersets: a device may report an older,
+    // Metric tables grow by appending, so a device may report an older,
     // shorter revision than the newest struct we model (e.g. an APU exposing
     // gpu_metrics v2.1 into the v2.4 layout). Pre-fill the destination with the
     // not-applicable sentinel and never read past our own buffer, so trailing
@@ -5476,9 +5496,10 @@ auto Device::dev_copy_internal_to_external_metrics(DevInfoTypes type)
   std::string gpu_metrics_path = get_sys_file_path_by_type(type, true);
 
   if (!m_gpu_metrics_ptr) {
-    // Callers stop on the NOT_SUPPORTED reported while reading the header, so a
-    // missing object here means a recognized version produced no table -- a
-    // genuine data inconsistency rather than an unsupported revision.
+    // Setup is what reports an unsupported revision; reaching this point without an
+    // object means a version this library does model produced no table, which is a
+    // data inconsistency. The status is therefore fixed, so log it as an error
+    // rather than routing it through the by-status helper.
     status_code = rsmi_status_t::RSMI_STATUS_UNEXPECTED_DATA;
     ss << __PRETTY_FUNCTION__ << " | ======= end ======= "
        << " | Fail "
@@ -5488,7 +5509,7 @@ auto Device::dev_copy_internal_to_external_metrics(DevInfoTypes type)
                                   gpu_metrics_path)
        << " | Cause: Couldn't get a valid metric object"
        << " | Returning = " << getRSMIStatusString(status_code, false) << " |";
-    log_by_metrics_status(ss, status_code);
+    LOG_ERROR(ss);
     return std::make_tuple(status_code, AMGpuMetricsPublicLatest_t());
   }
 
@@ -5520,7 +5541,13 @@ auto Device::run_internal_gpu_metrics_query(AMDGpuMetricsUnitType_t metric_count
   //            /sys/class/drm/renderDXXX/device/xcp/xcp_metrics
   status_code = setup_gpu_metrics_reading(type);
   if ((status_code != rsmi_status_t::RSMI_STATUS_SUCCESS) || (!m_gpu_metrics_ptr)) {
-    status_code = rsmi_status_t::RSMI_STATUS_UNEXPECTED_DATA;
+    // Keep the reason setup gave: a revision this library cannot model is
+    // NOT_SUPPORTED, and collapsing it here would contradict what the whole-table
+    // entry point reports for the same device. Only a setup that reported success
+    // yet left no object behind is a data inconsistency.
+    if (status_code == rsmi_status_t::RSMI_STATUS_SUCCESS) {
+      status_code = rsmi_status_t::RSMI_STATUS_UNEXPECTED_DATA;
+    }
     ss << __PRETTY_FUNCTION__ << " | ======= end ======= "
        << " | Fail "
        << " | Device #: " << index() << " | Type: " << Device::get_type_string(type)
@@ -5732,6 +5759,20 @@ rsmi_status_t log_metrics_setup_failure(const char* caller, uint32_t dv_ind, Dev
  */
 // log current gpu_metrics file content read
 // any metrics value can be a nullptr
+// The copy stage hands back a pointer into the device's own metrics object, which a
+// concurrent call on the same device can replace while this caller is still reading
+// it -- the device lock is released as soon as this function returns. Re-point the
+// caller at a thread-local copy taken while that lock is still held, which is also
+// the lifetime the public headers promise for this pointer.
+static void rebind_apu_metrics_to_thread_local(rsmi_gpu_metrics_t* metrics) {
+  if (metrics == nullptr || metrics->apu_metrics == nullptr) {
+    return;
+  }
+  thread_local rsmi_apu_metrics_t apu_metrics_copy{};
+  apu_metrics_copy = *metrics->apu_metrics;
+  metrics->apu_metrics = &apu_metrics_copy;
+}
+
 rsmi_status_t rsmi_dev_gpu_metrics_info_get(uint32_t dv_ind, rsmi_gpu_metrics_t* smu) {
   TRY DEVICE_MUTEX CHK_SUPPORT_NAME_ONLY(smu)
 
@@ -5796,6 +5837,7 @@ rsmi_status_t rsmi_dev_gpu_metrics_info_get(uint32_t dv_ind, rsmi_gpu_metrics_t*
   }
 
   *smu = external_metrics;
+  rebind_apu_metrics_to_thread_local(smu);
   ss << __PRETTY_FUNCTION__ << " | ======= end ======= "
      << " | Success "
      << " | Device #: " << dv_ind << " | Type: " << Device::get_type_string(type)
@@ -5875,6 +5917,7 @@ rsmi_status_t rsmi_dev_gpu_partition_metrics_info_get(uint32_t dv_ind, rsmi_gpu_
   }
 
   *smu = external_metrics;
+  rebind_apu_metrics_to_thread_local(smu);
   ss << __PRETTY_FUNCTION__ << " | ======= end ======= "
      << " | Success "
      << " | Device #: " << dv_ind << " | Type: " << Device::get_type_string(type)

projects/amdsmi/src/amd_smi/amd_smi.cc

@@ -4098,8 +4098,7 @@ amdsmi_status_t amdsmi_get_gpu_accelerator_partition_profile_config(
        << "\n | rsmi_dev_compute_partition_xcp_config_set(" << partition_type_str
        << ") Returning: " << smi_amdgpu_get_status_string(status, false)
        << "\n | Type: " << amd::smi::Device::get_type_string(amd::smi::kDevSupportedXcpConfigs)
-       << "\n | Data: "
-       << "N/A";
+       << "\n | Data: " << "N/A";
     // std::cout << ss.str() << std::endl;
     LOG_DEBUG(ss);
 
@@ -6484,9 +6483,8 @@ amdsmi_status_t amdsmi_get_processor_handle_from_bdf(amdsmi_bdf_t bdf,
     return status;
   }
   std::ostringstream bdf_sstream;
-  bdf_sstream << __PRETTY_FUNCTION__ << " | [bdf] domain_number:"
-              << "bus_number:"
-              << "device_number."
+  bdf_sstream << __PRETTY_FUNCTION__
+              << " | [bdf] domain_number:" << "bus_number:" << "device_number."
               << "function_number = ";
   bdf_sstream << std::hex << std::setfill('0') << std::setw(4) << bdf.domain_number << ":";
   bdf_sstream << std::hex << std::setfill('0') << std::setw(2) << bdf.bus_number << ":";
@@ -6514,9 +6512,8 @@ amdsmi_status_t amdsmi_get_processor_handle_from_bdf(amdsmi_bdf_t bdf,
         return status;
       }
       amdsmi_bdf_t found_bdf = gpu_device->get_bdf();
-      bdf_sstream << __PRETTY_FUNCTION__ << " | [found_bdf] domain_number:"
-                  << "bus_number:"
-                  << "device_number."
+      bdf_sstream << __PRETTY_FUNCTION__
+                  << " | [found_bdf] domain_number:" << "bus_number:" << "device_number."
                   << "function_number = ";
       bdf_sstream << std::hex << std::setfill('0') << std::setw(4) << found_bdf.domain_number
                   << ":";