From 9e69c1f9aa3cf0bca9bead4bfe60decb329bb62a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 24 Sep 2026 10:38:11 -0600 Subject: [PATCH] Savedata: Keep the IO thread out of the dialog's state Replaces the temporary fix that did the hidden modes' IO inside Update. The IO thread read and wrote the dialog's request, display state and save list, all shared with the emulator thread, which kept using them to draw the dialog and reload the request from the game. Now the IO thread works on its own copy of the request, its own SavedataParam and directory names resolved up front, and shares nothing else with the emulator thread but the (locked) file system, MemoryStick_FreeSpace's cached use and sceChnnlsv's scratch buffer and kirk state, the last two now under locks too. It still reads and writes the game's buffers directly, like a PSP's utility threads and sceIoReadAsync do, so a savestate waits for it before it saves or loads memory. Save bookkeeping, the save indicator and display changes happen on the emulator thread when the results are taken, and only the request fields the IO changed are copied back, so a game's own edits in the meantime survive. Hidden modes take the results at the next Update (or, with Host IO timing, the first Update that finds them done). The visible dialogs keep drawing and take them once the IO is done; save and load used to stall the emulator thread for the whole operation. Savestates keep results that haven't been taken yet. When the results land in PSP memory doesn't matter to games, so utility/savedata/filelist now only prints them once the utility has finished. Co-Authored-By: Claude Opus 5.5 (1M context) --- Core/Dialog/PSPSaveDialog.cpp | 301 ++++++++++++++++++++-------------- Core/Dialog/PSPSaveDialog.h | 27 ++- Core/Dialog/SavedataParam.cpp | 11 +- Core/Dialog/SavedataParam.h | 4 +- Core/HLE/sceChnnlsv.cpp | 11 ++ Core/HLE/sceUtility.cpp | 7 + Core/HLE/sceUtility.h | 2 + Core/HW/MemoryStick.cpp | 32 ++-- Core/SaveState.cpp | 3 + pspautotests | 2 +- 10 files changed, 261 insertions(+), 139 deletions(-) diff --git a/Core/Dialog/PSPSaveDialog.cpp b/Core/Dialog/PSPSaveDialog.cpp index 7254f6c217..271f3baae9 100755 --- a/Core/Dialog/PSPSaveDialog.cpp +++ b/Core/Dialog/PSPSaveDialog.cpp @@ -770,11 +770,8 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_SAVE_SAVING: - if (ioThreadStatus != SAVEIO_PENDING) { - if (ioThread.joinable()) { - ioThread.join(); - } - } + // The dialog keeps drawing while the IO runs, and takes the results once it's done. + FinishIO(false); StartDraw(); @@ -788,9 +785,6 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_SAVE_FAILED: - if (ioThread.joinable()) { - ioThread.join(); - } StartDraw(); DisplaySaveIcon(true); @@ -814,10 +808,6 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_SAVE_DONE: - if (ioThread.joinable()) { - ioThread.join(); - param.SetPspParam(param.GetPspParam()); - } StartDraw(); DisplaySaveIcon(true); @@ -879,11 +869,8 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_LOAD_LOADING: - if (ioThreadStatus != SAVEIO_PENDING) { - if (ioThread.joinable()) { - ioThread.join(); - } - } + // The dialog keeps drawing while the IO runs, and takes the results once it's done. + FinishIO(false); StartDraw(); @@ -897,9 +884,6 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_LOAD_FAILED: - if (ioThread.joinable()) { - ioThread.join(); - } StartDraw(); DisplaySaveIcon(true); @@ -922,9 +906,6 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_LOAD_DONE: - if (ioThread.joinable()) { - ioThread.join(); - } StartDraw(); DisplaySaveIcon(true); @@ -1010,11 +991,8 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_DELETE_DELETING: - if (ioThreadStatus != SAVEIO_PENDING) { - if (ioThread.joinable()) { - ioThread.join(); - } - } + // The dialog keeps drawing while the IO runs, and takes the results once it's done. + FinishIO(false); StartDraw(); @@ -1025,9 +1003,6 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_DELETE_FAILED: - if (ioThread.joinable()) { - ioThread.join(); - } StartDraw(); DisplayMessage(di->T("DeleteFailed", "Unable to delete data.")); @@ -1045,10 +1020,6 @@ int PSPSaveDialog::Update(int animSpeed) { EndDraw(); break; case DS_DELETE_DONE: - if (ioThread.joinable()) { - ioThread.join(); - param.SetPspParam(param.GetPspParam()); - } StartDraw(); DisplayMessage(di->T("Delete completed")); @@ -1088,28 +1059,10 @@ int PSPSaveDialog::Update(int animSpeed) { break; case DS_NONE: // For action which display nothing - switch (ioThreadStatus) { - case SAVEIO_NONE: - if (g_Config.iIOTimingMethod == IOTIMING_HOST) { - StartIOThread(); - } else { - // The IO thread writes the results into PSP memory while the game runs, landing at - // an arbitrary point in its code. On a PSP they're all there when Update returns. - ExecuteIOAction(); - } - break; - case SAVEIO_PENDING: - case SAVEIO_DONE: - // To make sure there aren't any timing variations, we sync the next frame. - if (g_Config.iIOTimingMethod == IOTIMING_HOST && ioThreadStatus == SAVEIO_PENDING) { - // ... except in Host IO timing, where we wait as long as needed. - break; - } - if (ioThread.joinable()) { - ioThread.join(); - } + if (ioThreadStatus == SAVEIO_NONE) { + StartIOThread(); + } else if (FinishIO(g_Config.iIOTimingMethod != IOTIMING_HOST)) { ChangeStatus(SCE_UTILITY_STATUS_FINISHED, 0); - break; } break; @@ -1125,37 +1078,26 @@ int PSPSaveDialog::Update(int animSpeed) { return 0; } -// It's kinda ugly how this uses the "global" 'display'... +// Runs on the IO thread. Of the dialog, it only touches the io* members (see StartIOThread and FinishIO). void PSPSaveDialog::ExecuteIOAction() { - param.ClearSFOCache(); - auto &result = param.GetPspParam()->common.result; - std::lock_guard guard(paramLock); - switch (display) { + ioParam_.ClearSFOCache(); + auto &result = ioRequest_.common.result; + ioDisplay_ = ioAction_; + switch (ioAction_) { case DS_LOAD_LOADING: - result = param.Load(param.GetPspParam(), GetSelectedSaveDirName(), currentSelectedSave); - if (result == 0) { - display = DS_LOAD_DONE; - g_lastSaveTime = time_now_d(); - } else { - display = DS_LOAD_FAILED; - } + result = ioParam_.Load(&ioRequest_, ioSaveDirName_, ioSaveId_); + ioDisplay_ = result == 0 ? DS_LOAD_DONE : DS_LOAD_FAILED; break; case DS_SAVE_SAVING: - SaveState::NotifySaveData(); - if (param.Save(param.GetPspParam(), GetSelectedSaveDirName()) == 0) { - display = DS_SAVE_DONE; - g_lastSaveTime = time_now_d(); - } else { - display = DS_SAVE_FAILED; - } + ioDisplay_ = ioParam_.Save(&ioRequest_, ioSaveDirName_) == 0 ? DS_SAVE_DONE : DS_SAVE_FAILED; break; case DS_DELETE_DELETING: - if (param.Delete(param.GetPspParam(), currentSelectedSave)) { + if (ioParam_.Delete(&ioRequest_, ioDeleteDir_)) { result = 0; - display = DS_DELETE_DONE; + ioDisplay_ = DS_DELETE_DONE; } else { //result = SCE_UTILITY_SAVEDATA_ERROR_DELETE_NO_DATA;// What the result should be? - display = DS_DELETE_FAILED; + ioDisplay_ = DS_DELETE_FAILED; } break; case DS_NONE: @@ -1167,42 +1109,36 @@ void PSPSaveDialog::ExecuteIOAction() { break; } - ioThreadStatus = SAVEIO_DONE; - param.ClearSFOCache(); + ioParam_.ClearSFOCache(); + ioThreadStatus = SAVEIO_READY; } void PSPSaveDialog::ExecuteNotVisibleIOAction() { - param.ClearSFOCache(); - auto &result = param.GetPspParam()->common.result; + auto &result = ioRequest_.common.result; - SceUtilitySavedataType utilityMode = (SceUtilitySavedataType)(u32)param.GetPspParam()->mode; + SceUtilitySavedataType utilityMode = (SceUtilitySavedataType)(u32)ioRequest_.mode; switch (utilityMode) { case SCE_UTILITY_SAVEDATA_TYPE_LOAD: // Only load and exit case SCE_UTILITY_SAVEDATA_TYPE_AUTOLOAD: - result = param.Load(param.GetPspParam(), GetSelectedSaveDirName(), currentSelectedSave); - ResetSecondsSinceLastGameSave(); - ShowSaveLoadIndicator(false); + result = ioParam_.Load(&ioRequest_, ioSaveDirName_, ioSaveId_); break; case SCE_UTILITY_SAVEDATA_TYPE_SAVE: // Only save and exit case SCE_UTILITY_SAVEDATA_TYPE_AUTOSAVE: - SaveState::NotifySaveData(); - result = param.Save(param.GetPspParam(), GetSelectedSaveDirName()); - ResetSecondsSinceLastGameSave(); - ShowSaveLoadIndicator(true); + result = ioParam_.Save(&ioRequest_, ioSaveDirName_); break; case SCE_UTILITY_SAVEDATA_TYPE_SIZES: - result = param.GetSizes(param.GetPspParam()); + result = ioParam_.GetSizes(&ioRequest_); break; case SCE_UTILITY_SAVEDATA_TYPE_LIST: - param.GetList(param.GetPspParam()); + ioParam_.GetList(&ioRequest_); result = 0; break; case SCE_UTILITY_SAVEDATA_TYPE_FILES: - result = param.GetFilesList(param.GetPspParam(), requestAddr); + result = ioParam_.GetFilesList(&ioRequest_, requestAddr, ioListSaveDirName_); break; case SCE_UTILITY_SAVEDATA_TYPE_GETSIZE: { - bool sizeResult = param.GetSize(param.GetPspParam()); + bool sizeResult = ioParam_.GetSize(&ioRequest_); // TODO: According to JPCSP, should test/verify this part but seems edge casey. if (MemoryStick_State() != PSP_MEMORYSTICK_STATE_INSERTED) { result = SCE_UTILITY_SAVEDATA_ERROR_RW_NO_MEMSTICK; @@ -1214,8 +1150,8 @@ void PSPSaveDialog::ExecuteNotVisibleIOAction() { } break; case SCE_UTILITY_SAVEDATA_TYPE_DELETEDATA: - DEBUG_LOG(Log::sceUtility, "sceUtilitySavedata DELETEDATA: %s", param.GetPspParam()->saveName); - if (param.Delete(param.GetPspParam(), param.GetSelectedSave())) { + DEBUG_LOG(Log::sceUtility, "sceUtilitySavedata DELETEDATA: %s", ioRequest_.saveName); + if (ioParam_.Delete(&ioRequest_, ioDeleteDir_)) { result = 0; } else { result = SCE_UTILITY_SAVEDATA_ERROR_RW_NO_DATA; @@ -1223,7 +1159,7 @@ void PSPSaveDialog::ExecuteNotVisibleIOAction() { break; case SCE_UTILITY_SAVEDATA_TYPE_AUTODELETE: case SCE_UTILITY_SAVEDATA_TYPE_DELETE: - if (param.Delete(param.GetPspParam(), param.GetSelectedSave())) { + if (ioParam_.Delete(&ioRequest_, ioDeleteDir_)) { result = 0; } else { result = SCE_UTILITY_SAVEDATA_ERROR_DELETE_NO_DATA; @@ -1232,41 +1168,30 @@ void PSPSaveDialog::ExecuteNotVisibleIOAction() { // TODO: Should reset the directory's other files. case SCE_UTILITY_SAVEDATA_TYPE_MAKEDATA: case SCE_UTILITY_SAVEDATA_TYPE_MAKEDATASECURE: - result = param.Save(param.GetPspParam(), GetSelectedSaveDirName(), param.GetPspParam()->mode == SCE_UTILITY_SAVEDATA_TYPE_MAKEDATASECURE); + result = ioParam_.Save(&ioRequest_, ioSaveDirName_, ioRequest_.mode == SCE_UTILITY_SAVEDATA_TYPE_MAKEDATASECURE); if (result == SCE_UTILITY_SAVEDATA_ERROR_SAVE_MS_NOSPACE) { result = SCE_UTILITY_SAVEDATA_ERROR_RW_MEMSTICK_FULL; - } else { - SaveState::NotifySaveData(); - ResetSecondsSinceLastGameSave(); - ShowSaveLoadIndicator(true); } break; case SCE_UTILITY_SAVEDATA_TYPE_WRITEDATA: case SCE_UTILITY_SAVEDATA_TYPE_WRITEDATASECURE: - SaveState::NotifySaveData(); - result = param.Save(param.GetPspParam(), GetSelectedSaveDirName(), param.GetPspParam()->mode == SCE_UTILITY_SAVEDATA_TYPE_WRITEDATASECURE); - ResetSecondsSinceLastGameSave(); - ShowSaveLoadIndicator(true); + result = ioParam_.Save(&ioRequest_, ioSaveDirName_, ioRequest_.mode == SCE_UTILITY_SAVEDATA_TYPE_WRITEDATASECURE); break; case SCE_UTILITY_SAVEDATA_TYPE_READDATA: case SCE_UTILITY_SAVEDATA_TYPE_READDATASECURE: - result = param.Load(param.GetPspParam(), GetSelectedSaveDirName(), currentSelectedSave, param.GetPspParam()->mode == SCE_UTILITY_SAVEDATA_TYPE_READDATASECURE); + result = ioParam_.Load(&ioRequest_, ioSaveDirName_, ioSaveId_, ioRequest_.mode == SCE_UTILITY_SAVEDATA_TYPE_READDATASECURE); if (result == SCE_UTILITY_SAVEDATA_ERROR_LOAD_DATA_BROKEN) result = SCE_UTILITY_SAVEDATA_ERROR_RW_DATA_BROKEN; if (result == SCE_UTILITY_SAVEDATA_ERROR_LOAD_NO_DATA) result = SCE_UTILITY_SAVEDATA_ERROR_RW_NO_DATA; - ResetSecondsSinceLastGameSave(); - ShowSaveLoadIndicator(false); break; case SCE_UTILITY_SAVEDATA_TYPE_ERASE: case SCE_UTILITY_SAVEDATA_TYPE_ERASESECURE: - result = param.DeleteData(param.GetPspParam()); + result = ioParam_.DeleteData(&ioRequest_); break; default: break; } - - param.ClearSFOCache(); } void PSPSaveDialog::StartIOThread() { @@ -1282,6 +1207,24 @@ void PSPSaveDialog::StartIOThread() { ShowSaveLoadIndicator(save); } + // Everything the IO thread needs from the dialog, taken now: it doesn't look at the dialog's state. + const SceUtilitySavedataType mode = (SceUtilitySavedataType)(u32)request.mode; + ioAction_ = display; + ioRequest_ = request; + ioRequestStart_ = request; + ioSaveId_ = currentSelectedSave; + ioSaveDirName_ = GetSelectedSaveDirName(); + ioListSaveDirName_.clear(); + if (display == DS_NONE && mode == SCE_UTILITY_SAVEDATA_TYPE_FILES) { + ioListSaveDirName_ = param.GetSaveDirName(&request, 0); + } + ioDeleteDir_.clear(); + if (display == DS_DELETE_DELETING) { + ioDeleteDir_ = param.GetSaveDir(currentSelectedSave); + } else if (display == DS_NONE && (mode == SCE_UTILITY_SAVEDATA_TYPE_DELETEDATA || mode == SCE_UTILITY_SAVEDATA_TYPE_AUTODELETE || mode == SCE_UTILITY_SAVEDATA_TYPE_DELETE)) { + ioDeleteDir_ = param.GetSaveDir(param.GetSelectedSave()); + } + ioThreadStatus = SAVEIO_PENDING; ioThread = std::thread([this]() { SetCurrentThreadName("SaveIO"); @@ -1291,6 +1234,93 @@ void PSPSaveDialog::StartIOThread() { }); } +// Takes the IO thread's results back. Without wait, only if it's done: returns whether it was. +bool PSPSaveDialog::FinishIO(bool wait) { + if (ioThreadStatus == SAVEIO_PENDING && !wait) { + return false; + } + if (ioThread.joinable()) { + ioThread.join(); + } + if (ioThreadStatus != SAVEIO_READY) { + return true; + } + + { + // Only what the IO changed: the game may have changed its request meanwhile (Update reloads it). + std::lock_guard guard(paramLock); + const u8 *before = (const u8 *)&ioRequestStart_; + const u8 *after = (const u8 *)&ioRequest_; + u8 *live = (u8 *)&request; + for (size_t i = 0; i < sizeof(request); ++i) { + if (after[i] != before[i]) { + live[i] = after[i]; + } + } + } + + switch (ioAction_) { + case DS_LOAD_LOADING: + if (ioDisplay_ == DS_LOAD_DONE) { + g_lastSaveTime = time_now_d(); + } + break; + case DS_SAVE_SAVING: + SaveState::NotifySaveData(); + if (ioDisplay_ == DS_SAVE_DONE) { + g_lastSaveTime = time_now_d(); + } + break; + case DS_NONE: + switch ((SceUtilitySavedataType)(u32)request.mode) { + case SCE_UTILITY_SAVEDATA_TYPE_LOAD: + case SCE_UTILITY_SAVEDATA_TYPE_AUTOLOAD: + case SCE_UTILITY_SAVEDATA_TYPE_READDATA: + case SCE_UTILITY_SAVEDATA_TYPE_READDATASECURE: + ResetSecondsSinceLastGameSave(); + ShowSaveLoadIndicator(false); + break; + case SCE_UTILITY_SAVEDATA_TYPE_MAKEDATA: + case SCE_UTILITY_SAVEDATA_TYPE_MAKEDATASECURE: + if (request.common.result == SCE_UTILITY_SAVEDATA_ERROR_RW_MEMSTICK_FULL) { + break; + } + [[fallthrough]]; + case SCE_UTILITY_SAVEDATA_TYPE_SAVE: + case SCE_UTILITY_SAVEDATA_TYPE_AUTOSAVE: + case SCE_UTILITY_SAVEDATA_TYPE_WRITEDATA: + case SCE_UTILITY_SAVEDATA_TYPE_WRITEDATASECURE: + SaveState::NotifySaveData(); + ResetSecondsSinceLastGameSave(); + ShowSaveLoadIndicator(true); + break; + default: + break; + } + break; + default: + break; + } + + if (ioAction_ != DS_NONE) { + display = ioDisplay_; + // Show what was saved or deleted in the list. + if (display == DS_SAVE_DONE || display == DS_DELETE_DONE) { + std::lock_guard guard(paramLock); + param.SetPspParam(param.GetPspParam()); + } + } + + ioThreadStatus = SAVEIO_DONE; + return true; +} + +void PSPSaveDialog::WaitForIO() { + if (ioThread.joinable()) { + ioThread.join(); + } +} + int PSPSaveDialog::Shutdown(bool force) { if (GetStatus() != SCE_UTILITY_STATUS_FINISHED && !force) return SCE_ERROR_UTILITY_INVALID_STATUS; @@ -1310,6 +1340,28 @@ int PSPSaveDialog::Shutdown(bool force) { return 0; } +// Version 4 of the dialog's state (never in a release) kept the IO's writes to PSP memory until the +// results were taken. Late is better than never. +static void DoStateOldPendingWrites(PointerWrap &p) { + auto s = p.Section("SavedataMemory", 1); + if (!s) { + return; + } + u32 count = 0; + Do(p, count); + for (u32 i = 0; i < count; ++i) { + u32 addr = 0; + std::string tag; + std::vector data; + Do(p, addr); + Do(p, tag); + Do(p, data); + if (p.mode == PointerWrap::MODE_READ && Memory::IsValidRange(addr, (u32)data.size())) { + Memory::Memcpy(addr, data.data(), (u32)data.size(), tag.c_str(), tag.size()); + } + } +} + void PSPSaveDialog::DoState(PointerWrap &p) { if (ioThread.joinable()) { ioThread.join(); @@ -1318,11 +1370,11 @@ void PSPSaveDialog::DoState(PointerWrap &p) { // Version 3 activates the s > 2 branch below, so ioThreadStatus survives // a savestate. Safe to restore: the IO thread was joined above, so the - // value is only ever SAVEIO_NONE or SAVEIO_DONE, and the operation's - // effects are already part of the serialized state. Without this, loading - // a state taken while a savedata operation was in flight would restart - // the operation instead of resuming from its recorded status. - auto s = p.Section("PSPSaveDialog", 1, 3); + // value is never SAVEIO_PENDING. Without this, loading a state taken + // while a savedata operation was in flight would restart the operation + // instead of resuming from its recorded status. Version 4 keeps the + // results of a finished operation that haven't been taken yet. + auto s = p.Section("PSPSaveDialog", 1, 5); if (!s) { return; } @@ -1339,10 +1391,21 @@ void PSPSaveDialog::DoState(PointerWrap &p) { Do(p, requestAddr); Do(p, currentSelectedSave); Do(p, yesnoChoice); + SaveIOStatus ioStatus = ioThreadStatus; if (s > 2) { - Do(p, ioThreadStatus); + Do(p, ioStatus); } else { - ioThreadStatus = SAVEIO_NONE; + ioStatus = SAVEIO_NONE; + } + ioThreadStatus = ioStatus; + if (s >= 4) { + Do(p, ioAction_); + Do(p, ioDisplay_); + Do(p, ioRequest_); + Do(p, ioRequestStart_); + } + if (s == 4) { + DoStateOldPendingWrites(p); } } diff --git a/Core/Dialog/PSPSaveDialog.h b/Core/Dialog/PSPSaveDialog.h index 8fe2b2d23e..0dbaf84467 100644 --- a/Core/Dialog/PSPSaveDialog.h +++ b/Core/Dialog/PSPSaveDialog.h @@ -17,6 +17,8 @@ #pragma once +#include +#include #include #include @@ -33,8 +35,8 @@ public: int Shutdown(bool force = false) override; void DoState(PointerWrap &p) override; pspUtilityDialogCommon *GetCommonParam() override; - - void ExecuteIOAction(); + // Waits for the IO thread, if any, to be done with PSP memory. Its results are still taken as usual. + void WaitForIO(); protected: bool UseAutoStatus() override { @@ -51,6 +53,8 @@ private: std::string GetSelectedSaveDirName() const; void StartIOThread(); + bool FinishIO(bool wait); + void ExecuteIOAction(); void ExecuteNotVisibleIOAction(); enum DisplayState { @@ -98,12 +102,29 @@ private: enum SaveIOStatus { SAVEIO_NONE, SAVEIO_PENDING, + // Finished, and the results taken. SAVEIO_DONE, + // Finished, but the request changes and bookkeeping haven't been taken yet. + SAVEIO_READY, }; std::thread ioThread; std::mutex paramLock; - volatile SaveIOStatus ioThreadStatus = SAVEIO_NONE; + std::atomic ioThreadStatus{ SAVEIO_NONE }; + + // The IO thread uses these instead of the dialog's own state (it writes its results to PSP + // memory directly). StartIOThread sets them up and FinishIO takes the results back, both on the + // emulator thread. + DisplayState ioAction_ = DS_NONE; + DisplayState ioDisplay_ = DS_NONE; + SceUtilitySavedataParam ioRequest_{}; + // The request when the IO started, to tell what the IO changed. + SceUtilitySavedataParam ioRequestStart_{}; + SavedataParam ioParam_; + int ioSaveId_ = 0; + std::string ioSaveDirName_; + std::string ioListSaveDirName_; + std::string ioDeleteDir_; }; void ResetSecondsSinceLastGameSave(); diff --git a/Core/Dialog/SavedataParam.cpp b/Core/Dialog/SavedataParam.cpp index 9b9b410a2e..f4923fc041 100644 --- a/Core/Dialog/SavedataParam.cpp +++ b/Core/Dialog/SavedataParam.cpp @@ -313,7 +313,7 @@ bool SavedataParam::HasKey(const SceUtilitySavedataParam *param) const return false; } -bool SavedataParam::Delete(SceUtilitySavedataParam* param, int saveId) { +bool SavedataParam::Delete(SceUtilitySavedataParam* param, const std::string &saveDir) { if (!param) { return false; } @@ -324,7 +324,7 @@ bool SavedataParam::Delete(SceUtilitySavedataParam* param, int saveId) { return false; } - std::string dirPath = GetSaveFilePath(param, GetSaveDir(saveId)); + std::string dirPath = GetSaveFilePath(param, saveDir); if (dirPath.size() == 0) { ERROR_LOG(Log::sceUtility, "GetSaveFilePath (%.*s) returned empty - cannot delete save directory. Might already be deleted?", (int)sizeof(param->gameName), param->gameName); return false; @@ -1281,7 +1281,7 @@ bool SavedataParam::GetList(SceUtilitySavedataParam *param) return true; } -int SavedataParam::GetFilesList(SceUtilitySavedataParam *param, u32 requestAddr) { +int SavedataParam::GetFilesList(SceUtilitySavedataParam *param, u32 requestAddr, const std::string &saveDirName) { if (!param) { return SCE_UTILITY_SAVEDATA_ERROR_RW_BAD_STATUS; } @@ -1339,7 +1339,7 @@ int SavedataParam::GetFilesList(SceUtilitySavedataParam *param, u32 requestAddr) requestPtr->bind = 1021; // Does not list directories, nor recurse into them, and ignores files not ALL UPPERCASE. - bool isCrypted = GetSaveCryptMode(param, GetSaveDirName(param, 0)) != 0; + bool isCrypted = GetSaveCryptMode(param, saveDirName) != 0; for (const auto &file : files) { if (file.type == FILETYPE_DIRECTORY) { continue; @@ -1795,6 +1795,9 @@ std::string SavedataParam::GetFilename(int idx) const } std::string SavedataParam::GetSaveDir(int idx) const { + if (!saveDataList || idx < 0 || idx >= saveDataListCount) { + return ""; + } return saveDataList[idx].saveDir; } diff --git a/Core/Dialog/SavedataParam.h b/Core/Dialog/SavedataParam.h index 65f47d7e8d..5689b51954 100644 --- a/Core/Dialog/SavedataParam.h +++ b/Core/Dialog/SavedataParam.h @@ -311,13 +311,13 @@ public: std::string GetSaveDirName(const SceUtilitySavedataParam *param, int saveId = -1) const; std::string GetSaveDir(const SceUtilitySavedataParam *param, int saveId = -1) const; std::string GetSaveDir(const SceUtilitySavedataParam *param, const std::string &saveDirName) const; - bool Delete(SceUtilitySavedataParam* param, int saveId = -1); + bool Delete(SceUtilitySavedataParam* param, const std::string &saveDir); int DeleteData(SceUtilitySavedataParam* param); int Save(SceUtilitySavedataParam* param, const std::string &saveDirName, bool secureMode = true); int Load(SceUtilitySavedataParam* param, const std::string &saveDirName, int saveId = -1, bool secureMode = true); int GetSizes(SceUtilitySavedataParam* param); bool GetList(SceUtilitySavedataParam* param); - int GetFilesList(SceUtilitySavedataParam* param, u32 requestAddr); + int GetFilesList(SceUtilitySavedataParam* param, u32 requestAddr, const std::string &saveDirName); bool GetSize(SceUtilitySavedataParam* param); int GetSaveCryptMode(const SceUtilitySavedataParam *param, const std::string &saveDirName); bool IsInSaveDataList(const std::string &saveName, int count); diff --git a/Core/HLE/sceChnnlsv.cpp b/Core/HLE/sceChnnlsv.cpp index 1ca310015b..918ca97696 100644 --- a/Core/HLE/sceChnnlsv.cpp +++ b/Core/HLE/sceChnnlsv.cpp @@ -15,6 +15,8 @@ // Official git repository and contact information can be found at // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. +#include + #include "Core/MemMapHelpers.h" #include "Core/HLE/HLE.h" #include "Core/HLE/FunctionWrappers.h" @@ -28,6 +30,9 @@ KirkState *__ChnnlsvKirkState() { return &g_kirk; } +// The savedata IO thread uses these through the sceSd functions below, while the game can call them +// (and the kirk ones) on the emulator thread. +static std::mutex g_lock; static u8 dataBuf[2048+20]; static u8 *dataBuf2 = dataBuf + 20; @@ -226,6 +231,7 @@ static int sceSdGetLastIndex(u32 addressCtx, u32 addressHash, u32 addressKey) { int sceSdMacFinal(pspChnnlsvContext1& ctx, u8* in_hash, const u8* in_key) { + std::lock_guard guard(g_lock); if(ctx.keyLength >= 17) return -1026; @@ -352,6 +358,7 @@ static int sceSdRemoveValue(u32 addressCtx, u32 addressData, int length) { int sceSdMacUpdate(pspChnnlsvContext1& ctx, const u8* data, int length) { + std::lock_guard guard(g_lock); if(ctx.keyLength >= 17) return -1026; @@ -404,6 +411,7 @@ static int sceSdCreateList(u32 ctx2Addr, int mode, int unkwn, u32 dataAddr, u32 int sceSdCipherInit(pspChnnlsvContext2& ctx2, int mode, int uknw, u8* data, const u8* cryptkey) { + std::lock_guard guard(g_lock); ctx2.mode = mode; ctx2.unkn = 1; if (uknw == 2) @@ -467,6 +475,7 @@ static int sceSdSetMember(u32 ctxAddr, u32 dataAddr, int alignedLen) { int sceSdCipherUpdate(pspChnnlsvContext2& ctx, u8* data, int alignedLen) { + std::lock_guard guard(g_lock); if (alignedLen == 0) { return 0; @@ -538,6 +547,7 @@ void Register_sceChnnlsv() static u32 sceUtilsBufferCopyWithRange(u32 outAddr, int outSize, u32 inAddr, int inSize, int cmd) { u8 *outAddress = Memory::IsValidRange(outAddr, outSize) ? Memory::GetPointerWriteUnchecked(outAddr) : nullptr; u8 *inAddress = Memory::IsValidRange(inAddr, inSize) ? Memory::GetPointerWriteUnchecked(inAddr) : nullptr; + std::lock_guard guard(g_lock); int temp = kirk_sceUtilsBufferCopyWithRange(&g_kirk, outAddress, outSize, inAddress, inSize, cmd); if (temp != 0) { ERROR_LOG(Log::sceKernel, "hleUtilsBufferCopyWithRange: Failed with %d", temp); @@ -549,6 +559,7 @@ static u32 sceUtilsBufferCopyWithRange(u32 outAddr, int outSize, u32 inAddr, int static int sceUtilsBufferCopyByPollingWithRange(u32 outAddr, int outSize, u32 inAddr, int inSize, int cmd) { u8 *outAddress = Memory::IsValidRange(outAddr, outSize) ? Memory::GetPointerWriteUnchecked(outAddr) : nullptr; u8 *inAddress = Memory::IsValidRange(inAddr, inSize) ? Memory::GetPointerWriteUnchecked(inAddr) : nullptr; + std::lock_guard guard(g_lock); return hleNoLog(kirk_sceUtilsBufferCopyWithRange(&g_kirk, outAddress, outSize, inAddress, inSize, cmd)); } diff --git a/Core/HLE/sceUtility.cpp b/Core/HLE/sceUtility.cpp index 67251a22a6..054b2b0a5e 100644 --- a/Core/HLE/sceUtility.cpp +++ b/Core/HLE/sceUtility.cpp @@ -626,6 +626,12 @@ void __UtilityDoState(PointerWrap &p) { } } +void __UtilityWaitForIO() { + if (saveDialog) { + saveDialog->WaitForIO(); + } +} + void __UtilityShutdown() { saveDialog->Shutdown(true); msgDialog->Shutdown(true); @@ -648,6 +654,7 @@ void __UtilityShutdown() { lastSaveStateVersion = -1; delete saveDialog; + saveDialog = nullptr; delete msgDialog; delete oskDialog; delete netDialog; diff --git a/Core/HLE/sceUtility.h b/Core/HLE/sceUtility.h index 2240708796..93b20b9a2a 100644 --- a/Core/HLE/sceUtility.h +++ b/Core/HLE/sceUtility.h @@ -123,6 +123,8 @@ enum class UtilityDialogType { void __UtilityInit(); void __UtilityDoState(PointerWrap &p); void __UtilityShutdown(); +// Before a savestate saves or loads memory: the savedata IO thread reads and writes it directly. +void __UtilityWaitForIO(); void UtilityDialogInitialize(UtilityDialogType type, int delayUs, int accessPriority, int graphicsPriority); void UtilityDialogShutdown(UtilityDialogType type, int delayUs, int accessPriority, int graphicsPriority); diff --git a/Core/HW/MemoryStick.cpp b/Core/HW/MemoryStick.cpp index 4e74526a37..6ba306115d 100644 --- a/Core/HW/MemoryStick.cpp +++ b/Core/HW/MemoryStick.cpp @@ -16,6 +16,8 @@ // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. #include +#include +#include #include #include #include @@ -42,8 +44,12 @@ static MemStickFatState memStickFatState; static bool memStickNeedsAssign = false; static uint64_t memStickInsertedAt = 0; static uint64_t memstickInitialFree = 0; +// The savedata IO thread asks for the free space too, so the cached use is guarded, and a write +// during the calculation leaves it stale rather than marked current. +static std::mutex memstickCurrentUseLock; static uint64_t memstickCurrentUse = 0; -static bool memstickCurrentUseValid = false; +static uint32_t memstickCurrentUseGeneration = 0; +static std::atomic memstickWriteGeneration{ 1 }; enum FreeCalcStatus { NONE, @@ -122,15 +128,21 @@ u64 MemoryStick_FreeSpace(std::string gameID) { const u64 memStickSize = flags.ReportSmallMemstick ? smallMemstickSize : (u64)g_Config.iMemStickSizeGB * 1024 * 1024 * 1024; // Assume the memory stick is only used to store savedata, for the current game only. - if (!memstickCurrentUseValid) { - Path saveFolder = GetSysDirectory(DIRECTORY_SAVEDATA); - memstickCurrentUse = ComputeSizeOfSavedataForGame(saveFolder, gameID); - memstickCurrentUseValid = true; + u64 currentUse; + { + std::lock_guard guard(memstickCurrentUseLock); + const uint32_t generation = memstickWriteGeneration; + if (memstickCurrentUseGeneration != generation) { + Path saveFolder = GetSysDirectory(DIRECTORY_SAVEDATA); + memstickCurrentUse = ComputeSizeOfSavedataForGame(saveFolder, gameID); + memstickCurrentUseGeneration = generation; + } + currentUse = memstickCurrentUse; } u64 simulatedFreeSpace = 0; - if (memstickCurrentUse < memStickSize) { - simulatedFreeSpace = memStickSize - memstickCurrentUse; + if (currentUse < memStickSize) { + simulatedFreeSpace = memStickSize - currentUse; } else if (flags.ReportSmallMemstick) { // There's more stuff in the memstick than the size we report. // This doesn't work, so we'll just have to lie. Not sure what the best way is. @@ -144,8 +156,8 @@ u64 MemoryStick_FreeSpace(std::string gameID) { // Assassin's Creed: Bloodlines fails to save if free space changes incorrectly during game. // See issue #12761 u64 realFreeSpace = 0; - if (memstickCurrentUse <= memstickInitialFree) { - realFreeSpace = memstickInitialFree - memstickCurrentUse; + if (currentUse <= memstickInitialFree) { + realFreeSpace = memstickInitialFree - currentUse; } space = std::min(simulatedFreeSpace, realFreeSpace); } else if (System_GetPropertyBool(SYSPROP_CAN_GET_FREE_SPACE_FAST) || g_Config.bReportAccurateFreeStorageSpace) { @@ -161,7 +173,7 @@ u64 MemoryStick_FreeSpace(std::string gameID) { } void MemoryStick_NotifyWrite() { - memstickCurrentUseValid = false; + memstickWriteGeneration++; } void MemoryStick_SetFatState(MemStickFatState state) { diff --git a/Core/SaveState.cpp b/Core/SaveState.cpp index 810b82f0db..b2f706b426 100644 --- a/Core/SaveState.cpp +++ b/Core/SaveState.cpp @@ -137,6 +137,9 @@ int g_screenshotFailures; // when in-game it's just not an issue. void SaveStart::DoState(PointerWrap &p) { + // Nothing may still be writing PSP memory while it's saved, or be left to write into what's loaded. + __UtilityWaitForIO(); + auto s = p.Section("SaveStart", 1, 3); if (!s) return; diff --git a/pspautotests b/pspautotests index 3dc4690e7d..2c804f5cb3 160000 --- a/pspautotests +++ b/pspautotests @@ -1 +1 @@ -Subproject commit 3dc4690e7d4f704764d23ef1ef0eb82ccc3dc30d +Subproject commit 2c804f5cb3cd97b5ca3e11242060fee73e50bc47