fix(amdsmi): stop amdsmi_set_clk_freq() from falsely returning INVAL

projects/amdsmi/CHANGELOG.md

@@ -263,6 +263,9 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr
   - The WSL backend returned success with a zeroed structure, so `rev_id` read as `0x0`, and where it did report the not-supported value Python rendered it as the raw `0xffffffff`. Python and the CLI now render it as `N/A`.
   - `amdsmi_asic_info_t` is now reset through one shared initializer used by every backend, so a field a backend cannot supply keeps its not-supported value rather than a plausible zero.
 
+- **Fixed `amdsmi_set_clk_freq()` falsely returning `AMDSMI_STATUS_INVAL` on Navi48 (gfx1201)**.  
+  - The deep-sleep bitmask validation added previously rejected the entire request when any requested bit fell outside a settable range computed from a prior read, so a supported clock set failed on gfx1201 even though the write is valid. The set is now lenient: out-of-range bits are dropped, and `AMDSMI_STATUS_INVAL` is only returned when no requested level is settable. The read-only check runs after the write, and `TestFrequenciesReadWrite` is re-enabled on Aldebaran (gfx90a / MI210).
+
 ### Upcoming Changes
 
 - **UUIDs will be replaced by CUIDs in an upcoming version**.  

projects/amdsmi/include/amd_smi/amdsmi.h

