From c30ec4793705b50f5652cd9bd42411e76b574a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 12:11:38 +0100 Subject: [PATCH 1/7] ZipFileReader: Better implementation of Rewind --- Common/File/VFS/ZipFileReader.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Common/File/VFS/ZipFileReader.cpp b/Common/File/VFS/ZipFileReader.cpp index 0ffa8fd187..b51b7f4793 100644 --- a/Common/File/VFS/ZipFileReader.cpp +++ b/Common/File/VFS/ZipFileReader.cpp @@ -290,22 +290,26 @@ VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference, size } void ZipFileReader::Rewind(VFSOpenFile *vfsOpenFile) { - ZipFileReaderOpenFile *openFile = (ZipFileReaderOpenFile *)vfsOpenFile; - // Close and re-open. - zip_fclose(openFile->zf); - openFile->zf = zip_fopen_index(zip_file_, openFile->reference->zi, 0); + ZipFileReaderOpenFile *file = (ZipFileReaderOpenFile *)vfsOpenFile; + _assert_(file); + _dbg_assert_(file->zf != nullptr); + zip_fseek(file->zf, 0, SEEK_SET); } size_t ZipFileReader::Read(VFSOpenFile *vfsOpenFile, void *buffer, size_t length) { ZipFileReaderOpenFile *file = (ZipFileReaderOpenFile *)vfsOpenFile; + _assert_(file); + _dbg_assert_(file->zf != nullptr); return zip_fread(file->zf, buffer, length); } void ZipFileReader::CloseFile(VFSOpenFile *vfsOpenFile) { ZipFileReaderOpenFile *file = (ZipFileReaderOpenFile *)vfsOpenFile; + _assert_(file); _dbg_assert_(file->zf != nullptr); zip_fclose(file->zf); file->zf = nullptr; + vfsOpenFile = nullptr; lock_.unlock(); delete file; } From cf678a1aa6649b7662e9b25248cc187c26a6629e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 12:17:05 +0100 Subject: [PATCH 2/7] JIT: Allow more blocks to be stored in the cache. Also, add margin to the IsFull check. --- Common/Net/Resolve.cpp | 1 + Core/MIPS/JitCommon/JitBlockCache.cpp | 5 ++--- Core/MIPS/JitCommon/JitBlockCache.h | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Common/Net/Resolve.cpp b/Common/Net/Resolve.cpp index bcdd1ad567..7ec678150b 100644 --- a/Common/Net/Resolve.cpp +++ b/Common/Net/Resolve.cpp @@ -71,6 +71,7 @@ void Shutdown() #endif } +// NOTE: Due to the nature of getaddrinfo, this can block indefinitely. Not good. bool DNSResolve(const std::string &host, const std::string &service, addrinfo **res, std::string &error, DNSType type) { #if PPSSPP_PLATFORM(SWITCH) // Force IPv4 lookups. diff --git a/Core/MIPS/JitCommon/JitBlockCache.cpp b/Core/MIPS/JitCommon/JitBlockCache.cpp index 021ee7b16c..c917eb05d1 100644 --- a/Core/MIPS/JitCommon/JitBlockCache.cpp +++ b/Core/MIPS/JitCommon/JitBlockCache.cpp @@ -91,8 +91,8 @@ bool JitBlock::ContainsAddress(u32 em_address) const { } bool JitBlockCache::IsFull() const { - // -10 to safely leave space for some proxy blocks, which we don't check before we allocate (not ideal, but should work). - return num_blocks_ >= MAX_NUM_BLOCKS - 10; + // Subtract some amount to safely leave space for some proxy blocks, which we don't check before we allocate (not ideal, but should be enough). + return num_blocks_ >= MAX_NUM_BLOCKS - 512; } void JitBlockCache::Init() { @@ -218,7 +218,6 @@ void JitBlockCache::ProxyBlock(u32 rootAddress, u32 startAddress, u32 size, cons void JitBlockCache::AddBlockMap(int block_num) { const JitBlock &b = blocks_[block_num]; // Convert the logical address to a physical address for the block map - // Yeah, this'll work fine for PSP too I think. u32 pAddr = b.originalAddress & 0x1FFFFFFF; block_map_[std::make_pair(pAddr + 4 * b.originalSize, pAddr)] = block_num; } diff --git a/Core/MIPS/JitCommon/JitBlockCache.h b/Core/MIPS/JitCommon/JitBlockCache.h index 16457b0b06..6cf805a6e5 100644 --- a/Core/MIPS/JitCommon/JitBlockCache.h +++ b/Core/MIPS/JitCommon/JitBlockCache.h @@ -227,7 +227,8 @@ private: std::pair blockMemRanges_[3]; enum { - MAX_NUM_BLOCKS = 65536*2 + // Where does this number come from? + MAX_NUM_BLOCKS = 65536 * 4 }; }; From 56a164a14e76be78be1a9dcb7f3bfbacaf205c88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 12:43:18 +0100 Subject: [PATCH 3/7] Handle one more error case in presentation --- GPU/Common/FramebufferManagerCommon.cpp | 16 ++++++++++++---- GPU/Common/FramebufferManagerCommon.h | 2 +- GPU/GPUCommonHW.cpp | 1 + GPU/GPUCommonHW.h | 1 + UI/EmuScreen.cpp | 10 +++++++--- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/GPU/Common/FramebufferManagerCommon.cpp b/GPU/Common/FramebufferManagerCommon.cpp index 305aff0bf4..5d8399e48c 100644 --- a/GPU/Common/FramebufferManagerCommon.cpp +++ b/GPU/Common/FramebufferManagerCommon.cpp @@ -1523,7 +1523,7 @@ Draw::Texture *FramebufferManagerCommon::MakePixelTexture(const u8 *srcPixels, G return tex; } -void FramebufferManagerCommon::DrawFramebufferToOutput(const u8 *srcPixels, int srcStride, GEBufferFormat srcPixelFormat) { +bool FramebufferManagerCommon::DrawFramebufferToOutput(const u8 *srcPixels, int srcStride, GEBufferFormat srcPixelFormat) { textureCache_->ForgetLastTexture(); shaderManager_->DirtyLastShader(); @@ -1531,7 +1531,7 @@ void FramebufferManagerCommon::DrawFramebufferToOutput(const u8 *srcPixels, int float v0 = 0.0f, v1 = 1.0f; Draw::Texture *pixelsTex = MakePixelTexture(srcPixels, srcPixelFormat, srcStride, 512, 272); if (!pixelsTex) - return; + return false; int uvRotation = useBufferedRendering_ ? g_Config.iInternalScreenRotation : ROTATION_LOCKED_HORIZONTAL; OutputFlags flags = g_Config.iDisplayFilter == SCALE_LINEAR ? OutputFlags::LINEAR : OutputFlags::NEAREST; @@ -1552,6 +1552,8 @@ void FramebufferManagerCommon::DrawFramebufferToOutput(const u8 *srcPixels, int DiscardFramebufferCopy(); currentRenderVfb_ = nullptr; + + return true; } void FramebufferManagerCommon::SetViewport2D(int x, int y, int w, int h) { @@ -1633,8 +1635,14 @@ void FramebufferManagerCommon::CopyDisplayToOutput(bool reallyDirty) { if (!vfb) { if (Memory::IsValidAddress(fbaddr)) { // The game is displaying something directly from RAM. In GTA, it's decoded video. - DrawFramebufferToOutput(Memory::GetPointerUnchecked(fbaddr), displayStride_, displayFormat_); - // This effectively calls presentation_->NotifyPresent(); + // If successful, this effectively calls presentation_->NotifyPresent(); + if (!DrawFramebufferToOutput(Memory::GetPointerUnchecked(fbaddr), displayStride_, displayFormat_)) { + if (useBufferedRendering_) { + // Bind and clear the backbuffer. This should be the first time during the frame that it's bound. + draw_->BindFramebufferAsRenderTarget(nullptr, { Draw::RPAction::CLEAR, Draw::RPAction::CLEAR, Draw::RPAction::CLEAR }, "CopyDisplayToOutput_DrawError"); + } + presentation_->NotifyPresent(); + } return; } else { DEBUG_LOG(Log::FrameBuf, "Found no FBO to display! displayFBPtr = %08x", fbaddr); diff --git a/GPU/Common/FramebufferManagerCommon.h b/GPU/Common/FramebufferManagerCommon.h index e8debc8d7b..1a3c9ead6a 100644 --- a/GPU/Common/FramebufferManagerCommon.h +++ b/GPU/Common/FramebufferManagerCommon.h @@ -356,7 +356,7 @@ public: void ReadFramebufferToMemory(VirtualFramebuffer *vfb, int x, int y, int w, int h, RasterChannel channel, Draw::ReadbackMode mode); void DownloadFramebufferForClut(u32 fb_address, u32 loadBytes); - void DrawFramebufferToOutput(const u8 *srcPixels, int srcStride, GEBufferFormat srcPixelFormat); + bool DrawFramebufferToOutput(const u8 *srcPixels, int srcStride, GEBufferFormat srcPixelFormat); void DrawPixels(VirtualFramebuffer *vfb, int dstX, int dstY, const u8 *srcPixels, GEBufferFormat srcPixelFormat, int srcStride, int width, int height, RasterChannel channel, const char *tag); diff --git a/GPU/GPUCommonHW.cpp b/GPU/GPUCommonHW.cpp index d4438501aa..9dcbc9cc3a 100644 --- a/GPU/GPUCommonHW.cpp +++ b/GPU/GPUCommonHW.cpp @@ -535,6 +535,7 @@ void GPUCommonHW::CopyDisplayToOutput(bool reallyDirty) { shaderManager_->DirtyLastShader(); + // after this, render pass is active. framebufferManager_->CopyDisplayToOutput(reallyDirty); gstate_c.Dirty(DIRTY_TEXTURE_IMAGE); diff --git a/GPU/GPUCommonHW.h b/GPU/GPUCommonHW.h index 7e3671cff6..56d51218e8 100644 --- a/GPU/GPUCommonHW.h +++ b/GPU/GPUCommonHW.h @@ -9,6 +9,7 @@ public: GPUCommonHW(GraphicsContext *gfxCtx, Draw::DrawContext *draw); ~GPUCommonHW(); + // This can fail, and if so no render pass is active. void CopyDisplayToOutput(bool reallyDirty) override; void DoState(PointerWrap &p) override; void DeviceLost() override; diff --git a/UI/EmuScreen.cpp b/UI/EmuScreen.cpp index 496ebf1050..2fe797db2e 100644 --- a/UI/EmuScreen.cpp +++ b/UI/EmuScreen.cpp @@ -1397,7 +1397,9 @@ ScreenRenderFlags EmuScreen::render(ScreenRenderMode mode) { } Draw::BackendState state = draw->GetCurrentBackendState(); - _assert_msg_(!state.valid || state.passes >= 1, "skipB: %d sw: %d mode: %d back: %d tag: %s", (int)skipBufferEffects, (int)g_Config.bSoftwareRendering, (int)mode, (int)g_Config.iGPUBackend, screenManager()->topScreen()->tag()); + if (state.valid) { + _assert_msg_(state.passes >= 1, "skipB: %d sw: %d mode: %d back: %d tag: %s", (int)skipBufferEffects, (int)g_Config.bSoftwareRendering, (int)mode, (int)g_Config.iGPUBackend, screenManager()->topScreen()->tag()); + } // Need to make sure the UI texture is available, for "darken". screenManager()->getUIContext()->BeginFrame(); @@ -1523,8 +1525,10 @@ ScreenRenderFlags EmuScreen::render(ScreenRenderMode mode) { Draw::BackendState state = draw->GetCurrentBackendState(); - // We allow if !state.valid, that means it's not the Vulkan backend. - _assert_msg_(!state.valid || state.passes >= 1, "skipB: %d sw: %d mode: %d back: %d bound: %d", (int)skipBufferEffects, (int)g_Config.bSoftwareRendering, (int)mode, (int)g_Config.iGPUBackend, (int)framebufferBound); + // State.valid just states whether the passes parameter has a meaningful value. + if (state.valid) { + _assert_msg_(state.passes >= 1, "skipB: %d sw: %d mode: %d back: %d bound: %d", (int)skipBufferEffects, (int)g_Config.bSoftwareRendering, (int)mode, (int)g_Config.iGPUBackend, (int)framebufferBound); + } screenManager()->getUIContext()->BeginFrame(); From fb4c1fb63fd4e622a34c08699393cd5d26118d29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 16:42:17 +0100 Subject: [PATCH 4/7] Reporting: Use g_DownloadManager instead of using a custom thread solution --- Common/Net/HTTPRequest.cpp | 3 ++- Core/Reporting.cpp | 44 +++++++++++--------------------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/Common/Net/HTTPRequest.cpp b/Common/Net/HTTPRequest.cpp index d606a6c946..28fa7332f7 100644 --- a/Common/Net/HTTPRequest.cpp +++ b/Common/Net/HTTPRequest.cpp @@ -9,7 +9,8 @@ namespace http { -Request::Request(RequestMethod method, const std::string &url, std::string_view name, bool *cancelled, ProgressBarMode mode) : method_(method), url_(url), name_(name), progress_(cancelled), progressBarMode_(mode) { +Request::Request(RequestMethod method, const std::string &url, std::string_view name, bool *cancelled, ProgressBarMode mode) + : method_(method), url_(url), name_(name), progress_(cancelled), progressBarMode_(mode) { INFO_LOG(Log::HTTP, "HTTP %s request: %s (%.*s)", RequestMethodToString(method), url.c_str(), (int)name.size(), name.data()); progress_.callback = [=](int64_t bytes, int64_t contentLength, bool done) { diff --git a/Core/Reporting.cpp b/Core/Reporting.cpp index 5e01300afa..75a4770475 100644 --- a/Core/Reporting.cpp +++ b/Core/Reporting.cpp @@ -308,30 +308,14 @@ namespace Reporting bool SendReportRequest(const char *uri, const std::string &data, const std::string &mimeType, Buffer *output = NULL) { - http::Client http; - net::RequestProgress progress(&pendingMessagesDone); - Buffer theVoid = Buffer::Void(); - - http.SetUserAgent(StringFromFormat("PPSSPP/%s", PPSSPP_GIT_VERSION)); - - if (output == nullptr) - output = &theVoid; - - const char *serverHost = ServerHostname(); - if (!serverHost) - return false; - - if (http.Resolve(serverHost, ServerPort())) { - int result = -1; - if (http.Connect()) { - result = http.POST(http::RequestParams(uri), data, mimeType, output, &progress); - http.Disconnect(); - } - - return result >= 200 && result < 300; - } else { - return false; - } + char url[1024]; + const char *hostname = ServerHostname(); + int port = ServerPort(); + snprintf(url, sizeof(url), "http://%s:%d%s", hostname, port, uri); + g_DownloadManager.AsyncPostWithCallback(url, data, mimeType, http::ProgressBarMode::NONE, [=](http::Request &req) { + serverWorking = !req.Failed(); + }); + return true; } std::string StripTrailingNull(const std::string &str) @@ -514,12 +498,8 @@ namespace Reporting } } - int Process(int pos) - { - SetCurrentThreadName("Report"); - - AndroidJNIThreadContext jniContext; // destructor detaches - + // Not the thread func, but called from it. + int Process(int pos) { Payload &payload = payloadBuffer[pos]; Buffer output; @@ -671,7 +651,7 @@ namespace Reporting return -1; } - int ProcessPending() { + int SendPendingReportsThread() { SetCurrentThreadName("Report"); std::unique_lock guard(pendingMessageLock); @@ -714,7 +694,7 @@ namespace Reporting pendingMessageCond.notify_one(); if (!messageThread.joinable()) { - messageThread = std::thread(ProcessPending); + messageThread = std::thread(SendPendingReportsThread); } } From 5244538b77a1832d9cdc6fe03af9e87178a8f419 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 16:59:53 +0100 Subject: [PATCH 5/7] Reporting: Delete the now unused extra threads, fix up compat reports. --- Common/LogReporting.cpp | 4 +- Core/Reporting.cpp | 159 ++++++++-------------------------------- UI/ReportScreen.cpp | 2 +- 3 files changed, 35 insertions(+), 130 deletions(-) diff --git a/Common/LogReporting.cpp b/Common/LogReporting.cpp index 7c6ac3b440..9f41875aa0 100644 --- a/Common/LogReporting.cpp +++ b/Common/LogReporting.cpp @@ -67,7 +67,7 @@ void ReportMessage(const char *message, ...) { return; const int MESSAGE_BUFFER_SIZE = 65536; - char temp[MESSAGE_BUFFER_SIZE]; + char *temp = new char [MESSAGE_BUFFER_SIZE]; va_list args; va_start(args, message); @@ -76,6 +76,8 @@ void ReportMessage(const char *message, ...) { va_end(args); messageCallback(message, temp); + + delete[] temp; } void ReportMessageFormatted(const char *message, const char *formatted) { diff --git a/Core/Reporting.cpp b/Core/Reporting.cpp index 75a4770475..d49ab0cdb5 100644 --- a/Core/Reporting.cpp +++ b/Core/Reporting.cpp @@ -84,22 +84,13 @@ namespace Reporting static int lastModuleVersion; static uint32_t lastModuleCrc; - static std::mutex pendingMessageLock; - static std::condition_variable pendingMessageCond; - static std::deque pendingMessages; - static bool pendingMessagesDone = false; - static std::thread messageThread; - static std::thread compatThread; - - enum class RequestType - { + enum class RequestType { NONE, MESSAGE, COMPAT, }; - struct Payload - { + struct Payload { RequestType type; std::string string1; std::string string2; @@ -107,8 +98,6 @@ namespace Reporting int int2; int int3; }; - static Payload payloadBuffer[PAYLOAD_BUFFER_SIZE]; - static int payloadBufferPos = 0; static std::mutex crcLock; static std::condition_variable crcCond; @@ -240,8 +229,7 @@ namespace Reporting } // Returns the full host (e.g. report.ppsspp.org:80.) - std::string ServerHost() - { + std::string ServerHost() { if (g_Config.sReportHost.compare("default") == 0) return ""; return g_Config.sReportHost; @@ -306,20 +294,15 @@ namespace Reporting return ++spamProtectionCount >= SPAM_LIMIT; } - bool SendReportRequest(const char *uri, const std::string &data, const std::string &mimeType, Buffer *output = NULL) - { + static void SendReportRequest(const char *uri, const std::string &data, const std::string &mimeType, std::function callback) { char url[1024]; const char *hostname = ServerHostname(); int port = ServerPort(); snprintf(url, sizeof(url), "http://%s:%d%s", hostname, port, uri); - g_DownloadManager.AsyncPostWithCallback(url, data, mimeType, http::ProgressBarMode::NONE, [=](http::Request &req) { - serverWorking = !req.Failed(); - }); - return true; + g_DownloadManager.AsyncPostWithCallback(url, data, mimeType, http::ProgressBarMode::NONE, callback); } - std::string StripTrailingNull(const std::string &str) - { + std::string StripTrailingNull(const std::string &str) { size_t pos = str.find_first_of('\0'); if (pos != str.npos) return str.substr(0, pos); @@ -369,14 +352,12 @@ namespace Reporting bool MessageAllowed(); void SendReportMessage(const char *message, const char *formatted); - void Init() - { + void Init() { // New game, clean slate. spamProtectionCount = 0; ResetCounts(); everUnsupported = false; currentSupported = IsSupported(); - pendingMessagesDone = false; Reporting::SetupCallbacks(&MessageAllowed, &SendReportMessage); lastModuleName.clear(); @@ -384,24 +365,14 @@ namespace Reporting lastModuleCrc = 0; } - void Shutdown() - { - pendingMessageLock.lock(); - pendingMessagesDone = true; - pendingMessageCond.notify_one(); - pendingMessageLock.unlock(); - if (compatThread.joinable()) - compatThread.join(); - if (messageThread.joinable()) - messageThread.join(); + void Shutdown() { PurgeCRC(); // Just so it can be enabled in the menu again. Init(); } - void DoState(PointerWrap &p) - { + void DoState(PointerWrap &p) { const int LATEST_VERSION = 1; auto s = p.Section("Reporting", 0, LATEST_VERSION); if (!s || s < LATEST_VERSION) { @@ -413,8 +384,7 @@ namespace Reporting Do(p, everUnsupported); } - void UpdateConfig() - { + void UpdateConfig() { currentSupported = IsSupported(); if (!currentSupported && PSP_IsInited()) everUnsupported = true; @@ -498,9 +468,7 @@ namespace Reporting } } - // Not the thread func, but called from it. - int Process(int pos) { - Payload &payload = payloadBuffer[pos]; + int Process(const Payload &payload) { Buffer output; MultipartFormDataEncoder postdata; @@ -509,21 +477,18 @@ namespace Reporting AddConfigInfo(postdata); AddGameplayInfo(postdata); - switch (payload.type) - { + switch (payload.type) { case RequestType::MESSAGE: // TODO: Add CRC? postdata.Add("message", payload.string1); postdata.Add("value", payload.string2); // We tend to get corrupted data, this acts as a very primitive verification check. postdata.Add("verify", payload.string1 + payload.string2); - payload.string1.clear(); - payload.string2.clear(); postdata.Finish(); - serverWorking = true; - if (!SendReportRequest("/report/message", postdata.ToString(), postdata.GetMimeType())) - serverWorking = false; + SendReportRequest("/report/message", postdata.ToString(), postdata.GetMimeType(), [=](http::Request &req) { + serverWorking = !req.Failed(); + }); break; case RequestType::COMPAT: @@ -536,31 +501,28 @@ namespace Reporting postdata.Add("crc", StringFromFormat("%08x", RetrieveCRCUnlessPowerSaving(PSP_CoreParameter().fileToStart))); postdata.Add("suggestions", payload.string1 != "perfect" && payload.string1 != "playable" ? "1" : "0"); AddScreenshotData(postdata, Path(payload.string2)); - payload.string1.clear(); - payload.string2.clear(); postdata.Finish(); serverWorking = true; - if (!SendReportRequest("/report/compat", postdata.ToString(), postdata.GetMimeType(), &output)) { - serverWorking = false; - } else { - std::string result; - output.TakeAll(&result); + SendReportRequest("/report/compat", postdata.ToString(), postdata.GetMimeType(), [=](http::Request &req) { + if (req.Failed()) { + serverWorking = false; + return; + } + serverWorking = true; + std::string result; + req.buffer().TakeAll(&result); lastCompatResult.clear(); if (result.empty() || result[0] == '0') serverWorking = false; else if (result[0] != '1') SplitString(result, '\n', lastCompatResult); - } + }); break; - case RequestType::NONE: break; } - - payload.type = RequestType::NONE; - return 0; } @@ -623,56 +585,12 @@ namespace Reporting g_Config.sReportHost = "default"; } - ReportStatus GetStatus() - { + ReportStatus GetStatus() { if (!serverWorking) return ReportStatus::FAILING; - - for (int pos = 0; pos < PAYLOAD_BUFFER_SIZE; ++pos) - { - if (payloadBuffer[pos].type != RequestType::NONE) - return ReportStatus::BUSY; - } - return ReportStatus::WORKING; } - int NextFreePos() - { - int start = payloadBufferPos % PAYLOAD_BUFFER_SIZE; - do - { - int pos = payloadBufferPos++ % PAYLOAD_BUFFER_SIZE; - if (payloadBuffer[pos].type == RequestType::NONE) - return pos; - } - while (payloadBufferPos != start); - - return -1; - } - - int SendPendingReportsThread() { - SetCurrentThreadName("Report"); - - std::unique_lock guard(pendingMessageLock); - while (!pendingMessagesDone) { - while (!pendingMessages.empty() && !pendingMessagesDone) { - int pos = pendingMessages.front(); - pendingMessages.pop_front(); - - guard.unlock(); - Process(pos); - guard.lock(); - } - if (pendingMessagesDone) { - break; - } - pendingMessageCond.wait(guard); - } - - return 0; - } - bool MessageAllowed() { if (!IsEnabled() || CheckSpamLimited()) return false; @@ -680,33 +598,20 @@ namespace Reporting } void SendReportMessage(const char *message, const char *formatted) { - int pos = NextFreePos(); - if (pos == -1) - return; + // MessageAllowed is checked first. - Payload &payload = payloadBuffer[pos]; + Payload payload{}; payload.type = RequestType::MESSAGE; payload.string1 = message; payload.string2 = formatted; - std::lock_guard guard(pendingMessageLock); - pendingMessages.push_back(pos); - pendingMessageCond.notify_one(); - - if (!messageThread.joinable()) { - messageThread = std::thread(SendPendingReportsThread); - } + Process(payload); } - void ReportCompatibility(const char *compat, int graphics, int speed, int gameplay, const std::string &screenshotFilename) - { + void ReportCompatibility(const char *compat, int graphics, int speed, int gameplay, const std::string &screenshotFilename) { if (!IsEnabled()) return; - int pos = NextFreePos(); - if (pos == -1) - return; - - Payload &payload = payloadBuffer[pos]; + Payload payload{}; payload.type = RequestType::COMPAT; payload.string1 = compat; payload.string2 = screenshotFilename; @@ -714,9 +619,7 @@ namespace Reporting payload.int2 = speed; payload.int3 = gameplay; - if (compatThread.joinable()) - compatThread.join(); - compatThread = std::thread(Process, pos); + Process(payload); } std::vector CompatibilitySuggestions() { diff --git a/UI/ReportScreen.cpp b/UI/ReportScreen.cpp index 6d65a67003..e698abf6d1 100644 --- a/UI/ReportScreen.cpp +++ b/UI/ReportScreen.cpp @@ -168,7 +168,7 @@ ReportScreen::ReportScreen(const Path &gamePath) // unused gamePath, after remo ScreenRenderFlags ReportScreen::render(ScreenRenderMode mode) { _dbg_assert_(mode & ScreenRenderMode::FIRST); - _dbg_assert_(mode & ScreenRenderMode::TOP); + // _dbg_assert_(mode & ScreenRenderMode::TOP); if (mode & ScreenRenderMode::TOP) { // We do this after render because we need it to be within the frame (so the screenshot works). From e51f7c3a84164175062fd4190371007ff9b04acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 17:10:44 +0100 Subject: [PATCH 6/7] Remove a potential race condition --- Core/Reporting.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/Core/Reporting.cpp b/Core/Reporting.cpp index d49ab0cdb5..fb0b895b38 100644 --- a/Core/Reporting.cpp +++ b/Core/Reporting.cpp @@ -68,9 +68,6 @@ namespace Reporting // Internal limiter on number of requests per instance. static u32 spamProtectionCount = 0; - // Temporarily stores a reference to the hostname. - static std::string lastHostname; - // Keeps track of whether a harmful setting was ever used. static bool everUnsupported = false; // Support is cached here to avoid checking it on every single request. @@ -231,7 +228,7 @@ namespace Reporting // Returns the full host (e.g. report.ppsspp.org:80.) std::string ServerHost() { if (g_Config.sReportHost.compare("default") == 0) - return ""; + return "report.ppsspp.org"; return g_Config.sReportHost; } @@ -255,8 +252,7 @@ namespace Reporting } // Returns only the hostname part (e.g. "report.ppsspp.org".) - static const char *ServerHostname() - { + static std::string ServerHostname() { if (!IsEnabled()) return NULL; @@ -265,10 +261,9 @@ namespace Reporting // This means there's no port number - it's already the hostname. if (length == host.npos) - lastHostname = host; + return host; else - lastHostname = host.substr(0, length); - return lastHostname.c_str(); + return host.substr(0, length); } // Returns only the port part (e.g. 80) as an int. @@ -296,9 +291,9 @@ namespace Reporting static void SendReportRequest(const char *uri, const std::string &data, const std::string &mimeType, std::function callback) { char url[1024]; - const char *hostname = ServerHostname(); + std::string hostname = ServerHostname(); int port = ServerPort(); - snprintf(url, sizeof(url), "http://%s:%d%s", hostname, port, uri); + snprintf(url, sizeof(url), "http://%s:%d%s", hostname.c_str(), port, uri); g_DownloadManager.AsyncPostWithCallback(url, data, mimeType, http::ProgressBarMode::NONE, callback); } From 6a7435341e472b9a514f64eaacc3517f427a13c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 28 Oct 2024 17:18:08 +0100 Subject: [PATCH 7/7] Some reporting cleanups --- Core/ELF/ElfReader.cpp | 3 ++- Core/MIPS/MIPSInt.cpp | 9 +++++---- Core/Reporting.cpp | 2 +- GPU/GPUCommon.cpp | 3 +-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Core/ELF/ElfReader.cpp b/Core/ELF/ElfReader.cpp index 7adceca25d..614f43a5ab 100644 --- a/Core/ELF/ElfReader.cpp +++ b/Core/ELF/ElfReader.cpp @@ -164,7 +164,8 @@ bool ElfReader::LoadRelocations(const Elf32_Rel *rels, int numRelocs) { // It appears the PSP takes any relocation that is not a HI16. if (t_type != R_MIPS_LO16) { if (t_type != R_MIPS_16) { - // Let's play it safe for now and skip. We've only seen this type. + // Let's play it safe for now and skip. We've only seen this type. + // These exists in some popular games like Assassin's Creed: Bloodlines and GTA: VCS: (https://report.ppsspp.org/logs/kind/1187) ERROR_LOG_REPORT(Log::Loader, "ELF relocation HI16/%d pair (instead of LO16) at %08x / %08x", t_type, addr, corrLoAddr); continue; } else { diff --git a/Core/MIPS/MIPSInt.cpp b/Core/MIPS/MIPSInt.cpp index ade4f32cd1..5a04ff2147 100644 --- a/Core/MIPS/MIPSInt.cpp +++ b/Core/MIPS/MIPSInt.cpp @@ -101,7 +101,7 @@ namespace MIPSInt int func = (op >> 16) & 0x1F; // Let's only report this once per run to be safe from impacting perf. - static bool reportedAlignment = false; + static bool loggedAlignment = false; // It appears that a cache line is 0x40 (64) bytes, loops in games // issue the cache instruction at that interval. @@ -120,9 +120,10 @@ namespace MIPSInt int size = 0x40 + (addr & 0x3F); MIPSComp::jit->InvalidateCacheAt(alignedAddr, size); // Using a bool to avoid locking/etc. in case it's slow. - if (!reportedAlignment && (addr & 0x3F) != 0) { - WARN_LOG_REPORT(Log::JIT, "Unaligned icache invalidation of %08x (%08x + %d) at PC=%08x", addr, R(rs), imm, PC); - reportedAlignment = true; + if (!loggedAlignment && (addr & 0x3F) != 0) { + // These are seen exclusively in Lego games, and are really no big deal. Reporting removed. + WARN_LOG(Log::JIT, "Unaligned icache invalidation of %08x (%08x + %d) at PC=%08x", addr, R(rs), imm, PC); + loggedAlignment = true; } if (alignedAddr <= PC + 4 && alignedAddr + size >= PC - 4) { // This is probably rare so we don't use a static bool. diff --git a/Core/Reporting.cpp b/Core/Reporting.cpp index fb0b895b38..41077ddc20 100644 --- a/Core/Reporting.cpp +++ b/Core/Reporting.cpp @@ -254,7 +254,7 @@ namespace Reporting // Returns only the hostname part (e.g. "report.ppsspp.org".) static std::string ServerHostname() { if (!IsEnabled()) - return NULL; + return ""; std::string host = ServerHost(); size_t length = ServerHostnameLength(); diff --git a/GPU/GPUCommon.cpp b/GPU/GPUCommon.cpp index 4305917502..6f024b25dd 100644 --- a/GPU/GPUCommon.cpp +++ b/GPU/GPUCommon.cpp @@ -1349,8 +1349,7 @@ void GPUCommon::FlushImm() { } void GPUCommon::Execute_Unknown(u32 op, u32 diff) { - if ((op & 0xFFFFFF) != 0) - WARN_LOG_REPORT_ONCE(unknowncmd, Log::G3D, "Unknown GE command : %08x ", op); + // Do nothing. We used to report here, but we're confident we have them all so no need to report unknown. } void GPUCommon::FastLoadBoneMatrix(u32 target) {