GLideN64: draw from the vertices written past the ring's first segment

Since the vertex and index rings were split into fenced segments, a draw
that would straddle two segments moves to the next segment's start, and
_reserve then set the buffer's element position to 0, as it did for the
old wrap back to the start of the buffer. The draws index the ring by
that position (glDrawArrays' first vertex, the base vertex and the index
offset), so every draw after such a move read vertices from the start of
the ring instead of what it had just written. The segments did not start
on a vertex either: 8 MB / 3 is not a multiple of a RectVertex (32
bytes) or a Vertex (44). Once a game had drawn past the first 2.7 MB,
geometry tore into garbage triangles, with persistent mapped rings and
with the glMapBufferRange path alike.

RingSegments::init takes the size of what the ring holds and starts every
segment on one; _reserve sets the position to the element the placed
offset is at.

CI never went past the first segment: krom's tests draw far less than
2.7 MB in 40 frames. GLIDEN64_RING_BYTES shrinks the rings (to 48 KB at
the least, so a segment still holds the largest vertex batch), and the
GL renderers lane now holds GLideN64 to the same frames with 48 KB rings
as with the full ones, through persistent mapped rings and, with Mesa's
buffer storage hidden, through glMapBufferRange. Before this fix cube16,
rottri32 and rottri32aa draw 24 to 33 of their 40 frames differently
with the small rings, on both paths, and with either half of the fix
alone. test_gliden64_ring_segments now also runs BufferedDrawer's
cursor over the strides its rings hold: every placement must start on an
element and every draw read the elements it wrote; the old cursor fails.

.github/workflows/gl-renderers.yml runs the ring test and the GL lane on
GitHub as well, release and with AddressSanitizer and
UndefinedBehaviorSanitizer, as the GitLab job runs them.

.github/workflows/gl-renderers.yml

@@ -0,0 +1,70 @@
+# The OpenGL renderers on Mesa's llvmpipe through headless EGL, no GPU,
+# as the GitLab test-gl-renderers-linux-x64 job runs them.
+#
+# tools/lrhost_gl runs krom's RDP tests through Glide64, GLideN64, Rice
+# and glN64 (tools/ci/gl_renderers.py): each must run to the end and draw;
+# GLideN64 must draw the same frames whatever its colour buffer copy to
+# RDRAM is set to, and the same frames with its vertex rings shrunk
+# (GLIDEN64_RING_BYTES) as with the full ones, both through persistent
+# mapped rings and through glMapBufferRange. The small rings go round many
+# times in a test this short: GLideN64 used to draw torn, garbage
+# geometry once its draws moved to the ring's second segment, and the
+# full ring never got there in CI. check-core holds the ring's placement
+# and the element the draws index from to what was written
+# (test_gliden64_ring_segments). The sanitize leg runs it all again with
+# AddressSanitizer and UndefinedBehaviorSanitizer: any report fails.
+name: GL renderers
+
+on:
+  push:
+  pull_request:
+  workflow_dispatch:
+
+permissions:
+  contents: read
+
+jobs:
+  gl-renderers:
+    name: GL renderers (${{ matrix.name }})
+    runs-on: ubuntu-24.04
+    timeout-minutes: 60
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - name: release
+            sanitizer: ""
+            sanitizer_env: ""
+          - name: asan-ubsan
+            sanitizer: address,undefined
+            sanitizer_env: ASAN_OPTIONS=detect_leaks=0 UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1
+    env:
+      MAKEFLAGS: --no-print-directory
+      CORE_FLAGS: platform=unix WITH_DYNAREC=x86_64 HAVE_THR_AL=1 HAVE_PARALLEL=1 HAVE_PARALLEL_RSP=1
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Install
+        run: |
+          sudo apt-get update -qq
+          sudo apt-get install -y -qq build-essential nasm git ca-certificates curl python3 \
+            libgl-dev libegl-dev libgles-dev libegl-mesa0 libgl1-mesa-dri
+
+      - name: Ring unit test
+        run: |
+          make platform=unix test_gliden64_ring_segments
+          ./test_gliden64_ring_segments
+
+      - name: Build core and hosts
+        run: |
+          make -j$(nproc) $CORE_FLAGS SANITIZER=${{ matrix.sanitizer }}
+          make platform=unix SANITIZER=${{ matrix.sanitizer }} lrhost_gl
+
+      - name: krom ROMs cache
+        uses: actions/cache@v4
+        with:
+          path: krom-roms
+          key: krom-roms-${{ hashFiles('tools/ci/parallel_rdp_regress.py') }}
+
+      - name: GL renderers lane
+        run: env ${{ matrix.sanitizer_env }} python3 tools/ci/gl_renderers.py --core ./parallel_n64_libretro.so --host ./lrhost_gl