@@ -4089,7 +4089,13 @@ amdsmi_status_t amdsmi_get_gpu_pci_replay_counter(amdsmi_processor_handle proces
  *  ::amdsmi_frequencies_t::num_supported (of ::amdsmi_pcie_bandwidth_t) bits of
  *  this mask are relevant.
  *
- *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail
+ *  @retval ::AMDSMI_STATUS_SUCCESS call was successful
+ *  @retval ::AMDSMI_STATUS_NOT_SUPPORTED installed software or hardware does not
+ *  support this function with the given arguments
+ *  @retval ::AMDSMI_STATUS_NO_PERM function requires root access
+ *  @retval ::AMDSMI_STATUS_INVAL @p bw_bitmask selects no settable bandwidth
+ *  @retval ::AMDSMI_STATUS_UNEXPECTED_DATA the device reports zero settable
+ *  bandwidths
  */
 amdsmi_status_t amdsmi_set_gpu_pci_bandwidth(amdsmi_processor_handle processor_handle,
                                              uint64_t bw_bitmask);
@@ -5457,33 +5463,46 @@ amdsmi_status_t amdsmi_set_gpu_overdrive_level(amdsmi_processor_handle processor
  *
  *  @platform{gpu_bm_linux}
  *
- *  @details Given a processor handle @p processor_handle, a clock type @p clk_type, and a
- *  64 bit bitmask @p freq_bitmask, this function will limit the set of
- *  allowable frequencies. If a bit in @p freq_bitmask has a value of 1, then
- *  the frequency (as ordered in an ::amdsmi_frequencies_t returned by
- *  amdsmi_get_clk_freq()) corresponding to that bit index will be
- *  allowed.
- *
- *  This function will change the performance level to
- *  ::AMDSMI_DEV_PERF_LEVEL_MANUAL in order to modify the set of allowable
- *  frequencies. Caller will need to set to ::AMDSMI_DEV_PERF_LEVEL_AUTO in order
- *  to get back to default state.
- *
- *  All bits with indices greater than or equal to
- *  ::amdsmi_frequencies_t::num_supported will be ignored.
+ *  @details Given a processor handle @p processor_handle, this
+ *  function restricts clock @p clk_type to the frequencies
+ *  selected in @p freq_bitmask. Bit N maps to DPM level N in the
+ *  order returned by amdsmi_get_clk_freq(). Set a bit to 1 to
+ *  enable and 0 to disable that level.
+ *
+ *  Only settable DPM levels can be enabled. Bits outside the settable range are
+ *  dropped before the request is sent, so an out-of-range level is never
+ *  requested; the remaining in-range bits are still applied. The deep-sleep
+ *  frequency (see ::amdsmi_frequencies_t::has_deep_sleep)
+ *  is not settable, so when it is present the settable level count is
+ *  ::amdsmi_frequencies_t::num_supported minus one. Without a deep-sleep
+ *  frequency, all ::amdsmi_frequencies_t::num_supported levels (0 to
+ *  num_supported - 1) are settable. The call returns ::AMDSMI_STATUS_INVAL if
+ *  @p freq_bitmask selects no settable level, or ::AMDSMI_STATUS_UNEXPECTED_DATA
+ *  if the device reports zero settable levels for @p clk_type.
+ *
+ *  This call sets the performance level to ::AMDSMI_DEV_PERF_LEVEL_MANUAL. Set it
+ *  back to ::AMDSMI_DEV_PERF_LEVEL_AUTO to restore the default behavior.
  *
  *  @note This function requires admin/sudo privileges
  *
  *  @param[in] processor_handle a processor handle
  *
- *  @param[in] clk_type the type of clock for which the set of frequencies
- *  will be modified
+ *  @param[in] clk_type the type of clock to modify
  *
- *  @param[in] freq_bitmask A bitmask indicating the indices of the
- *  frequencies that are to be enabled (1) and disabled (0). Only the lowest
- *  ::amdsmi_frequencies_t.num_supported bits of this mask are relevant.
+ *  @param[in] freq_bitmask Bitmask selecting which DPM levels to allow, where
+ *  bit N enables (1) or disables (0) DPM level N. Bits above the highest settable
+ *  level are ignored. See notes above on settable levels.
  *
- *  @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail
+ *  @retval ::AMDSMI_STATUS_SUCCESS call was successful
+ *  @retval ::AMDSMI_STATUS_NOT_SUPPORTED installed software or hardware does not
+ *  support this function with the given arguments
+ *  @retval ::AMDSMI_STATUS_NO_PERM function requires root access
+ *  @retval ::AMDSMI_STATUS_INVAL @p clk_type is not a valid ::amdsmi_clk_type_t,
+ *  or @p freq_bitmask selects no settable DPM level
+ *  @retval ::AMDSMI_STATUS_UNEXPECTED_SIZE the device reports more supported
+ *  frequencies than AMDSMI_MAX_NUM_FREQUENCIES
+ *  @retval ::AMDSMI_STATUS_UNEXPECTED_DATA the device reports zero settable DPM
+ *  levels for @p clk_type
  */
 amdsmi_status_t amdsmi_set_clk_freq(amdsmi_processor_handle processor_handle,
                                     amdsmi_clk_type_t clk_type, uint64_t freq_bitmask);

projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h

@@ -2651,6 +2651,9 @@ rsmi_status_t rsmi_dev_pci_replay_counter_get(uint32_t dv_ind, uint64_t* counter
  *
  *  @retval ::RSMI_STATUS_SUCCESS call was successful
  *  @retval ::RSMI_STATUS_PERMISSION function requires root access
+ *  @retval ::RSMI_STATUS_INVALID_ARGS @p bw_bitmask selects no settable bandwidth
+ *  @retval ::RSMI_STATUS_UNEXPECTED_DATA the device reports zero settable
+ *  bandwidths
  *
  */
 rsmi_status_t rsmi_dev_pci_bandwidth_set(uint32_t dv_ind, uint64_t bw_bitmask);
@@ -4038,34 +4041,43 @@ rsmi_status_t rsmi_dev_overdrive_level_set_v1(uint32_t dv_ind, uint32_t od);
  * @brief Control the set of allowed frequencies that can be used for the
  * specified clock.
  *
- * @details Given a device index @p dv_ind, a clock type @p clk_type, and a
- * 64 bit bitmask @p freq_bitmask, this function will limit the set of
- * allowable frequencies. If a bit in @p freq_bitmask has a value of 1, then
- * the frequency (as ordered in an ::rsmi_frequencies_t returned by
- * rsmi_dev_gpu_clk_freq_get()) corresponding to that bit index will be
- * allowed.
- *
- * This function will change the performance level to
- * ::RSMI_DEV_PERF_LEVEL_MANUAL in order to modify the set of allowable
- * frequencies. Caller will need to set to ::RSMI_DEV_PERF_LEVEL_AUTO in order
- * to get back to default state.
- *
- * All bits with indices greater than or equal to
- * ::rsmi_frequencies_t::num_supported will be ignored.
+ * @details Given a device index @p dv_ind, this function will restrict clock
+ * @p clk_type to the frequencies selected in @p freq_bitmask. Bit N maps to
+ * DPM level N in the order returned by rsmi_dev_gpu_clk_freq_get(). Set a bit
+ * to 1 to enable and 0 to disable that level.
+ *
+ * Only settable DPM levels can be enabled. Bits outside the settable range are
+ * dropped before the request is sent, so an out-of-range level is never
+ * requested; the remaining in-range bits are still applied. The deep-sleep
+ * frequency (see ::rsmi_frequencies_t::has_deep_sleep)
+ * is not settable, so when it is present the settable level count is
+ * ::rsmi_frequencies_t::num_supported minus one. Without a deep-sleep
+ * frequency, all ::rsmi_frequencies_t::num_supported levels (0 to
+ * num_supported - 1) are settable. The call returns ::RSMI_STATUS_INVALID_ARGS
+ * if @p freq_bitmask selects no settable level, or ::RSMI_STATUS_UNEXPECTED_DATA
+ * if the device reports zero settable levels for @p clk_type.
+ *
+ * This call sets the performance level to ::RSMI_DEV_PERF_LEVEL_MANUAL. Set it
+ * back to ::RSMI_DEV_PERF_LEVEL_AUTO to restore the default behavior.
  *
  *  @param[in] dv_ind a device index
  *
- *  @param[in] clk_type the type of clock for which the set of frequencies
- *  will be modified
+ *  @param[in] clk_type the type of clock to modify
  *
- *  @param[in] freq_bitmask A bitmask indicating the indices of the
- *  frequencies that are to be enabled (1) and disabled (0). Only the lowest
- *  ::rsmi_frequencies_t.num_supported bits of this mask are relevant.
+ *  @param[in] freq_bitmask Bitmask selecting which DPM levels to allow, where
+ *  bit N enables (1) or disables (0) DPM level N. Bits above the highest settable
+ *  level are ignored. See notes above on settable levels.
  *
  *  @retval ::RSMI_STATUS_SUCCESS is returned upon successful call.
  *  @retval ::RSMI_STATUS_NOT_SUPPORTED installed software or hardware does not
  *  support this function with the given arguments
  *  @retval ::RSMI_STATUS_PERMISSION function requires root access
+ *  @retval ::RSMI_STATUS_INVALID_ARGS @p clk_type is not a valid ::rsmi_clk_type_t,
+ *  or @p freq_bitmask selects no settable DPM level
+ *  @retval ::RSMI_STATUS_UNEXPECTED_SIZE the device reports more supported
+ *  frequencies than RSMI_MAX_NUM_FREQUENCIES
+ *  @retval ::RSMI_STATUS_UNEXPECTED_DATA the device reports zero settable DPM
+ *  levels for @p clk_type
  *
  */
 rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_type,

projects/amdsmi/rocm_smi/src/rocm_smi.cc

@@ -2085,20 +2085,36 @@ rsmi_status_t rsmi_dev_firmware_version_get(uint32_t dv_ind, rsmi_fw_block_t blo
   CATCH
 }
 
-static std::string bitfield_to_freq_string(uint64_t bitf, uint32_t num_supported) {
-  std::string bf_str;
-  std::bitset<RSMI_MAX_NUM_FREQUENCIES> bs(bitf);
-
+// Build the space-separated DPM-level string for pp_dpm_* from a bitmask: bit N
+// selects level N (e.g. 0b101 -> "0 2 "). Bits above the highest settable level
+// are dropped before writing so an out-of-range level never gets requested.
+// The deep-sleep level ('S') is counted in num_supported but is not writable,
+// so it is excluded from the settable range. Throws RSMI_STATUS_UNEXPECTED_DATA
+// if the device reports zero settable levels (this should not happen on any
+// supported ASIC), or RSMI_STATUS_INVALID_ARGS if the mask selects no
+// settable level.
+static std::string bitfield_to_freq_string(uint64_t bitf, uint32_t num_supported,
+                                           bool has_deep_sleep = false) {
   if (num_supported > RSMI_MAX_NUM_FREQUENCIES) {
     throw amd::smi::rsmi_exception(RSMI_STATUS_INVALID_ARGS, __FUNCTION__);
   }
 
-  for (uint32_t i = 0; i < num_supported; ++i) {
+  uint32_t settable = (has_deep_sleep && num_supported > 0) ? num_supported - 1 : num_supported;
+  if (settable == 0) {
+    throw amd::smi::rsmi_exception(RSMI_STATUS_UNEXPECTED_DATA, __FUNCTION__);
+  }
+
+  std::string bf_str;
+  std::bitset<RSMI_MAX_NUM_FREQUENCIES> bs(bitf);
+  for (uint32_t i = 0; i < settable; ++i) {
     if (bs[i]) {
       bf_str += std::to_string(i);
       bf_str += " ";
     }
   }
+  if (bf_str.empty()) {
+    throw amd::smi::rsmi_exception(RSMI_STATUS_INVALID_ARGS, __FUNCTION__);
+  }
   return bf_str;
 }
 
@@ -2125,6 +2141,18 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ
     return RSMI_STATUS_INVALID_ARGS;
   }
 
+  amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance();
+  assert(dv_ind < smi.devices().size());
+  std::shared_ptr<amd::smi::Device> dev = smi.devices()[dv_ind];
+  assert(dev != nullptr);
+
+  std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true);
+  bool read_only = false;
+  // Dont try to set on pp_dpm* files that are read only
+  if (amd::smi::isReadOnlyForAll(sysfs_path, &read_only) == 0 && read_only) {
+    return RSMI_STATUS_NOT_SUPPORTED;
+  }
+
   ret = rsmi_dev_gpu_clk_freq_get(dv_ind, clk_type, &freqs);
 
   if (ret != RSMI_STATUS_SUCCESS) {
@@ -2136,46 +2164,26 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ
     return RSMI_STATUS_UNEXPECTED_SIZE;
   }
 
-  // Deep sleep entry (index 0 in pp_dpm_sclk marked with 'S') is not a real
-  // DPM level; subtract it from the valid count so the bitmask only covers
-  // actual DPM levels.  Bitmask bit i maps to sysfs DPM level i, which
-  // corresponds to freqs.frequency[i] when has_deep_sleep is false, or
-  // freqs.frequency[i+1] when has_deep_sleep is true.
-  if (freqs.num_supported > 0) {
-    uint32_t max_levels = freqs.num_supported;
-    if (freqs.has_deep_sleep) {
-      max_levels--;
-    }
-    uint64_t valid_mask = (max_levels > 0) ? (1ULL << max_levels) - 1 : 0;
-    if (freq_bitmask == 0 || (freq_bitmask & ~valid_mask)) {
-      return RSMI_STATUS_INVALID_ARGS;
-    }
-  }
-
-  amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance();
-
-  // Above call to rsmi_dev_get_gpu_clk_freq should have emitted an error if
-  // assert below is not true
-  assert(dv_ind < smi.devices().size());
-
-  std::shared_ptr<amd::smi::Device> dev = smi.devices()[dv_ind];
-  assert(dev != nullptr);
-
-  // If the sysfs node is read-only, force DPM level is not supported
-  std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true);
-  bool read_only = false;
-  int ro_ret = amd::smi::isReadOnlyForAll(sysfs_path, &read_only);
-  if (ro_ret != 0) {
-    return amd::smi::ErrnoToRsmiStatus(ro_ret);
-  }
-  if (read_only) {
-    return RSMI_STATUS_NOT_SUPPORTED;
+  // Logged before bitfield_to_freq_string()/perf-level-set so this context is
+  // captured even if a later step fails or throws (e.g. no settable level).
+  std::string clock_name = kClkStateMap.find(clk_type)->second;
+  ss << __PRETTY_FUNCTION__ << " | Requested clock frequency set for device " << dv_ind << "for "
+     << clock_name << "\n : mask=0x" << std::hex << freq_bitmask << std::dec
+     << ", num_supported=" << freqs.num_supported
+     << ", deep_sleep=" << (freqs.has_deep_sleep ? "Y" : "N") << ", current=" << freqs.current
+     << " | freqs[0.." << freqs.num_supported << "):";
+  for (uint32_t fi = 0; fi < freqs.num_supported && fi < RSMI_MAX_NUM_FREQUENCIES; ++fi) {
+    ss << " [" << fi << "]=" << freqs.frequency[fi];
   }
+  LOG_DEBUG(ss);
 
-  // bitfield_to_freq_string iterates [0, num_supported), but the validation
-  // above already guarantees no bits are set beyond the valid DPM range, so
-  // the extra iteration over the deep sleep index (if present) is harmless.
-  std::string freq_enable_str = bitfield_to_freq_string(freq_bitmask, freqs.num_supported);
+  // The deep-sleep pseudo-level ('S') is counted in freqs.num_supported but is
+  // not a writable pp_dpm_* label, so it is excluded from the settable range.
+  std::string freq_enable_str =
+      bitfield_to_freq_string(freq_bitmask, freqs.num_supported, freqs.has_deep_sleep);
+  ss << __PRETTY_FUNCTION__ << " | About to try writing clock frequency mask for device " << dv_ind
+     << " for " << clock_name << ": freq_enable_str=|" << freq_enable_str << "|";
+  LOG_DEBUG(ss);
 
   ret = rsmi_dev_perf_level_set_v1(dv_ind, RSMI_DEV_PERF_LEVEL_MANUAL);
   if (ret != RSMI_STATUS_SUCCESS) {
@@ -2184,14 +2192,10 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ
 
   rsmi_status_t status;
   status = amd::smi::ErrnoToRsmiStatus(dev->writeDevInfo(dev_type, freq_enable_str));
-
-  if (status == RSMI_STATUS_PERMISSION) {
-    bool post_read_only = false;
-    amd::smi::isReadOnlyForAll(sysfs_path, &post_read_only);
-    if (post_read_only) {
-      return RSMI_STATUS_NOT_SUPPORTED;
-    }
-  }
+  ss << __PRETTY_FUNCTION__ << " | Wrote clock frequency mask for device " << dv_ind << " for "
+     << clock_name << ": freq_enable_str=|" << freq_enable_str << "|"
+     << "; returned: " << amd::smi::getRSMIStatusString(status, false);
+  LOG_DEBUG(ss);
 
   return status;
 

projects/amdsmi/tests/amd_smi_test/amdsmitst.exclude

@@ -47,7 +47,6 @@ $BLACKLIST_ALL_ASICS\
 "GpuFunctionalReadOnly.TestVoltCurvRead:"\
 "GpuFunctionalReadOnly.TestFrequenciesRead:"\
 "GpuFunctionalReadWrite.FanReadWrite:"\
-"GpuFunctionalReadWrite.TestFrequenciesReadWrite:"\
 "GpuFunctionalReadWrite.TestPciReadWrite:"\
 "GpuFunctionalReadWrite.TestPowerReadWrite"