fix(rdc): report N/A for values amd-smi cannot read

projects/rdc/CHANGELOG.md

@@ -27,6 +27,14 @@ Full documentation for RDC is available at [ROCm DataCenter Tool User Guide](htt
 - RDC no longer reports another GPU's XGMI link states, or garbage values that could crash `rdcd`, for a GPU whose link status AMD SMI cannot read. That GPU now reports no links.
   - `rdci link` now prints an error and exits with a nonzero status when it cannot get the link status, instead of printing nothing and exiting with 0.
 
+- `rdcd` no longer crashes reading `RDC_FI_GPU_CLOCK` or `RDC_FI_MEM_CLOCK` when AMD SMI reports no current clock level, as it does for a power-gated clock domain on some APUs. The field is N/A then.
+
+- Fields that AMD SMI cannot read are N/A instead of AMD SMI's "not available" value, such as 4294967295 for `RDC_FI_GPU_GFX_BUSY_INST` or 65535 for `RDC_FI_GPU_VCN_BUSY_INST` on Radeon GPUs. This covers the utilization and clock fields, the `RDC_FI_GPU_*_BUSY_INST` fields, and the `RDC_HEALTH_*` throttle and residency counters.
+
+- `RDC_FI_ECC_CORRECT_TOTAL`, `RDC_FI_ECC_UNCORRECT_TOTAL`, and `RDC_FI_ECC_DEFERRED_TOTAL` are N/A on GPUs without ECC, like the per-block ECC fields, instead of 0. The memory health check still checks such a GPU's retired pages.
+
+- `RDC_FI_GPU_MEMORY_CUR_BANDWIDTH` is N/A instead of 0 when the GPU reports no memory activity.
+
 - `RDC_FI_GPU_MEMORY_CUR_BANDWIDTH` no longer reports zero under DMA or copy-only memory traffic on GPUs whose instantaneous UMC activity does not reflect DMA transfers. It now derives memory activity from the `mem_activity_acc` accumulator over firmware time, and falls back to the instantaneous `umc_activity` reading when the accumulator or firmware timestamp is unavailable.
 
 - `rdci topo` and `rdc_device_topology_get()` now report the links of the requested GPU. They reported links between other GPU pairs, which showed the wrong link types on systems where GPU pairs are not all linked the same way, such as XGMI hives joined by PCIe.

projects/rdc/rdc_libs/rdc/src/RdcMetricFetcherImpl.cc

@@ -61,6 +61,14 @@ static const std::unordered_map<rdc_field_t, amdsmi_event_type_t> rdc_evnt_2_smi
     {RDC_EVNT_XGMI_5_THRPUT, AMDSMI_EVNT_XGMI_DATA_OUT_5},
 };
 