.gitlab-ci.yml

@@ -203,7 +203,9 @@ test-parallel-rdp-tsan-linux-x64:
 # EGL, no GPU. tools/lrhost_gl runs krom's RDP tests through Glide64,
 # GLideN64, Rice and glN64: each must run to the end and draw, and
 # GLideN64 must draw the same frames whatever its colour buffer copy to
-# RDRAM is set to. check_vtables.py fails on a class that declares a
+# RDRAM is set to, and the same with its vertex rings shrunk so they go
+# round (the draws used to read the wrong vertices past the first
+# segment). .github/workflows/gl-renderers.yml runs the same lane. check_vtables.py fails on a class that declares a
 # virtual function nobody defines (GCC 16's MinGW link failed on two such
 # Rice classes). test_glitch64_lfb_write holds Glide64's LFB write to
 # the image it was given, and test_glitch64_textures(_gles) its texture

mupen64plus-video-gliden64/src/Graphics/OpenGLContext/opengl_BufferedDrawer.cpp

@@ -1,3 +1,4 @@
+#include <stdlib.h>
 #include <Config.h>
 #include <CRC.h>
 #include "GLFunctions.h"
@@ -8,6 +9,23 @@ using namespace graphics;
 using namespace opengl;
 
 const u32 BufferedDrawer::m_bufMaxSize = 8 * 1024 * 1024; // 8 MB
+/* a segment still holds the largest vertex batch (VERTBUFF_SIZE) */
+const u32 BufferedDrawer::m_bufMinSize = 48 * 1024;
+
+/* GLIDEN64_RING_BYTES shrinks the rings so a short test goes round them
+ * many times: CI draws the same frames with a small ring as with the
+ * full one, which every draw placed at a segment start or back at 0
+ * must keep. */
+u32 BufferedDrawer::_ringSize()
+{
+	const char * env = getenv("GLIDEN64_RING_BYTES");
+	if (env == nullptr || *env == '\0')
+		return m_bufMaxSize;
+	const unsigned long bytes = strtoul(env, nullptr, 0);
+	if (bytes < m_bufMinSize)
+		return m_bufMinSize;
+	return bytes > m_bufMaxSize ? m_bufMaxSize : u32(bytes);
+}
 #ifndef GL_DEBUG
 const GLbitfield BufferedDrawer::m_bufAccessBits = GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT | GL_MAP_COHERENT_BIT;
 const GLbitfield BufferedDrawer::m_bufMapBits = GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT | GL_MAP_COHERENT_BIT;
