From d91f340c869a1496804a3a2fff493e0ae60c8b53 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 2 Jan 2023 18:21:04 -0800 Subject: [PATCH 1/3] VertexManagerBase: Move free space check to after the buffer is reset Fixes incorrect logspam when the buffer needed to be reset on flushes (which we already were doing, but 52feed04dbf9d487138944ed834bd877620eef74 moved it to after the check was made). This is https://bugs.dolphin-emu.org/issues/10312. I also converted it to an assert, as if this does happen, things are going to render incorrectly, so we want to make it obvious. --- Source/Core/VideoCommon/VertexManagerBase.cpp | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/Source/Core/VideoCommon/VertexManagerBase.cpp b/Source/Core/VideoCommon/VertexManagerBase.cpp index 88d7c47fba..85e2b48556 100644 --- a/Source/Core/VideoCommon/VertexManagerBase.cpp +++ b/Source/Core/VideoCommon/VertexManagerBase.cpp @@ -140,26 +140,11 @@ DataReader VertexManagerBase::PrepareForAdditionalData(OpcodeDecoder::Primitive } // Check for size in buffer, if the buffer gets full, call Flush() - if (!m_is_flushed && - (count > m_index_generator.GetRemainingIndices(primitive) || - count > GetRemainingIndices(primitive) || needed_vertex_bytes > GetRemainingSize())) + if (!m_is_flushed && (count > m_index_generator.GetRemainingIndices(primitive) || + count > GetRemainingIndices(primitive) || + needed_vertex_bytes > GetRemainingSize())) [[unlikely]] { Flush(); - - if (count > m_index_generator.GetRemainingIndices(primitive)) - { - ERROR_LOG_FMT(VIDEO, "Too little remaining index values. Use 32-bit or reset them on flush."); - } - if (count > GetRemainingIndices(primitive)) - { - ERROR_LOG_FMT(VIDEO, "VertexManager: Buffer not large enough for all indices! " - "Increase MAXIBUFFERSIZE or we need primitive breaking after all."); - } - if (needed_vertex_bytes > GetRemainingSize()) - { - ERROR_LOG_FMT(VIDEO, "VertexManager: Buffer not large enough for all vertices! " - "Increase MAXVBUFFERSIZE or we need primitive breaking after all."); - } } m_cull_all = cullall; @@ -182,6 +167,23 @@ DataReader VertexManagerBase::PrepareForAdditionalData(OpcodeDecoder::Primitive m_is_flushed = false; } + // Now that we've reset the buffer, there should be enough space. It's possible that we still + // won't have enough space in a few rare cases, such as vertex shader line/point expansion with a + // ton of lines in one draw command, in which case we will either need to add support for + // splitting a single draw command into multiple draws or using bigger indices. + ASSERT_MSG(VIDEO, count <= m_index_generator.GetRemainingIndices(primitive), + "VertexManager: Too few remaining index values ({} > {}). " + "32-bit indices or primitive breaking needed.", + count, m_index_generator.GetRemainingIndices(primitive)); + ASSERT_MSG(VIDEO, count <= GetRemainingIndices(primitive), + "VertexManager: Buffer not large enough for all indices! ({} > {}) " + "Increase MAXIBUFFERSIZE or we need primitive breaking after all.", + count, GetRemainingIndices(primitive)); + ASSERT_MSG(VIDEO, needed_vertex_bytes <= GetRemainingSize(), + "VertexManager: Buffer not large enough for all vertices! ({} > {}) " + "Increase MAXVBUFFERSIZE or we need primitive breaking after all.", + needed_vertex_bytes, GetRemainingSize()); + return DataReader(m_cur_buffer_pointer, m_end_buffer_pointer); } From 6c58ba353caced302f1e4e23eee0de6d59a527a2 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 2 Jan 2023 18:56:25 -0800 Subject: [PATCH 2/3] IndexGenerator: Add assertion for overflow in GetRemainingIndices This assertion is currently triggered by Pocoyo Racing (https://bugs.dolphin-emu.org/issues/13136). --- Source/Core/VideoCommon/IndexGenerator.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Source/Core/VideoCommon/IndexGenerator.cpp b/Source/Core/VideoCommon/IndexGenerator.cpp index 5d86561ffd..04068e88ec 100644 --- a/Source/Core/VideoCommon/IndexGenerator.cpp +++ b/Source/Core/VideoCommon/IndexGenerator.cpp @@ -334,6 +334,15 @@ u32 IndexGenerator::GetRemainingIndices(OpcodeDecoder::Primitive primitive) cons max_index >>= 2; // -1 is reserved for primitive restart + max_index = max_index - 1; - return max_index - m_base_index - 1; + if (m_base_index > max_index) [[unlikely]] + { + PanicAlertFmt("GetRemainingIndices would overflow; we've already written too many indices? " + "base index {} > max index {}", + m_base_index, max_index); + return 0; + } + + return max_index - m_base_index; } From cefcd9c93c2d76074c3be7a6cb96812dad082e9c Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 2 Jan 2023 19:23:31 -0800 Subject: [PATCH 3/3] IndexGenerator: Fix off-by-one in GetRemainingIndices Fixes https://bugs.dolphin-emu.org/issues/13136. --- Source/Core/VideoCommon/IndexGenerator.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/Source/Core/VideoCommon/IndexGenerator.cpp b/Source/Core/VideoCommon/IndexGenerator.cpp index 04068e88ec..03d0815a13 100644 --- a/Source/Core/VideoCommon/IndexGenerator.cpp +++ b/Source/Core/VideoCommon/IndexGenerator.cpp @@ -328,13 +328,21 @@ void IndexGenerator::AddExternalIndices(const u16* indices, u32 num_indices, u32 u32 IndexGenerator::GetRemainingIndices(OpcodeDecoder::Primitive primitive) const { - u32 max_index = USHRT_MAX; + u32 max_index = UINT16_MAX; if (g_Config.UseVSForLinePointExpand() && primitive >= OpcodeDecoder::Primitive::GX_DRAW_LINES) max_index >>= 2; - // -1 is reserved for primitive restart - max_index = max_index - 1; + // Although we reserve UINT16_MAX for primitive restart, we aren't allowed to use that as an + // actual index. But, the maximum number of vertices a game can send is UINT16_MAX, so up to + // 0xffff indices will be used by the game. These indices would be 0x0000 through 0xfffe, + // and since m_base_index gets incremented for each index used, after that m_base_index + // would be 0xffff and no indices remain. If a game uses 0xfffe vertices, assuming m_base_index + // started at 0 it would end at 0xfffe and one more index could be used. So, we do not need to + // subtract 1 from max_index to correctly reserve one index for primitive restart. + // + // Pocoyo Racing uses a draw command with 0xffff vertices, which previously caused issues; see + // https://bugs.dolphin-emu.org/issues/13136 for details. if (m_base_index > max_index) [[unlikely]] {