+// amd-smi reports a value it cannot read as the maximum of the field's type. Some uint32_t
+// fields hold the uint16_t maximum of the gpu_metrics field they are copied from, as older
+// amd-smi did for power. Only for values that stay far below 65535: percentages, W, MHz,
+// counts.
+static bool smi_not_available(uint32_t v) {
+  return v == std::numeric_limits<uint32_t>::max() || v == std::numeric_limits<uint16_t>::max();
+}
+
 RdcMetricFetcherImpl::RdcMetricFetcherImpl() : task_started_(true) {
   // kick off another thread for async fetch
   updater_ = std::async(std::launch::async, [this]() {
@@ -301,6 +309,7 @@ void RdcMetricFetcherImpl::get_ecc_total(uint32_t gpu_index, rdc_field_t field_i
   amdsmi_status_t err = AMDSMI_STATUS_SUCCESS;
   uint64_t correctable_count = 0;
   uint64_t uncorrectable_count = 0;
+  bool counted = false;  // a block reported its counts
   amdsmi_ras_err_state_t err_state = AMDSMI_RAS_ERR_STATE_INVALID;
 
   amdsmi_processor_handle processor_handle = nullptr;
@@ -324,10 +333,12 @@ void RdcMetricFetcherImpl::get_ecc_total(uint32_t gpu_index, rdc_field_t field_i
     if (err == AMDSMI_STATUS_SUCCESS) {
       correctable_count += ec.correctable_count;
       uncorrectable_count += ec.uncorrectable_count;
+      counted = true;
     }
   }
 
-  value->status = AMDSMI_STATUS_SUCCESS;
+  // Without a count from any block, as on GPUs without ECC, report N/A rather than 0 errors.
+  value->status = counted ? AMDSMI_STATUS_SUCCESS : AMDSMI_STATUS_NOT_SUPPORTED;
   value->type = INTEGER;
   if (field_id == RDC_FI_ECC_CORRECT_TOTAL) {
     value->value.l_int = correctable_count;
@@ -422,6 +433,7 @@ void RdcMetricFetcherImpl::get_ecc_deferred_total(uint32_t gpu_index, rdc_field_
   amdsmi_status_t err = get_processor_handle_from_id(gpu_index, &processor_handle);
 
   uint64_t deferred_count = 0;
+  bool counted = false;  // a block reported its counts
   // amdsmi_gpu_block_t is a 64-bit flag enum; a 32-bit counter wraps to 0 and never exits.
   for (uint64_t b = AMDSMI_GPU_BLOCK_FIRST; b <= AMDSMI_GPU_BLOCK_LAST; b = b * 2) {
     amdsmi_ras_err_state_t err_state = AMDSMI_RAS_ERR_STATE_INVALID;
@@ -433,10 +445,11 @@ void RdcMetricFetcherImpl::get_ecc_deferred_total(uint32_t gpu_index, rdc_field_
     err = amdsmi_get_gpu_ecc_count(processor_handle, static_cast<amdsmi_gpu_block_t>(b), &ec);
     if (err == AMDSMI_STATUS_SUCCESS) {
       deferred_count += ec.deferred_count;
+      counted = true;
     }
   }
 
-  value->status = AMDSMI_STATUS_SUCCESS;
+  value->status = counted ? AMDSMI_STATUS_SUCCESS : AMDSMI_STATUS_NOT_SUPPORTED;
   value->type = INTEGER;
   value->value.l_int = deferred_count;
 }
@@ -547,6 +560,10 @@ rdc_status_t RdcMetricFetcherImpl::bulk_fetch_smi_fields(
       RDC_FI_GPU_UTIL      // average_gfx_activity
   };
 
+  // gpu_metrics reports a value it does not have as UINT16_MAX. Such a field is left out
+  // here, as for a power of 0, and fetched one by one.
+  constexpr uint16_t kU16NotSupported = std::numeric_limits<uint16_t>::max();
+
   // To prevent always call the bulk API even if it is not supported,
   // the static is used to cache last try.
   static amdsmi_status_t rs = AMDSMI_STATUS_SUCCESS;
@@ -587,13 +604,16 @@ rdc_status_t RdcMetricFetcherImpl::bulk_fetch_smi_fields(
 
       switch (field_id) {
         case RDC_FI_GPU_CLOCK:  // current_gfxclk * 1000000
+          if (gpu_metrics.current_gfxclk == kU16NotSupported) continue;
           value.field_value.value.l_int =
               static_cast<int64_t>(gpu_metrics.current_gfxclk) * 1000000;
           break;
         case RDC_FI_MEMORY_TEMP:  // temperature_mem * 1000
+          if (gpu_metrics.temperature_mem == kU16NotSupported) continue;
           value.field_value.value.l_int = static_cast<int64_t>(gpu_metrics.temperature_mem) * 1000;
           break;
         case RDC_FI_GPU_TEMP:  // temperature_edge * 1000
+          if (gpu_metrics.temperature_edge == kU16NotSupported) continue;
           value.field_value.value.l_int = static_cast<int64_t>(gpu_metrics.temperature_edge) * 1000;
           break;
         case RDC_FI_POWER_USAGE:  // average_socket_power
@@ -617,6 +637,7 @@ rdc_status_t RdcMetricFetcherImpl::bulk_fetch_smi_fields(
           value.field_value.value.l_int *= 1000000;
           break;
         case RDC_FI_GPU_UTIL:  // average_gfx_activity
+          if (gpu_metrics.average_gfx_activity == kU16NotSupported) continue;
           value.field_value.value.l_int = static_cast<int64_t>(gpu_metrics.average_gfx_activity);
           break;
         default:
@@ -700,6 +721,11 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       return;
     }
 
+    // Map a narrower field's "not available" maximum to uint64_t's maximum.
+    auto widen = [](auto v) -> uint64_t {
+      using T = decltype(v);
+      return v == std::numeric_limits<T>::max() ? std::numeric_limits<uint64_t>::max() : v;
+    };
     const std::unordered_map<rdc_field_t, uint64_t> rdc_field_to_gpu_metrics = {
         {RDC_FI_XGMI_0_READ_KB, gpu_metrics.xgmi_read_data_acc[0]},
         {RDC_FI_XGMI_1_READ_KB, gpu_metrics.xgmi_read_data_acc[1]},
@@ -726,10 +752,9 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
         {RDC_FI_GFX_ACTIVITY_ACC, gpu_metrics.gfx_activity_acc},
         {RDC_FI_MEM_ACTIVITY_ACC, gpu_metrics.mem_activity_acc},
         {RDC_FI_ACCUMULATION_COUNTER, gpu_metrics.accumulation_counter},
-        {RDC_FI_GPU_GFX_BUSY_INST,
-         static_cast<uint64_t>(gpu_metrics.xcp_stats[0].gfx_busy_inst[0])},
-        {RDC_FI_GPU_VCN_BUSY_INST, static_cast<uint64_t>(gpu_metrics.xcp_stats[0].vcn_busy[0])},
-        {RDC_FI_GPU_JPEG_BUSY_INST, static_cast<uint64_t>(gpu_metrics.xcp_stats[0].jpeg_busy[0])},
+        {RDC_FI_GPU_GFX_BUSY_INST, widen(gpu_metrics.xcp_stats[0].gfx_busy_inst[0])},
+        {RDC_FI_GPU_VCN_BUSY_INST, widen(gpu_metrics.xcp_stats[0].vcn_busy[0])},
+        {RDC_FI_GPU_JPEG_BUSY_INST, widen(gpu_metrics.xcp_stats[0].jpeg_busy[0])},
     };
 
     // In gpu_metrics,the max value means not supported
@@ -772,7 +797,11 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       value->status = amdsmi_get_gpu_activity(processor_handle, &engine_usage);
       value->type = INTEGER;
       if (value->status == AMDSMI_STATUS_SUCCESS) {
-        value->value.l_int = static_cast<int64_t>(engine_usage.umc_activity);
+        if (smi_not_available(engine_usage.umc_activity)) {
+          value->status = AMDSMI_STATUS_NOT_SUPPORTED;
+        } else {
+          value->value.l_int = static_cast<int64_t>(engine_usage.umc_activity);
+        }
       }
       break;
     }
@@ -804,9 +833,11 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       // Instantaneous UMC controller activity. On some ASICs this only reflects
       // compute-shader memory traffic and reads zero under DMA/copy traffic.
       double activity_pct = 0.0;
+      bool have_activity = false;
       if (metrics_status == AMDSMI_STATUS_SUCCESS &&
           gpu_metrics.average_umc_activity != kU16NotSupported) {
         activity_pct = static_cast<double>(gpu_metrics.average_umc_activity);
+        have_activity = true;
       }
 
       // Prefer the memory-activity accumulator when available: unlike the
@@ -819,6 +850,7 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
         std::lock_guard<std::mutex> lock(mem_activity_mutex_);
         auto prev = mem_activity_cache_.find(gpu_index);
         const bool have_prev = prev != mem_activity_cache_.end();
+        have_activity = have_activity || have_prev;  // a first sample has no rate yet
         activity_pct = derive_mem_activity_percent(
             activity_pct, have_prev, have_prev ? prev->second.mem_activity_acc : 0,
             have_prev ? prev->second.firmware_timestamp : 0, gpu_metrics.mem_activity_acc,
@@ -841,6 +873,9 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
               : 0;
       // Don't mask a metrics-fetch failure with vram-info success.
       value->status = (metrics_status != AMDSMI_STATUS_SUCCESS) ? metrics_status : vram_status;
+      if (value->status == AMDSMI_STATUS_SUCCESS && !have_activity) {
+        value->status = AMDSMI_STATUS_NOT_SUPPORTED;
+      }
       break;
     }
     case RDC_FI_GPU_MEMORY_FREE: {
@@ -1007,7 +1042,12 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       value->status = amdsmi_get_clk_freq(processor_handle, clk_type, &f);
       value->type = INTEGER;
       if (value->status == AMDSMI_STATUS_SUCCESS) {
-        value->value.l_int = f.frequency[f.current];
+        // current is UINT32_MAX when no level is current, as for a power-gated clock domain.
+        if (f.current < f.num_supported && f.current < AMDSMI_MAX_NUM_FREQUENCIES) {
+          value->value.l_int = static_cast<int64_t>(f.frequency[f.current]);
+        } else {
+          value->status = AMDSMI_STATUS_NOT_SUPPORTED;
+        }
       }
       break;
     }
@@ -1034,7 +1074,11 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       value->status = amdsmi_get_clock_info(processor_handle, clk_type, &clk_info);
       value->type = INTEGER;
       if (value->status == AMDSMI_STATUS_SUCCESS) {
-        value->value.l_int = static_cast<int64_t>(clk_info.clk) * 1000000;
+        if (smi_not_available(clk_info.clk)) {
+          value->status = AMDSMI_STATUS_NOT_SUPPORTED;
+        } else {
+          value->value.l_int = static_cast<int64_t>(clk_info.clk) * 1000000;
+        }
       }
       break;
     }
@@ -1043,7 +1087,11 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       value->status = amdsmi_get_gpu_activity(processor_handle, &engine_usage);
       value->type = INTEGER;
       if (value->status == AMDSMI_STATUS_SUCCESS) {
-        value->value.l_int = static_cast<int64_t>(engine_usage.gfx_activity);
+        if (smi_not_available(engine_usage.gfx_activity)) {
+          value->status = AMDSMI_STATUS_NOT_SUPPORTED;
+        } else {
+          value->value.l_int = static_cast<int64_t>(engine_usage.gfx_activity);
+        }
       }
       break;
     }
@@ -1385,40 +1433,43 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
       value->status = Smi2RdcError(ret);
       value->type = INTEGER;
       if (value->status == AMDSMI_STATUS_SUCCESS) {
+        // amd-smi reports a counter the GPU does not provide as UINT64_MAX.
+        constexpr uint64_t kU64NotSupported = std::numeric_limits<uint64_t>::max();
+        uint64_t raw = kU64NotSupported;
         switch (field_id) {
           case RDC_HEALTH_POWER_THROTTLE_TIME:
-            value->value.l_int = static_cast<int64_t>(violation_status.acc_ppt_pwr);
+            raw = violation_status.acc_ppt_pwr;
             break;
           case RDC_HEALTH_THERMAL_THROTTLE_TIME:
           case RDC_HEALTH_SOCKET_THRM_ACC:  // alias — same underlying acc_socket_thrm counter
-            value->value.l_int = static_cast<int64_t>(violation_status.acc_socket_thrm);
+            raw = violation_status.acc_socket_thrm;
             break;
           case RDC_HEALTH_VIOLATION_COUNTER:
-            value->value.l_int = static_cast<int64_t>(violation_status.acc_counter);
+            raw = violation_status.acc_counter;
             break;
           case RDC_HEALTH_PROCHOT_RESIDENCY_ACC:
-            value->value.l_int = static_cast<int64_t>(violation_status.acc_prochot_thrm);
+            raw = violation_status.acc_prochot_thrm;
             break;
           case RDC_HEALTH_PROCHOT_RESIDENCY_PCT:
-            value->value.l_int = static_cast<int64_t>(violation_status.per_prochot_thrm);
+            raw = violation_status.per_prochot_thrm;
             break;
           case RDC_HEALTH_PPT_RESIDENCY_PCT:
-            value->value.l_int = static_cast<int64_t>(violation_status.per_ppt_pwr);
+            raw = violation_status.per_ppt_pwr;
             break;
           case RDC_HEALTH_SOCKET_THRM_PCT:
-            value->value.l_int = static_cast<int64_t>(violation_status.per_socket_thrm);
+            raw = violation_status.per_socket_thrm;
             break;
           case RDC_HEALTH_VR_THRM_ACC:
-            value->value.l_int = static_cast<int64_t>(violation_status.acc_vr_thrm);
+            raw = violation_status.acc_vr_thrm;
             break;
           case RDC_HEALTH_VR_THRM_PCT:
-            value->value.l_int = static_cast<int64_t>(violation_status.per_vr_thrm);
+            raw = violation_status.per_vr_thrm;
             break;
           case RDC_HEALTH_HBM_THRM_ACC:
-            value->value.l_int = static_cast<int64_t>(violation_status.acc_hbm_thrm);
+            raw = violation_status.acc_hbm_thrm;
             break;
           case RDC_HEALTH_HBM_THRM_PCT:
-            value->value.l_int = static_cast<int64_t>(violation_status.per_hbm_thrm);
+            raw = violation_status.per_hbm_thrm;
             break;
           case RDC_HEALTH_GFX_CLK_LMT_PWR_ACC:
           case RDC_HEALTH_GFX_CLK_LMT_PWR_PCT:
@@ -1428,9 +1479,7 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
           case RDC_HEALTH_LOW_UTIL_PCT:
           case RDC_HEALTH_GFX_CLK_LMT_TOTAL_ACC:
           case RDC_HEALTH_GFX_CLK_LMT_TOTAL_PCT: {
-            // Driver 1.8 XCP/XCC fields — max uint64 means not supported
-            const auto not_sup = std::numeric_limits<uint64_t>::max();
-            uint64_t raw = not_sup;
+            // Driver 1.8 XCP/XCC fields
             switch (field_id) {
               case RDC_HEALTH_GFX_CLK_LMT_PWR_ACC:
                 raw = violation_status.acc_gfx_clk_below_host_limit_pwr[0][0];
@@ -1459,16 +1508,16 @@ rdc_status_t RdcMetricFetcherImpl::fetch_gpu_field_(uint32_t gpu_index, rdc_fiel
               default:
                 break;
             }
-            if (raw == not_sup) {
-              value->status = AMDSMI_STATUS_NOT_SUPPORTED;
-            } else {
-              value->value.l_int = static_cast<int64_t>(raw);
-            }
             break;
           }
           default:
             break;
         }
+        if (raw == kU64NotSupported) {
+          value->status = AMDSMI_STATUS_NOT_SUPPORTED;
+        } else {
+          value->value.l_int = static_cast<int64_t>(raw);
+        }
       }
       break;
     }

projects/rdc/rdc_libs/rdc/src/RdcWatchTableImpl.cc

@@ -30,6 +30,7 @@ THE SOFTWARE.
 #include <sstream>
 #include <unordered_map>
 
+#include "amd_smi/amdsmi.h"
 #include "common/rdc_utils.h"
 #include "rdc/rdc.h"
 #include "rdc_lib/RdcLogger.h"
@@ -614,10 +615,12 @@ rdc_status_t RdcWatchTableImpl::memory_check(rdc_gpu_group_t group_id, uint32_t
   rdc_field_value start = {}, end = {};
   rdc_status_t result =
       get_start_end_values(group_id, gpu_index, RDC_FI_ECC_UNCORRECT_TOTAL, 0, nullptr, &end);
-  if (result != RDC_ST_OK) return result;
+  // A GPU without ECC has no count to check, but can still have retired pages. The
+  // fetcher returns RDC_ST_SMI_ERROR for every failure and keeps amd-smi's status in the value.
+  if (result != RDC_ST_OK && end.status != AMDSMI_STATUS_NOT_SUPPORTED) return result;
 
   uint64_t ecc_uncorrectable_count = 0;
-  ecc_uncorrectable_count = end.value.l_int;
+  if (result == RDC_ST_OK) ecc_uncorrectable_count = end.value.l_int;
   if (ecc_uncorrectable_count > 0) {
     rdc_health_incidents_t* incident = &response->incidents[response->incidents_count];