@@ -22,10 +40,11 @@ BufferedDrawer::BufferedDrawer(const GLInfo & _glinfo, CachedVertexAttribArray *
 , m_bindBuffer(_bindBuffer)
 {
 	m_vertices.resize(VERTBUFF_SIZE);
+	const u32 ringSize = _ringSize();
 	/* Init buffers for rects */
 	glGenVertexArrays(1, &m_rectsBuffers.vao);
 	glBindVertexArray(m_rectsBuffers.vao);
-	_initBuffer(m_rectsBuffers.vbo, m_bufMaxSize);
+	_initBuffer(m_rectsBuffers.vbo, ringSize, sizeof(RectVertex));
 	m_cachedAttribArray->enableVertexAttribArray(rectAttrib::position, true);
 	m_cachedAttribArray->enableVertexAttribArray(rectAttrib::texcoord0, true);
 	m_cachedAttribArray->enableVertexAttribArray(rectAttrib::texcoord1, true);
@@ -36,8 +55,8 @@ BufferedDrawer::BufferedDrawer(const GLInfo & _glinfo, CachedVertexAttribArray *
 	/* Init buffers for triangles */
 	glGenVertexArrays(1, &m_trisBuffers.vao);
 	glBindVertexArray(m_trisBuffers.vao);
-	_initBuffer(m_trisBuffers.vbo, m_bufMaxSize);
-	_initBuffer(m_trisBuffers.ebo, m_bufMaxSize);
+	_initBuffer(m_trisBuffers.vbo, ringSize, sizeof(Vertex));
+	_initBuffer(m_trisBuffers.ebo, ringSize, sizeof(GLushort));
 	m_cachedAttribArray->enableVertexAttribArray(triangleAttrib::position, true);
 	m_cachedAttribArray->enableVertexAttribArray(triangleAttrib::color, true);
 	m_cachedAttribArray->enableVertexAttribArray(triangleAttrib::texcoord, true);
@@ -49,10 +68,10 @@ BufferedDrawer::BufferedDrawer(const GLInfo & _glinfo, CachedVertexAttribArray *
 	glVertexAttribPointer(triangleAttrib::modify, 4, GL_BYTE, GL_TRUE, sizeof(Vertex), (const GLvoid *)(offsetof(Vertex, modify)));
 }
 
-void BufferedDrawer::_initBuffer(Buffer & _buffer, GLuint _bufSize)
+void BufferedDrawer::_initBuffer(Buffer & _buffer, GLuint _bufSize, GLuint _stride)
 {
-	_buffer.size = _bufSize;
-	_buffer.ring.init(_bufSize);
+	_buffer.ring.init(_bufSize, _stride);
+	_buffer.size = _buffer.ring.size();
 	glGenBuffers(1, &_buffer.handle);
 	m_bindBuffer->bind(Parameter(_buffer.type), ObjectHandle(_buffer.handle));
 	if (m_glInfo.bufferStorage) {
@@ -108,8 +127,9 @@ GLubyte * BufferedDrawer::_reserve(Buffer & _buffer, u32 _dataSize)
 	SegmentFences fences = { _buffer.fences };
 	const u32 offset = _buffer.ring.place(u32(_buffer.offset), _dataSize, fences);
 	if (offset != u32(_buffer.offset)) {
+		/* a segment start, or 0: the draws index from there */
 		_buffer.offset = offset;
-		_buffer.pos = 0;
+		_buffer.pos = GLint(_buffer.ring.element(offset));
 	}
 	return m_glInfo.bufferStorage ? &_buffer.data[_buffer.offset] : nullptr;
 }

mupen64plus-video-gliden64/src/Graphics/OpenGLContext/opengl_BufferedDrawer.h

@@ -63,7 +63,7 @@ namespace opengl {
 			u32 modify;
 		};
 
-		void _initBuffer(Buffer & _buffer, GLuint _bufSize);
+		void _initBuffer(Buffer & _buffer, GLuint _bufSize, GLuint _stride);
 		void _updateBuffer(Buffer & _buffer, u32 _count, u32 _dataSize, const void * _data);
 		GLubyte * _reserve(Buffer & _buffer, u32 _dataSize);
 		void _commit(Buffer & _buffer, u32 _count, u32 _dataSize);
@@ -84,6 +84,8 @@ namespace opengl {
 		BufferOffsets m_rectBufferOffsets;
 
 		static const u32 m_bufMaxSize;
+		static const u32 m_bufMinSize;
+		static u32 _ringSize();
 		static const GLbitfield m_bufAccessBits;
 		static const GLbitfield m_bufMapBits;
 	};

mupen64plus-video-gliden64/src/Graphics/OpenGLContext/opengl_RingSegments.h

@@ -11,10 +11,13 @@ template <u32 Segments = 3>
 class RingSegments
 {
 public:
-	void init(u32 _size)
+	/* _align: the size of what the ring holds. The draws index the ring
+	 * in those (a vertex, an index), so every segment starts on one. */
+	void init(u32 _size, u32 _align = 1)
 	{
-		m_size = _size;
-		m_segSize = _size / Segments;
+		m_align = _align != 0 ? _align : 1;
+		m_segSize = _size / Segments / m_align * m_align;
+		m_size = m_segSize * Segments;
 		m_cur = 0;
 		for (u32 i = 0; i < Segments; ++i)
 			m_fenced[i] = false;
@@ -51,6 +54,11 @@ public:
 
 	u32 segment() const { return m_cur; }
 
+	/* the element a placed offset is at: what a draw indexes the ring by */
+	u32 element(u32 _offset) const { return _offset / m_align; }
+
+	u32 size() const { return m_size; }
+
 private:
 	u32 _seg(u32 _at) const
 	{
@@ -60,6 +68,7 @@ private:
 
 	u32 m_size = 0;
 	u32 m_segSize = 0;
+	u32 m_align = 1;
 	u32 m_cur = 0;
 	bool m_fenced[Segments];
 };

tools/ci/gl_renderers.py

@@ -17,7 +17,17 @@ across drivers, so the frames are not held to stored hashes; the lane holds them
   * GLideN64 drawing the same frames whatever its colour buffer copy to
     RDRAM is set to (Off, Sync, Async, TripleBuffer): the copy writes RDRAM
     and must not disturb what is drawn, its pixel buffers or the vertex
-    ring the draws read.
+    ring the draws read;
+  * GLideN64 drawing the same frames with its vertex rings shrunk to the
+    smallest they go (GLIDEN64_RING_BYTES) as with the full 8 MB, which
+    these tests never go round: the small rings go round many times, so
+    every draw placed at a segment start or back at 0 must index the
+    vertices it wrote. The draws used to index from 0 after a move to a
+    segment start, and the segments did not start on a vertex: geometry
+    torn into garbage triangles once a game went past the first segment.
+    Both with persistent mapped rings (GL 4.4 / ARB_buffer_storage) and,
+    with Mesa told to hide them, the rings written through
+    glMapBufferRange.
 
 CI runs the lane a second time with the core and host built with
 SANITIZER=address,undefined (glN64 used to read its texture cache after
@@ -37,13 +47,18 @@ PLUGINS = ["glide64", "gliden64", "rice", "gln64"]
 PRESENTS_NOTHING = {"gln64"}
 COPY_MODES = ["Off", "Sync", "Async", "TripleBuffer"]
 FRAMES = 40
+SMALL_RING = "49152"
+# Mesa without buffer storage: GLideN64 writes its rings through glMapBufferRange
+NO_BUFFER_STORAGE = {"MESA_GL_VERSION_OVERRIDE": "4.3",
+                     "MESA_EXTENSION_OVERRIDE": "-GL_ARB_buffer_storage"}
 
 
-def run(host, core, rom, plugin, extra, tmp):
+def run(host, core, rom, plugin, extra, tmp, env_extra=None):
     hashlog = os.path.join(tmp, "hash")
     if os.path.exists(hashlog):
         os.remove(hashlog)
     env = dict(os.environ, EGL_PLATFORM="surfaceless", LRHOST_HASHLOG=hashlog)
+    env.update(env_extra or {})
     args = [host, core, rom, str(FRAMES),
             "parallel-n64-gfxplugin=" + plugin,
             "parallel-n64-rspplugin=hle",
@@ -96,6 +111,25 @@ def main():
                     failures += 1
                     continue
                 print("%-10s gliden64 copy %-12s ok" % (rom, mode))
+            for name, mesa in (("buffer storage", {}), ("map range", NO_BUFFER_STORAGE)):
+                runs = []
+                for ring in (None, SMALL_RING):
+                    env_extra = dict(mesa)
+                    if ring is not None:
+                        env_extra["GLIDEN64_RING_BYTES"] = ring
+                    runs.append(run(a.host, a.core, path, "gliden64", [], tmp, env_extra))
+                bad = [r for r in runs if r[0] != 0 or not r[1]]
+                if bad:
+                    print("%-10s gliden64 ring %-14s FAILED (exit %d)" % (rom, name, bad[0][0]))
+                    sys.stdout.write(bad[0][2][-2000:])
+                    failures += 1
+                elif runs[0][1] != runs[1][1]:
+                    differ = sum(1 for x, y in zip(runs[0][1], runs[1][1]) if x != y)
+                    print("%-10s gliden64 ring %-14s draws other frames with a small ring (%d of %d differ)"
+                          % (rom, name, differ, len(runs[0][1])))
+                    failures += 1
+                else:
+                    print("%-10s gliden64 ring %-14s ok" % (rom, name))
 
     if failures:
         print("%d failures" % failures)

tools/test_gliden64_ring_segments.cpp

@@ -12,6 +12,16 @@
  * against the bytes of the draws not yet run, over ring sizes, allocation
  * sizes and lags, and the waits are counted: there must be none while the
  * GPU keeps within a lap less a segment. The old wrap must fail the check.
+ *
+ * The draws index the ring in vertices (or indices), BufferedDrawer
+ * keeping the element its writes are at beside the byte offset: each
+ * draw must read the element its data starts at. A move to a segment
+ * start used to set the element to 0, and the segments did not start on
+ * a vertex (8 MB / 3 is not a multiple of 32 or 44 bytes): GLideN64 drew
+ * garbage triangles once a game went past the first segment. Over the
+ * strides BufferedDrawer uses, every placement must start on an element
+ * and the element kept must be the one the placement is at; the old
+ * cursor must fail that.
  */
 #include <cstdio>
 #include <cstdlib>
@@ -100,6 +110,38 @@ static bool run(u32 _size, u32 _maxAlloc, u32 _lag, u32 &_waits)
 	return !gpu.failed;
 }
 
+/* BufferedDrawer's cursor over a ring of _stride byte elements: whether
+ * every draw read the elements it wrote */
+template <bool Old>
+static bool run_cursor(u32 _size, u32 _stride, u32 _maxCount)
+{
+	opengl::RingSegments<3> ring;
+	Gpu gpu(16, 3);
+	Ops ops = { gpu };
+	u32 offset = 0, pos = 0;
+	ring.init(_size, Old ? 1 : _stride);
+	if (!Old && ring.size() % (3 * _stride) != 0)
+		return false;
+	for (u32 k = 0; k < 4000; ++k) {
+		const u32 count = 1 + next_rand(_maxCount);
+		const u32 bytes = count * _stride;
+		gpu.run();
+		const u32 start = ring.place(offset, bytes, ops);
+		if (start != offset) {
+			offset = start;
+			pos = Old ? 0 : ring.element(start);
+		}
+		/* the draw reads from element pos: its data is at offset */
+		if (start % _stride != 0 || pos * _stride != start || start + bytes > _size)
+			return false;
+		Draw d = { start, start + bytes, false };
+		gpu.draws.push_back(d);
+		offset += bytes;
+		pos += count;
+	}
+	return true;
+}
+
 int main()
 {
 	int failures = 0;
@@ -123,6 +165,24 @@ int main()
 				}
 			}
 
+	/* RectVertex, Vertex, GLushort: what BufferedDrawer's rings hold */
+	const u32 strides[] = { 32, 44, 2 };
+	const u32 cursorSizes[] = { 49152, 50000, 1u << 20, 8u << 20 };
+	for (u32 size : cursorSizes)
+		for (u32 stride : strides) {
+			const u32 maxCount = size / 3 / stride < 256 ? size / 3 / stride : 256;
+			++runs;
+			if (!run_cursor<false>(size, stride, maxCount)) {
+				fprintf(stderr, "size %u stride %u: a draw read other elements than it wrote\n",
+				        size, stride);
+				++failures;
+			}
+		}
+	if (run_cursor<true>(8u << 20, 44, 256) || run_cursor<true>(49152, 32, 256)) {
+		fprintf(stderr, "the check passes the old cursor\n");
+		return 1;
+	}
+
 	/* the check catches the old wrap */
 	{
 		u32 waits = 0;