From bc04205646c682dcfb838412fb9a3c89e6493c6a Mon Sep 17 00:00:00 2001 From: Connor McLaughlin Date: Sat, 11 May 2024 21:04:21 +1000 Subject: [PATCH 1/3] GS/HW: Fix possible texture leak on skipped draw --- pcsx2/GS/Renderers/Common/GSDevice.cpp | 5 +++++ pcsx2/GS/Renderers/Common/GSDevice.h | 6 ++++++ pcsx2/GS/Renderers/HW/GSRendererHW.cpp | 29 ++++++++++---------------- pcsx2/GS/Renderers/HW/GSRendererHW.h | 4 ++-- 4 files changed, 24 insertions(+), 20 deletions(-) diff --git a/pcsx2/GS/Renderers/Common/GSDevice.cpp b/pcsx2/GS/Renderers/Common/GSDevice.cpp index 36a402400cf51..d0b40b11d248d 100644 --- a/pcsx2/GS/Renderers/Common/GSDevice.cpp +++ b/pcsx2/GS/Renderers/Common/GSDevice.cpp @@ -396,6 +396,11 @@ bool GSDevice::UpdateImGuiFontTexture() return true; } +void GSDevice::TextureRecycleDeleter::operator()(GSTexture* const tex) +{ + g_gs_device->Recycle(tex); +} + GSTexture* GSDevice::FetchSurface(GSTexture::Type type, int width, int height, int levels, GSTexture::Format format, bool clear, bool prefer_unused_texture) { const GSVector2i size(width, height); diff --git a/pcsx2/GS/Renderers/Common/GSDevice.h b/pcsx2/GS/Renderers/Common/GSDevice.h index ba18334f68521..38878156ee823 100644 --- a/pcsx2/GS/Renderers/Common/GSDevice.h +++ b/pcsx2/GS/Renderers/Common/GSDevice.h @@ -763,6 +763,12 @@ class GSDevice : public GSAlignedClass<32> GSHWDrawConfig::ColorMaskSelector wmask; // 0xf for all channels by default }; + struct TextureRecycleDeleter + { + void operator()(GSTexture* const tex); + }; + using RecycledTexture = std::unique_ptr; + enum BlendFactor : u8 { // HW blend factors diff --git a/pcsx2/GS/Renderers/HW/GSRendererHW.cpp b/pcsx2/GS/Renderers/HW/GSRendererHW.cpp index 63d5435e79c79..83bcc02226e2a 100644 --- a/pcsx2/GS/Renderers/HW/GSRendererHW.cpp +++ b/pcsx2/GS/Renderers/HW/GSRendererHW.cpp @@ -4586,7 +4586,8 @@ __ri static constexpr u8 EffectiveClamp(u8 clamp, bool has_region) return (clamp >= CLAMP_REGION_CLAMP && has_region) ? (clamp ^ 3) : clamp; } -__ri void GSRendererHW::EmulateTextureSampler(const GSTextureCache::Target* rt, const GSTextureCache::Target* ds, GSTextureCache::Source* tex, const TextureMinMaxResult& tmm, GSTexture*& src_copy) +__ri void GSRendererHW::EmulateTextureSampler(const GSTextureCache::Target* rt, const GSTextureCache::Target* ds, GSTextureCache::Source* tex, + const TextureMinMaxResult& tmm, GSDevice::RecycledTexture& src_copy) { // don't overwrite the texture when using channel shuffle, but keep the palette if (!m_channel_shuffle) @@ -4908,7 +4909,7 @@ __ri void GSRendererHW::EmulateTextureSampler(const GSTextureCache::Target* rt, __ri void GSRendererHW::HandleTextureHazards(const GSTextureCache::Target* rt, const GSTextureCache::Target* ds, const GSTextureCache::Source* tex, const TextureMinMaxResult& tmm, GSTextureCache::SourceRegion& source_region, - bool& target_region, GSVector2i& unscaled_size, float& scale, GSTexture*& src_copy) + bool& target_region, GSVector2i& unscaled_size, float& scale, GSDevice::RecycledTexture& src_copy) { // Detect framebuffer read that will need special handling const GSTextureCache::Target* src_target = nullptr; @@ -5051,11 +5052,11 @@ __ri void GSRendererHW::HandleTextureHazards(const GSTextureCache::Target* rt, c GSVector2i(static_cast(std::ceil(static_cast(copy_dst_offset.x) * scale)), static_cast(std::ceil(static_cast(copy_dst_offset.y) * scale))); - src_copy = src_target->m_texture->IsDepthStencil() ? + src_copy.reset(src_target->m_texture->IsDepthStencil() ? g_gs_device->CreateDepthStencil( scaled_copy_size.x, scaled_copy_size.y, src_target->m_texture->GetFormat(), false) : g_gs_device->CreateTexture( - scaled_copy_size.x, scaled_copy_size.y, 1, src_target->m_texture->GetFormat(), true); + scaled_copy_size.x, scaled_copy_size.y, 1, src_target->m_texture->GetFormat(), true)); if (!src_copy) [[unlikely]] { Console.Error("Failed to allocate %dx%d texture for hazard copy", scaled_copy_size.x, scaled_copy_size.y); @@ -5065,8 +5066,8 @@ __ri void GSRendererHW::HandleTextureHazards(const GSTextureCache::Target* rt, c } g_gs_device->CopyRect( - src_target->m_texture, src_copy, scaled_copy_range, scaled_copy_dst_offset.x, scaled_copy_dst_offset.y); - m_conf.tex = src_copy; + src_target->m_texture, src_copy.get(), scaled_copy_range, scaled_copy_dst_offset.x, scaled_copy_dst_offset.y); + m_conf.tex = src_copy.get(); } bool GSRendererHW::CanUseTexIsFB(const GSTextureCache::Target* rt, const GSTextureCache::Source* tex, @@ -5761,12 +5762,12 @@ __ri void GSRendererHW::DrawPrims(GSTextureCache::Target* rt, GSTextureCache::Ta m_conf.datm = static_cast(m_cached_ctx.TEST.DATM); // If we're doing stencil DATE and we don't have a depth buffer, we need to allocate a temporary one. - GSTexture* temp_ds = nullptr; + GSDevice::RecycledTexture temp_ds; if (m_conf.destination_alpha >= GSHWDrawConfig::DestinationAlphaMode::Stencil && m_conf.destination_alpha <= GSHWDrawConfig::DestinationAlphaMode::StencilOne && !m_conf.ds) { - temp_ds = g_gs_device->CreateDepthStencil(m_conf.rt->GetWidth(), m_conf.rt->GetHeight(), GSTexture::Format::DepthStencil, false); - m_conf.ds = temp_ds; + temp_ds.reset(g_gs_device->CreateDepthStencil(m_conf.rt->GetWidth(), m_conf.rt->GetHeight(), GSTexture::Format::DepthStencil, false)); + m_conf.ds = temp_ds.get(); } // vs @@ -5868,7 +5869,7 @@ __ri void GSRendererHW::DrawPrims(GSTextureCache::Target* rt, GSTextureCache::Ta m_conf.cb_ps.FogColor_AREF = fc.blend32<8>(m_conf.cb_ps.FogColor_AREF); } - GSTexture* tex_copy = nullptr; + GSDevice::RecycledTexture tex_copy; if (tex) { EmulateTextureSampler(rt, ds, tex, tmm, tex_copy); @@ -5983,10 +5984,7 @@ __ri void GSRendererHW::DrawPrims(GSTextureCache::Target* rt, GSTextureCache::Ta if (!ate_first_pass) { if (!m_conf.alpha_second_pass.enable) - { - CleanupDraw(true); return; - } // RenderHW always renders first pass, replace first pass with second std::memcpy(&m_conf.ps, &m_conf.alpha_second_pass.ps, sizeof(m_conf.ps)); @@ -5999,11 +5997,6 @@ __ri void GSRendererHW::DrawPrims(GSTextureCache::Target* rt, GSTextureCache::Ta m_conf.drawlist = (m_conf.require_full_barrier && m_vt.m_primclass == GS_SPRITE_CLASS) ? &m_drawlist : nullptr; g_gs_device->RenderHW(m_conf); - - if (tex_copy) - g_gs_device->Recycle(tex_copy); - if (temp_ds) - g_gs_device->Recycle(temp_ds); } // If the EE uploaded a new CLUT since the last draw, use that. diff --git a/pcsx2/GS/Renderers/HW/GSRendererHW.h b/pcsx2/GS/Renderers/HW/GSRendererHW.h index 77b2f163c128d..78a123a94483c 100644 --- a/pcsx2/GS/Renderers/HW/GSRendererHW.h +++ b/pcsx2/GS/Renderers/HW/GSRendererHW.h @@ -92,10 +92,10 @@ class GSRendererHW : public GSRenderer void CleanupDraw(bool invalidate_temp_src); void EmulateTextureSampler(const GSTextureCache::Target* rt, const GSTextureCache::Target* ds, - GSTextureCache::Source* tex, const TextureMinMaxResult& tmm, GSTexture*& src_copy); + GSTextureCache::Source* tex, const TextureMinMaxResult& tmm, GSDevice::RecycledTexture& src_copy); void HandleTextureHazards(const GSTextureCache::Target* rt, const GSTextureCache::Target* ds, const GSTextureCache::Source* tex, const TextureMinMaxResult& tmm, GSTextureCache::SourceRegion& source_region, - bool& target_region, GSVector2i& unscaled_size, float& scale, GSTexture*& src_copy); + bool& target_region, GSVector2i& unscaled_size, float& scale, GSDevice::RecycledTexture& src_copy); bool CanUseTexIsFB(const GSTextureCache::Target* rt, const GSTextureCache::Source* tex, const TextureMinMaxResult& tmm); From 416dfb67bd92c27d83b725573359440a3fdad5c0 Mon Sep 17 00:00:00 2001 From: Connor McLaughlin Date: Sat, 11 May 2024 21:04:37 +1000 Subject: [PATCH 2/3] GS/HW: Fix invalid self copy from move in DX renderers --- pcsx2/GS/Renderers/HW/GSTextureCache.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pcsx2/GS/Renderers/HW/GSTextureCache.cpp b/pcsx2/GS/Renderers/HW/GSTextureCache.cpp index b0c97df0f29b6..a80342499d775 100644 --- a/pcsx2/GS/Renderers/HW/GSTextureCache.cpp +++ b/pcsx2/GS/Renderers/HW/GSTextureCache.cpp @@ -3626,7 +3626,8 @@ bool GSTextureCache::Move(u32 SBP, u32 SBW, u32 SPSM, int sx, int sy, u32 DBP, u // DX11/12 is a bit lame and can't partial copy depth targets. We could do this with a blit instead, // but so far haven't seen anything which needs it. const GSRendererType renderer = GSGetCurrentRenderer(); - if (renderer == GSRendererType::DX11 || renderer == GSRendererType::DX12) + const bool renderer_is_directx = (renderer == GSRendererType::DX11 || renderer == GSRendererType::DX12); + if (renderer_is_directx) { if (spsm_s.depth || dpsm_s.depth) return false; @@ -3744,7 +3745,8 @@ bool GSTextureCache::Move(u32 SBP, u32 SBW, u32 SPSM, int sx, int sy, u32 DBP, u } // If the copies overlap, this is a validation error, so we need to copy to a temporary texture first. - if ((SBP == DBP) && !(GSVector4i(sx, sy, sx + w, sy + h).rintersect(GSVector4i(dx, dy, dx + w, dy + h))).rempty()) + // DirectX also can't copy to the same texture it's reading from (except potentially with enhanced barriers). + if (SBP == DBP && (!(GSVector4i(sx, sy, sx + w, sy + h).rintersect(GSVector4i(dx, dy, dx + w, dy + h))).rempty() || renderer_is_directx)) { GSTexture* tmp_texture = src->m_texture->IsDepthStencil() ? g_gs_device->CreateDepthStencil(src->m_texture->GetWidth(), src->m_texture->GetHeight(), src->m_texture->GetFormat(), false) : From 689e092e87810eebd5188efd300be293e4d06640 Mon Sep 17 00:00:00 2001 From: Connor McLaughlin Date: Sat, 11 May 2024 21:21:03 +1000 Subject: [PATCH 3/3] GS: Fix use-after-free on lost device --- pcsx2/GS/GS.cpp | 72 +++++++++++++++--------- pcsx2/GS/GS.h | 3 +- pcsx2/GS/Renderers/Common/GSRenderer.cpp | 7 +-- pcsx2/GS/Renderers/Common/GSRenderer.h | 2 - 4 files changed, 48 insertions(+), 36 deletions(-) diff --git a/pcsx2/GS/GS.cpp b/pcsx2/GS/GS.cpp index 0bc78a50f3f35..9fa2c6f8821c8 100644 --- a/pcsx2/GS/GS.cpp +++ b/pcsx2/GS/GS.cpp @@ -210,14 +210,24 @@ static void CloseGSRenderer() } } -bool GSreopen(bool recreate_device, GSRendererType new_renderer, std::optional old_config) +bool GSreopen(bool recreate_device, bool recreate_renderer, GSRendererType new_renderer, + std::optional old_config) { Console.WriteLn("Reopening GS with %s device", recreate_device ? "new" : "existing"); g_gs_renderer->Flush(GSState::GSFlushReason::GSREOPEN); - if (GSConfig.UserHacks_ReadTCOnClose) + if (recreate_device && !recreate_renderer) + { + // Keeping the renderer around, this probably means we lost the device, so toss everything. + g_gs_renderer->PurgeTextureCache(true, true, true); + g_gs_device->ClearCurrent(); + g_gs_device->PurgePool(); + } + else if (GSConfig.UserHacks_ReadTCOnClose) + { g_gs_renderer->ReadbackTextureCache(); + } std::string capture_filename; GSVector2i capture_size; @@ -232,21 +242,25 @@ bool GSreopen(bool recreate_device, GSRendererType new_renderer, std::optionalGetRegsMem(); freezeData fd = {}; - if (g_gs_renderer->Freeze(&fd, true) != 0) + std::unique_ptr fd_data; + if (recreate_renderer) { - Console.Error("(GSreopen) Failed to get GS freeze size"); - return false; - } + if (g_gs_renderer->Freeze(&fd, true) != 0) + { + Console.Error("(GSreopen) Failed to get GS freeze size"); + return false; + } - std::unique_ptr fd_data = std::make_unique(fd.size); - fd.data = fd_data.get(); - if (g_gs_renderer->Freeze(&fd, false) != 0) - { - Console.Error("(GSreopen) Failed to freeze GS"); - return false; - } + fd_data = std::make_unique(fd.size); + fd.data = fd_data.get(); + if (g_gs_renderer->Freeze(&fd, false) != 0) + { + Console.Error("(GSreopen) Failed to freeze GS"); + return false; + } - CloseGSRenderer(); + CloseGSRenderer(); + } if (recreate_device) { @@ -274,16 +288,19 @@ bool GSreopen(bool recreate_device, GSRendererType new_renderer, std::optionalDefrost(&fd) != 0) - { - Console.Error("(GSreopen) Failed to defrost"); - return false; + if (g_gs_renderer->Defrost(&fd) != 0) + { + Console.Error("(GSreopen) Failed to defrost"); + return false; + } } if (!capture_filename.empty()) @@ -693,7 +710,7 @@ void GSUpdateConfig(const Pcsx2Config::GSOptions& new_config) // Options which need a full teardown/recreate. if (!GSConfig.RestartOptionsAreEqual(old_config)) { - if (!GSreopen(true, GSConfig.Renderer, &old_config)) + if (!GSreopen(true, true, GSConfig.Renderer, &old_config)) pxFailRel("Failed to do full GS reopen"); return; } @@ -702,7 +719,7 @@ void GSUpdateConfig(const Pcsx2Config::GSOptions& new_config) if (GSConfig.SWExtraThreads != old_config.SWExtraThreads || GSConfig.SWExtraThreadsHeight != old_config.SWExtraThreadsHeight) { - if (!GSreopen(false, GSConfig.Renderer, &old_config)) + if (!GSreopen(false, true, GSConfig.Renderer, &old_config)) pxFailRel("Failed to do quick GS reopen"); return; @@ -738,7 +755,8 @@ void GSUpdateConfig(const Pcsx2Config::GSOptions& new_config) if (GSConfig.UserHacks_ReadTCOnClose) g_gs_renderer->ReadbackTextureCache(); g_gs_renderer->PurgeTextureCache(true, true, true); - g_gs_renderer->PurgePool(); + g_gs_device->ClearCurrent(); + g_gs_device->PurgePool(); } // clear out the sampler cache when AF options change, since the anisotropy gets baked into them @@ -776,7 +794,7 @@ void GSSetSoftwareRendering(bool software_renderer, GSInterlaceMode new_interlac // Config might be SW, and we're switching to HW -> use Auto. const GSRendererType renderer = (software_renderer ? GSRendererType::SW : (GSConfig.Renderer == GSRendererType::SW ? GSRendererType::Auto : GSConfig.Renderer)); - if (!GSreopen(false, renderer, std::nullopt)) + if (!GSreopen(false, true, renderer, std::nullopt)) pxFailRel("Failed to reopen GS for renderer switch."); } } @@ -1115,7 +1133,7 @@ BEGIN_HOTKEY_LIST(g_gs_hotkeys){"Screenshot", TRANSLATE_NOOP("Hotkeys", "Graphic MTGS::RunOnGSThread([new_level]() { GSConfig.HWMipmap = new_level; g_gs_renderer->PurgeTextureCache(true, false, true); - g_gs_renderer->PurgePool(); + g_gs_device->PurgePool(); }); }}, {"CycleInterlaceMode", TRANSLATE_NOOP("Hotkeys", "Graphics"), TRANSLATE_NOOP("Hotkeys", "Cycle Deinterlace Mode"), diff --git a/pcsx2/GS/GS.h b/pcsx2/GS/GS.h index 69b7dfb063387..4b42cd32ef753 100644 --- a/pcsx2/GS/GS.h +++ b/pcsx2/GS/GS.h @@ -57,7 +57,8 @@ s16 GSLookupBeforeDrawFunctionId(const std::string_view& name); s16 GSLookupMoveHandlerFunctionId(const std::string_view& name); bool GSopen(const Pcsx2Config::GSOptions& config, GSRendererType renderer, u8* basemem); -bool GSreopen(bool recreate_device, GSRendererType new_renderer, std::optional old_config); +bool GSreopen(bool recreate_device, bool recreate_renderer, GSRendererType new_renderer, + std::optional old_config); void GSreset(bool hardware_reset); void GSclose(); void GSgifSoftReset(u32 mask); diff --git a/pcsx2/GS/Renderers/Common/GSRenderer.cpp b/pcsx2/GS/Renderers/Common/GSRenderer.cpp index 8dc7535277899..46f7bbe23146d 100644 --- a/pcsx2/GS/Renderers/Common/GSRenderer.cpp +++ b/pcsx2/GS/Renderers/Common/GSRenderer.cpp @@ -75,11 +75,6 @@ void GSRenderer::Destroy() GSCapture::EndCapture(); } -void GSRenderer::PurgePool() -{ - g_gs_device->PurgePool(); -} - void GSRenderer::UpdateRenderFixes() { } @@ -518,7 +513,7 @@ bool GSRenderer::BeginPresentFrame(bool frame_skip) // Device lost, something went really bad. // Let's just toss out everything, and try to hobble on. - if (!GSreopen(true, GSGetCurrentRenderer(), std::nullopt)) + if (!GSreopen(true, false, GSGetCurrentRenderer(), std::nullopt)) { pxFailRel("Failed to recreate GS device after loss."); return false; diff --git a/pcsx2/GS/Renderers/Common/GSRenderer.h b/pcsx2/GS/Renderers/Common/GSRenderer.h index bcd65f3726af5..46f0dd6088e7f 100644 --- a/pcsx2/GS/Renderers/Common/GSRenderer.h +++ b/pcsx2/GS/Renderers/Common/GSRenderer.h @@ -44,8 +44,6 @@ class GSRenderer : public GSState virtual void UpdateRenderFixes(); - void PurgePool(); - virtual void VSync(u32 field, bool registers_written, bool idle_frame); virtual bool CanUpscale() { return false; } virtual float GetUpscaleMultiplier() { return 1.0f; }