From 202b46d96f342f30ce805bd36cc7229c539458a4 Mon Sep 17 00:00:00 2001 From: "Galantsev, Dmitrii" Date: Wed, 11 Jun 2025 10:23:47 -0500 Subject: [PATCH] DRM - Remove caching Change-Id: I21716cc953462e385e981024f75a9a7c2d76a466 Signed-off-by: Galantsev, Dmitrii --- include/amd_smi/impl/amd_smi_utils.h | 51 +++++++----- src/amd_smi/amd_smi.cc | 32 ++------ src/amd_smi/amd_smi_drm.cc | 4 +- src/amd_smi/amd_smi_utils.cc | 114 +++++++++++++-------------- 4 files changed, 94 insertions(+), 107 deletions(-) diff --git a/include/amd_smi/impl/amd_smi_utils.h b/include/amd_smi/impl/amd_smi_utils.h index 1014237a44..818ba642ec 100644 --- a/include/amd_smi/impl/amd_smi_utils.h +++ b/include/amd_smi/impl/amd_smi_utils.h @@ -60,32 +60,15 @@ std::string smi_amdgpu_get_status_string(amdsmi_status_t ret, bool fullStatus); amdsmi_status_t smi_clear_char_and_reinitialize(char buffer[], uint32_t len, std::string newString); -/** - * @brief Opens a file descriptor for the specified path with RAII semantics and caching. - * - * This function attempts to open a file descriptor (FD) for the given file path and flags. - * It maintains a cache of weak pointers to previously opened FDs, allowing for reuse of - * file descriptors if they are still valid. If a valid FD for the path exists in the cache, - * it is reused; otherwise, a new FD is opened. The returned FD is managed by a std::shared_ptr - * with a custom deleter that ensures the FD is properly closed when no longer in use. - * - * Thread safety is ensured via a static mutex. - * - * @param path The file system path to open. - * @param flags Flags to use when opening the file (as per open(2)). - * @return std::shared_ptr Shared pointer managing the file descriptor, or nullptr on failure. - */ -std::shared_ptr amdsmi_RAII_FD_handler(const std::string& path, int flags); - /** * @brief Wait for user input, a debugging function to pause the program - * + * * @details This function will wait for user input before continuing. * It is useful for debugging purposes to allow the user to inspect the state of the program * before it continues. The function will print a message to the console and then wait for * the user to press Enter. Once Enter is pressed, the function will return and the program * will continue executing. - * + * * @note This function is intended for debugging purposes only and should not be used in production code. * It will block the program execution and cause it to wait indefinitely for user input. */ @@ -94,7 +77,7 @@ void amdsmi_wait_for_user_input(void); /** * @brief Get the device index given the processor handle. * - * @details Given a processor handle @p processor_handle + * @details Given a processor handle @p processor_handle * and a pointer to a uint32_t @p device_index will be returned. * * @param[in] processor_handle Device which to query @@ -205,3 +188,31 @@ void fill_2d_array(A& arr, T value) { } #endif // AMD_SMI_INCLUDE_AMD_SMI_UTILS_H_ + +#ifndef SCOPED_FD_H +#define SCOPED_FD_H + +class ScopedFD { +private: + int fd_; + std::string path_; + +public: + ScopedFD(const std::string& path, int flags); + ~ScopedFD(); + + // Non-copyable + ScopedFD(const ScopedFD&) = delete; + ScopedFD& operator=(const ScopedFD&) = delete; + + // Movable + ScopedFD(ScopedFD&& other) noexcept; + ScopedFD& operator=(ScopedFD&& other) noexcept; + + int get() const; + bool valid() const; + operator int() const; // Allows direct use as int + int operator*() const; // Allows *fd usage like your current code +}; + +#endif // SCOPED_FD_H diff --git a/src/amd_smi/amd_smi.cc b/src/amd_smi/amd_smi.cc index 6fbec15656..7dc92fffda 100644 --- a/src/amd_smi/amd_smi.cc +++ b/src/amd_smi/amd_smi.cc @@ -904,12 +904,7 @@ amdsmi_status_t amdsmi_get_gpu_vram_usage(amdsmi_processor_handle processor_hand } std::string path = "/dev/dri/" + render_name; - auto drm_fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); - ss << __PRETTY_FUNCTION__ - << " | open(" << path << ") returned: " << strerror(errno) << "\n" - << " | drm_fd: " << (drm_fd == nullptr ? "nullptr" : std::to_string(*drm_fd)) << "\n" - << " | render_name: " << render_name << "\n"; - LOG_INFO(ss); + ScopedFD drm_fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!drm_fd) { ss << __PRETTY_FUNCTION__ << " | Failed to open " << path << ": " << strerror(errno) @@ -1666,12 +1661,7 @@ amdsmi_get_gpu_asic_info(amdsmi_processor_handle processor_handle, amdsmi_asic_i return AMDSMI_STATUS_NOT_SUPPORTED; } std::string path = "/dev/dri/" + render_name; - auto drm_fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); - ss << __PRETTY_FUNCTION__ - << " | open(" << path << ") returned: " << strerror(errno) << "\n" - << " | drm_fd: " << (drm_fd == nullptr ? "nullptr" : std::to_string(*drm_fd)) << "\n" - << " | render_name: " << render_name << "\n"; - LOG_INFO(ss); + ScopedFD drm_fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!drm_fd) { ss << __PRETTY_FUNCTION__ << " | Failed to open " << path << ": " << strerror(errno) @@ -1894,12 +1884,7 @@ amdsmi_status_t amdsmi_get_gpu_vram_info( return AMDSMI_STATUS_NOT_SUPPORTED; } - auto drm_fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); - ss << __PRETTY_FUNCTION__ - << " | open(" << path << ") returned: " << strerror(errno) << "\n" - << " | drm_fd: " << (drm_fd == nullptr ? "nullptr" : std::to_string(*drm_fd)) << "\n" - << " | render_name: " << render_name << "\n"; - LOG_INFO(ss); + ScopedFD drm_fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!drm_fd) { ss << __PRETTY_FUNCTION__ << " | Failed to open " << path << ": " << strerror(errno) @@ -3757,12 +3742,7 @@ amdsmi_get_gpu_vbios_info(amdsmi_processor_handle processor_handle, amdsmi_vbios return AMDSMI_STATUS_NOT_SUPPORTED; } - auto drm_fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); - ss << __PRETTY_FUNCTION__ - << " | open(" << path << ") returned: " << strerror(errno) << "\n" - << " | drm_fd: " << (drm_fd == nullptr ? "nullptr" : std::to_string(*drm_fd)) << "\n" - << " | render_name: " << render_name << "\n"; - LOG_INFO(ss); + ScopedFD drm_fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!drm_fd) { ss << __PRETTY_FUNCTION__ << " | Failed to open " << path << ": " << strerror(errno) @@ -4325,7 +4305,7 @@ amdsmi_status_t amdsmi_get_gpu_driver_info(amdsmi_processor_handle processor_han return AMDSMI_STATUS_NOT_SUPPORTED; } - auto drm_fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); + ScopedFD drm_fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!drm_fd) { ss << __PRETTY_FUNCTION__ << " | Failed to open " << path << ": " << strerror(errno) @@ -4801,7 +4781,7 @@ amdsmi_get_gpu_virtualization_mode(amdsmi_processor_handle processor_handle, return AMDSMI_STATUS_NOT_SUPPORTED; } - auto drm_fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); + ScopedFD drm_fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!drm_fd) { ss << __PRETTY_FUNCTION__ << " | Failed to open " << path << ": " << strerror(errno) diff --git a/src/amd_smi/amd_smi_drm.cc b/src/amd_smi/amd_smi_drm.cc index 00add9d4e1..40078a5777 100644 --- a/src/amd_smi/amd_smi_drm.cc +++ b/src/amd_smi/amd_smi_drm.cc @@ -131,12 +131,12 @@ amdsmi_status_t AMDSmiDrm::init() { // looking for /sys/class/drm/card0/../renderD* std::string render_name = find_file_in_folder(renderD_folder, regex); std::string name = "/dev/dri/" + render_name; - auto fd = amdsmi_RAII_FD_handler(name.c_str(), O_RDWR | O_CLOEXEC); + ScopedFD fd(name.c_str(), O_RDWR | O_CLOEXEC); amdsmi_bdf_t bdf; if (*fd >= 0) { auto version = drm_get_version(*fd); - if (*fd >= 0 && drm_get_device(*fd, &device) != 0) { + if (drm_get_device(*fd, &device) != 0) { drm_free_device(&device); } ss << __PRETTY_FUNCTION__ << " | " diff --git a/src/amd_smi/amd_smi_utils.cc b/src/amd_smi/amd_smi_utils.cc index 4db60531ca..e0fe8ec733 100644 --- a/src/amd_smi/amd_smi_utils.cc +++ b/src/amd_smi/amd_smi_utils.cc @@ -407,9 +407,9 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ } else { /** * if the first line contains '*', then - * we are saving that value as current_freq then checking + * we are saving that value as current_freq then checking * for other dpm levels if none are found then we - * set min and max to current_freq as per Driver + * set min and max to current_freq as per Driver * We then skip to the next line to avoid getting * incorrect min value. */ @@ -671,15 +671,11 @@ amdsmi_status_t smi_amdgpu_get_market_name_from_dev_id(amd::smi::AMDSmiGPUDevice return AMDSMI_STATUS_NOT_SUPPORTED; } - auto fd = amdsmi_RAII_FD_handler(path.c_str(), O_RDWR | O_CLOEXEC); - ss << __PRETTY_FUNCTION__ << " | Render Name: " - << render_name << "; path: " << path << "; fd: " - << (fd == nullptr ? "nullptr" : std::to_string(*fd)) << "\n"; - LOG_DEBUG(ss); + ScopedFD fd(path.c_str(), O_RDWR | O_CLOEXEC); if (!fd) { ss << __PRETTY_FUNCTION__ << " | Render Name: " << render_name << "; path: " << path << "; fd: " - << (fd == nullptr ? "nullptr" : std::to_string(*fd)) << "\n" + << (fd < 0 ? "less than 0" : std::to_string(*fd)) << "\n" << "; Returning: " << smi_amdgpu_get_status_string(AMDSMI_STATUS_FILE_ERROR, false) << "\n"; LOG_INFO(ss); @@ -1023,57 +1019,57 @@ void amdsmi_wait_for_user_input(void) { } } -std::shared_ptr amdsmi_RAII_FD_handler(const std::string& path, int flags) { - static std::mutex fd_mutex; - static std::map> open_files; - static std::ostringstream ss; - - std::lock_guard lock(fd_mutex); - - // Clean up expired entries from the cache - for (auto it = open_files.begin(); it != open_files.end();) { - if (it->second.expired()) { - it = open_files.erase(it); - } else { - ++it; - } - } - - // Try to reuse an existing open FD - auto it = open_files.find(path); - if (it != open_files.end()) { - if (auto existing_fd = it->second.lock()) { - ss <<__PRETTY_FUNCTION__ << " | Reusing FD for path: " << path; - LOG_INFO(ss); - return existing_fd; - } - } - - // Open a new file descriptor - int fd = open(path.c_str(), flags); - if (fd < 0) { - ss << __PRETTY_FUNCTION__ << " | Failed to open file: " << path - << " | Error: " << strerror(errno); +ScopedFD::ScopedFD(const std::string& path, int flags) + : fd_(open(path.c_str(), flags)), path_(path) { + std::ostringstream ss; + if (fd_ >= 0) { + ss << __PRETTY_FUNCTION__ << " | Opened FD: " << fd_ + << " for path: " << path_; LOG_INFO(ss); - return nullptr; + } else { + ss << __PRETTY_FUNCTION__ << " | Failed to open file: " << path_ + << " | Error: " << strerror(errno); + LOG_ERROR(ss); } - - ss << __PRETTY_FUNCTION__ << " | Opened FD: " << std::to_string(fd) - << " for path: " << path; - LOG_INFO(ss); - - // Create a shared_ptr with a custom deleter to close the FD - auto fd_ptr = std::shared_ptr(new int(fd), [path](int* fd) { - if (fd && *fd >= 0) { - ss << __PRETTY_FUNCTION__ << " | Closing FD: " << std::to_string(*fd) - << " | Path: " << path << std::endl; - LOG_INFO(ss); - close(*fd); - delete fd; - } - }); - - // Store weak_ptr in cache for reuse - open_files[path] = fd_ptr; - return fd_ptr; +} + +ScopedFD::~ScopedFD() { + if (fd_ >= 0) { + std::ostringstream ss; + ss << __PRETTY_FUNCTION__ << " | Closing FD: " << fd_ + << " for path: " << path_; + LOG_INFO(ss); + close(fd_); + } +} + +ScopedFD::ScopedFD(ScopedFD&& other) noexcept + : fd_(other.fd_), path_(std::move(other.path_)) { + other.fd_ = -1; +} + +ScopedFD& ScopedFD::operator=(ScopedFD&& other) noexcept { + if (this != &other) { + if (fd_ >= 0) close(fd_); + fd_ = other.fd_; + path_ = std::move(other.path_); + other.fd_ = -1; + } + return *this; +} + +int ScopedFD::get() const { + return fd_; +} + +bool ScopedFD::valid() const { + return fd_ >= 0; +} + +ScopedFD::operator int() const { + return fd_; +} + +int ScopedFD::operator*() const { + return fd_; }