8000 More crash fixes by hrydgard · Pull Request #17399 · hrydgard/ppsspp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

More crash fixes #17399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions Common/GPU/OpenGL/GLRenderManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,11 @@ void GLPushBuffer::Flush() {
// Must be called from the render thread.
_dbg_assert_(OnRenderThread());

if (buf_ >= buffers_.size()) {
_dbg_assert_msg_(false, "buf_ somehow got out of sync: %d vs %d", (int)buf_, (int)buffers_.size());
return;
}

buffers_[buf_]. 8000 flushOffset = offset_;
if (!buffers_[buf_].deviceMemory && writePtr_) {
auto &info = buffers_[buf_];
Expand Down Expand Up @@ -589,6 +594,7 @@ bool GLPushBuffer::AddBuffer() {
if (!info.localMemory)
return false;
info.buffer = render_->CreateBuffer(target_, size_, GL_DYNAMIC_DRAW);
info.size = size_;
buf_ = buffers_.size();
buffers_.push_back(info);
return true;
Expand All @@ -605,7 +611,6 @@ void GLPushBuffer::Destroy(bool onRenderThread) {
} else {
render_->DeleteBuffer(info.buffer);
}

FreeAlignedMemory(info.localMemory);
}
buffers_.clear();
Expand Down Expand Up @@ -640,7 +645,7 @@ void GLPushBuffer::Defragment() {
_dbg_assert_msg_(!OnRenderThread(), "Defragment must not run on the render thread");

if (buffers_.size() <= 1) {
// Let's take this chance to jetison localMemory we don't need.
// Let's take this opportunity to jettison any localMemory we don't need.
for (auto &info : buffers_) {
if (info.deviceMemory) {
FreeAlignedMemory(info.localMemory);
Expand All @@ -652,18 +657,31 @@ void GLPushBuffer::Defragment() {
}

// Okay, we have more than one. Destroy them all and start over with a larger one.
size_t newSize = size_ * buffers_.size();

// When calling AddBuffer, we sometimes increase size_. So if we allocated multiple buffers in a frame,
// they won't all have the same size. Sum things up properly.
size_t newSize = 0;
for (int i = 0; i < (int)buffers_.size(); i++) {
newSize += buffers_[i].size;
}

Destroy(false);

size_ = newSize;
// Set some sane but very free limits. If there's another spike, we'll just allocate more anyway.
size_ = std::min(std::max(newSize, (size_t)65536), (size_t)(512 * 1024 * 1024));
bool res = AddBuffer();
_assert_msg_(res, "AddBuffer failed");
}

size_t GLPushBuffer::GetTotalSize() const {
size_t sum = 0;
if (buffers_.size() > 1)
sum += size_ * (buffers_.size() - 1);
// When calling AddBuffer, we sometimes increase size_. So if we allocated multiple buffers in a frame,
// they won't all have the same size. Sum things up properly.
if (buffers_.size() > 1) {
for (int i = 0; i < (int)buffers_.size() - 1; i++) {
sum += buffers_[i].size;
}
}
sum += offset_;
return sum;
}
Expand Down
1 change: 1 addition & 0 deletions Common/GPU/OpenGL/GLRenderManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ class GLPushBuffer {
uint8_t *localMemory = nullptr;
uint8_t *deviceMemory = nullptr;
size_t flushOffset = 0;
size_t size;
};

GLPushBuffer(GLRenderManager *render, GLuint target, size_t size);
Expand Down
4 changes: 3 additions & 1 deletion Common/UI/PopupScreens.h
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ class PopupMultiChoiceDynamic : public PopupMultiChoice {

protected:
void PostChoiceCallback(int num) override {
*valueStr_ = choices_[num];
if (valueStr_) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, if anything I'd be worried about a lifetime issue, i.e. the std::string passed to PopupMultiChoiceDynamic could be dead and this could be a use-after-free if the callback somehow happens late.

-[Unknown]

*valueStr_ = choices_[num];
}
}

private:
Expand Down
1 change: 1 addition & 0 deletions Common/UI/UIScreen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ void UIScreen::postRender() {
if (!draw) {
return;
}
screenManager()->getUIContext()->Flush();
draw->EndFrame();
}

Expand Down
3 changes: 2 additions & 1 deletion GPU/Common/DrawEngineCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ VertexDecoder *DrawEngineCommon::GetVertexDecoder(u32 vtype) {
if (dec)
return dec;
dec = new VertexDecoder();
_assert_(dec);
dec->SetVertexType(vtype, decOptions_, decJitCache_);
decoderMap_.Insert(vtype, dec);
return dec;
Expand Down Expand Up @@ -809,7 +810,7 @@ void DrawEngineCommon::SubmitPrim(const void *verts, const void *inds, GEPrimiti
}

// If vtype has changed, setup the vertex decoder.
if (vertTypeID != lastVType_) {
if (vertTypeID != lastVType_ || !dec_) {
dec_ = GetVertexDecoder(vertTypeID);
lastVType_ = vertTypeID;
}
Expand Down
6 changes: 5 additions & 1 deletion GPU/D3D11/DrawEngineD3D11.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,11 @@ class DrawEngineD3D11 : public DrawEngineCommon {
DecodeVerts(decoded);
}

void DispatchFlush() override { Flush(); }
void DispatchFlush() override {
if (!numDrawCalls)
return;
Flush();
}

void ClearTrackedVertexArrays() override;

Expand Down
6 changes: 5 additions & 1 deletion GPU/Directx9/DrawEngineDX9.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,11 @@ class DrawEngineDX9 : public DrawEngineCommon {
DecodeVerts(decoded);
}

void DispatchFlush() override { Flush(); }
void DispatchFlush() override {
if (!numDrawCalls)
return;
Flush();
}

protected:
// Not currently supported.
Expand Down
18 changes: 14 additions & 4 deletions GPU/GLES/DrawEngineGLES.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,20 @@ void DrawEngineGLES::Invalidate(InvalidationCallbackFlags flags) {
void DrawEngineGLES::DoFlush() {
PROFILE_THIS_SCOPE("flush");
FrameData &frameData = frameData_[render_->GetCurFrame()];

// Attempt to gather some information (asserts now upload the game name).
_assert_(render_->IsInRenderPass());
VShaderID vsid;

if (!render_->IsInRenderPass()) {
// Something went badly wrong. Try to survive by simply skipping the draw, though.
_dbg_assert_msg_(false, "Trying to DoFlush while not in a render pass. This is bad.");
// can't goto bail here, skips too many variable initializations. So let's wipe the most important stuff.
indexGen.Reset();
decodedVerts_ = 0;
numDrawCalls = 0;
vertexCountInDrawCalls_ = 0;
decodeCounter_ = 0;
dcid_ = 0;
return;
}

bool textureNeedsApply = false;
if (gstate_c.IsDirty(DIRTY_TEXTURE_IMAGE | DIRTY_TEXTURE_PARAMS) && !gstate.isModeClear() && gstate.isTextureMapEnabled()) {
Expand All @@ -265,7 +276,6 @@ void DrawEngineGLES::DoFlush() {

GEPrimitiveType prim = prevPrim_;

VShaderID vsid;
Shader *vshader = shaderManager_->ApplyVertexShader(CanUseHardwareTransform(prim), useHWTessellation_, dec_, decOptions_.expandAllWeightsToFloat, decOptions_.applySkinInDecode || !CanUseHardwareTransform(prim), &vsid);

GLRBuffer *vertexBuffer = nullptr;
Expand Down
6 changes: 5 additions & 1 deletion GPU/GLES/DrawEngineGLES.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,11 @@ class DrawEngineGLES : public DrawEngineCommon {
DoFlush();
}

void DispatchFlush() override { Flush(); }
void DispatchFlush() override {
if (!numDrawCalls)
return;
Flush();
}

GLPushBuffer *GetPushVertexBuffer() {
return frameData_[render_->GetCurFrame()].pushVertex;
Expand Down
1 change: 1 addition & 0 deletions GPU/GPUCommonHW.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class GPUCommonHW : public GPUCommon {

void Execute_TexFlush(u32 op, u32 diff);

// TODO: Have these return an error code if they jump to a bad address. If bad, stop the FastRunLoop.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just set gpuState = GPU_ERROR; and set downcount = 0; to force a recheck.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that's probably the slickest solution.

Copy link
Owner Author
@hrydgard hrydgard May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it seems we already effectively do that in many cases through UpdateState. But not in fast memory mode.

I am going to remove Execute_JumpFast and Execute_CallFast. I don't believe they are worth it.

typedef void (GPUCommonHW::*CmdFunc)(u32 op, u32 diff);

void FastRunLoop(DisplayList &list) override;
Expand Down
6 changes: 5 additions & 1 deletion GPU/Vulkan/DrawEngineVulkan.h
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,11 @@ class DrawEngineVulkan : public DrawEngineCommon {
DoFlush();
}

void DispatchFlush() override { Flush(); }
void DispatchFlush() override {
if (!numDrawCalls)
return;
Flush();
}

VkPipelineLayout GetPipelineLayout() const {
return pipelineLayout_;
Expand Down
6 changes: 4 additions & 2 deletions GPU/Vulkan/GPU_Vulkan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,10 @@ void GPU_Vulkan::SaveCache(const Path &filename) {
}

GPU_Vulkan::~GPU_Vulkan() {
VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER);
rm->DrainCompileQueue();
if (draw_) {
VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER);
rm->DrainCompileQueue();
}

SaveCache(shaderCachePath_);
// Note: We save the cache in DeviceLost
Expand Down
18 changes: 13 additions & 5 deletions UI/GameInfoCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,12 @@ bool GameInfo::LoadFromPath(const Path &gamePath) {
std::lock_guard<std::mutex> guard(lock);
// No need to rebuild if we already have it loaded.
if (filePath_ != gamePath) {
fileLoader.reset(ConstructFileLoader(gamePath));
if (!fileLoader)
return false;
{
std::lock_guard<std::mutex> guard(loaderLock);
fileLoader.reset(ConstructFileLoader(gamePath));
if (!fileLoader)
return false;
}
filePath_ = gamePath;

// This is a fallback title, while we're loading / if unable to load.
Expand All @@ -215,13 +218,18 @@ std::shared_ptr<FileLoader> GameInfo::GetFileLoader() {
// because Priority() calls GetFileLoader()... gnarly.
return fileLoader;
}

std::lock_guard<std::mutex> guard(loaderLock);
if (!fileLoader) {
fileLoader.reset(ConstructFileLoader(filePath_));
FileLoader *loader = ConstructFileLoader(filePath_);
fileLoader.reset(loader);
return fileLoader;
}
return fileLoader;
}

void GameInfo::DisposeFileLoader() {
std::lock_guard<std::mutex> guard(loaderLock);
fileLoader.reset();
}

Expand Down Expand Up @@ -365,7 +373,7 @@ class GameInfoWorkItem : public Task {
}

// In case of a remote file, check if it actually exists before locking.
if (!info_->GetFileLoader()->Exists()) {
if (!info_->GetFileLoader() || !info_->GetFileLoader()->Exists()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I think we now schedule these on multiple threads. There might be a race condition now where DisposeFileLoader() could be run by one thread while another is starting up for the very same file (i.e. with additional want flags.)

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good catch, very likely what happens..

return;
}

Expand Down
3 changes: 3 additions & 0 deletions UI/GameInfoCache.h
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ class GameInfo {
// to it.
std::mutex lock;

// Controls access to the fileLoader pointer.
std::mutex loaderLock;

std::string id;
std::string id_version;
int disc_total = 0;
Expand Down
0