Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Vulkan: Fix double-free in the low-memory fallback. Also, reject too-big textures #18475

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Vulkan: Fix double-free in the low-memory fallback. Also, reject too-big textures #18475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb8ad0c
Very minor cleanup in display list processing
hrydgard Dec 4, 2023
5373b8c
Fix double-free problem in "low-memory" texture fallback (Vulkan)
hrydgard Dec 4, 2023
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
21 changes: 19 additions & 2 deletions GPU/Common/TextureCacheCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "Common/TimeUtil.h"
#include "Common/Math/math_util.h"
#include "Common/GPU/thin3d.h"
#include "Core/HDRemaster.h"
#include "Core/Config.h"
#include "Core/Debugger/MemBlockInfo.h"
#include "Core/System.h"
Expand Down Expand Up @@ -147,7 +148,7 @@ void TextureCacheCommon::StartFrame() {
Clear(true);
clearCacheNextFrame_ = false;
} else {
Decimate(false);
Decimate(nullptr, false);
}
}

Expand Down Expand Up @@ -776,7 +777,7 @@ bool TextureCacheCommon::GetBestFramebufferCandidate(const TextureDefinition &en
}

// Removes old textures.
void TextureCacheCommon::Decimate(bool forcePressure) {
void TextureCacheCommon::Decimate(TexCacheEntry *exceptThisOne, bool forcePressure) {
if (--decimationCounter_ <= 0) {
decimationCounter_ = TEXCACHE_DECIMATION_INTERVAL;
} else {
Expand All @@ -789,6 +790,10 @@ void TextureCacheCommon::Decimate(bool forcePressure) {
ForgetLastTexture();
int killAgeBase = lowMemoryMode_ ? TEXTURE_KILL_AGE_LOWMEM : TEXTURE_KILL_AGE;
for (TexCache::iterator iter = cache_.begin(); iter != cache_.end(); ) {
if (iter->second.get() == exceptThisOne) {
++iter;
continue;
}
bool hasClut = (iter->second->status & TexCacheEntry::STATUS_CLUT_VARIANTS) != 0;
int killAge = hasClut ? TEXTURE_KILL_AGE_CLUT : killAgeBase;
if (iter->second->lastFrame + killAge < gpuStats.numFlips) {
Expand All @@ -806,6 +811,10 @@ void TextureCacheCommon::Decimate(bool forcePressure) {
const u32 had = secondCacheSizeEstimate_;

for (TexCache::iterator iter = secondCache_.begin(); iter != secondCache_.end(); ) {
if (iter->second.get() == exceptThisOne) {
++iter;
continue;
}
// In low memory mode, we kill them all since secondary cache is disabled.
if (lowMemoryMode_ || iter->second->lastFrame + TEXTURE_SECOND_KILL_AGE < gpuStats.numFlips) {
ReleaseTexture(iter->second.get(), true);
Expand Down Expand Up @@ -2803,6 +2812,14 @@ bool TextureCacheCommon::PrepareBuildTexture(BuildTexturePlan &plan, TexCacheEnt
plan.w = gstate.getTextureWidth(0);
plan.h = gstate.getTextureHeight(0);

if (!g_DoubleTextureCoordinates) {
// Refuse to load invalid-ly sized textures, which can happen through display list corruption.
if (plan.w > 512 || plan.h > 512) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought Tactics Ogre or something accessed a 1024x texture at some point? They have predictable behavior, and work fine as long as you use the top left 512x512 of them. I don't think I've ever seen above 1024 though.

-[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.

Yeah, I had to change this to 1024 later, for this reason (was reported in FF III though).

ERROR_LOG(G3D, "Bad texture dimensions: %dx%d", plan.w, plan.h);
return false;
}
}

bool isPPGETexture = entry->addr >= PSP_GetKernelMemoryBase() && entry->addr < PSP_GetKernelMemoryEnd();

// Don't scale the PPGe texture.
Expand Down
2 changes: 1 addition & 1 deletion GPU/Common/TextureCacheCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ class TextureCacheCommon {
virtual void Unbind() = 0;
virtual void ReleaseTexture(TexCacheEntry *entry, bool delete_them) = 0;
void DeleteTexture(TexCache::iterator it);
void Decimate(bool forcePressure = false);
void Decimate(TexCacheEntry *exceptThisOne, bool forcePressure); // forcePressure defaults to false.

void ApplyTextureFramebuffer(VirtualFramebuffer *framebuffer, GETextureFormat texFormat, RasterChannel channel);
void ApplyTextureDepal(TexCacheEntry *entry);
Expand Down
5 changes: 1 addition & 4 deletions GPU/GPUCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,9 +921,6 @@ void GPUCommon::Execute_Call(u32 op, u32 diff) {
}

void GPUCommon::DoExecuteCall(u32 target) {
// Saint Seiya needs correct support for relative calls.
const u32 retval = currentList->pc + 4;

// Bone matrix optimization - many games will CALL a bone matrix (!).
// We don't optimize during recording - so the matrix data gets recorded.
if (!debugRecording_ && Memory::IsValidRange(target, 13 * 4) && (Memory::ReadUnchecked_U32(target) >> 24) == GE_CMD_BONEMATRIXDATA) {
Expand All @@ -944,7 +941,7 @@ void GPUCommon::DoExecuteCall(u32 target) {
// TODO: UpdateState(GPUSTATE_ERROR) ?
} else {
auto &stackEntry = currentList->stack[currentList->stackptr++];
stackEntry.pc = retval;
stackEntry.pc = currentList->pc + 4;
stackEntry.offsetAddr = gstate_c.offsetAddr;
// The base address is NOT saved/restored for a regular call.
UpdatePC(currentList->pc, target - 4);
Expand Down
9 changes: 7 additions & 2 deletions GPU/Vulkan/TextureCacheVulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,11 @@ void TextureCacheVulkan::BuildTexture(TexCacheEntry *const entry) {
plan.hardwareScaling = g_Config.bTexHardwareScaling && uploadCS_ != VK_NULL_HANDLE;
plan.slowScaler = !plan.hardwareScaling || vulkan->DevicePerfClass() == PerfClass::SLOW;
if (!PrepareBuildTexture(plan, entry)) {
// We're screwed?
// We're screwed (invalid size or something, corrupt display list), let's just zap it.
if (entry->vkTex) {
delete entry->vkTex;
entry->vkTex = nullptr;
}
return;
}

Expand Down Expand Up @@ -511,7 +515,8 @@ void TextureCacheVulkan::BuildTexture(TexCacheEntry *const entry) {
WARN_LOG_REPORT(G3D, "Texture cache ran out of GPU memory; switching to low memory mode");
lowMemoryMode_ = true;
decimationCounter_ = 0;
Decimate();
Decimate(entry, true);

// TODO: We should stall the GPU here and wipe things out of memory.
// As is, it will almost definitely fail the second time, but next frame it may recover.

Expand Down