From 105592d979b333ff4d99d47a06aba964e97ea9c4 Mon Sep 17 00:00:00 2001
From: Erik Abair <erik.abair@gmail.com>
Date: Fri, 11 Feb 2022 16:37:57 -0800
Subject: [PATCH] Responses to code review comments.

---
 NV2ALog.py    |   2 +-
 Trace.py      |  61 ++++++++++++++++++++++++++++----------------------
 XboxHelper.py |  21 ++++++++---------
 kick_fifo     | Bin 92 -> 84 bytes
 kick_fifo.asm |  19 +---------------
 nv2a-trace.py |  12 ++++++++--
 6 files changed, 57 insertions(+), 58 deletions(-)

diff --git a/NV2ALog.py b/NV2ALog.py
index f58a195..c03193b 100644
--- a/NV2ALog.py
+++ b/NV2ALog.py
@@ -13,7 +13,7 @@ def __init__(self, path):
         self.path = path
 
         with open(self.path, "w", encoding="utf8") as logfile:
-            logfile.write("xemu style NV2A log from nv2a-trace.py\n\n")
+            logfile.write("pgraph method log from nv2a-trace.py\n\n")
 
     def log(self, message):
         """Append the given string to the nv2a log."""
diff --git a/Trace.py b/Trace.py
index afc86f2..a70c511 100644
--- a/Trace.py
+++ b/Trace.py
@@ -55,18 +55,28 @@ def _dump_pfb(xbox):
     return bytes(buffer)
 
 
-def _read_pgraph_rdi(xbox: Xbox, offset: int, count: int) -> bytes:
+def _read_pgraph_rdi(xbox: Xbox, offset: int, count: int):
     # FIXME: Assert pusher access is disabled
     # FIXME: Assert PGRAPH idle
 
     NV10_PGRAPH_RDI_INDEX = 0xFD400750
     NV10_PGRAPH_RDI_DATA = 0xFD400754
 
-    original_offset = ExchangeU32.exchange_u32(xbox, NV10_PGRAPH_RDI_INDEX, offset)
-
-    data = xbox.read(NV10_PGRAPH_RDI_DATA, count * 4)
-
-    xbox.write_u32(NV10_PGRAPH_RDI_INDEX, original_offset)
+    # JFR: Reading the DATA register 4 times returns X,Y,Z,W (not in that order as far
+    # as I remember), but during that time the INDEX register will stay constant, only
+    # on the final read I think it auto-increments the INDEX.
+    # It is not safe and likely incorrect to do a bulk read so this must be done
+    # individualy despite the interface communication overhead.
+    xbox.write_u32(NV10_PGRAPH_RDI_INDEX, offset)
+    data = bytearray()
+    for _ in range(count):
+        word = xbox.read_u32(NV10_PGRAPH_RDI_DATA)
+        data += struct.pack("<L", word)
+
+    # FIXME: Restore original RDI?
+    # Context from JFR: It may not be possible to restore the original index.
+    # If you touch the INDEX register, you may or may not be resetting the internal state
+    # machine.
 
     # FIXME: Assert the conditions from entry have not changed
     return data
@@ -91,6 +101,7 @@ def __init__(
         enable_texture_dumping=True,
         enable_surface_dumping=True,
         enable_raw_pixel_dumping=True,
+        enable_rdi=True,
         verbose=False,
         max_frames=0,
     ):
@@ -110,6 +121,7 @@ def __init__(
         self.enable_texture_dumping = enable_texture_dumping
         self.enable_surface_dumping = enable_surface_dumping
         self.enable_raw_pixel_dumping = enable_raw_pixel_dumping
+        self.enable_rdi = enable_rdi
         self.verbose = verbose
         self.max_frames = max_frames
 
@@ -314,14 +326,8 @@ def _dump_texture(self, index):
             return ""
 
         offset = self.xbox.read_u32(XboxHelper.PGRAPH_TEXOFFSET0 + reg_offset)
-        # FIXME: read pitch from registers instead of guessing
+        # FIXME: Use pitch from registers for linear formats.
         # FIXME: clean up associated fallback code in Texture.py
-        # There is a case in Morrowind where pitch is set to 0x8 which is clearly wrong
-        #   Texture 0 [0x03CC6000, 128 x 128 x 1 (pitch: 0x8), format 0xC]
-        #   Texture 1 [0x03C98000, 64 x 64 x 1 (pitch: 0x8), format 0x6]
-        # Double check the register for correctness and see if swizzling has something
-        # to do with it (e.g., like how NV097_SET_TEXTURE_IMAGE_RECT is only needed by
-        # linear formats).
         reg_pitch = self.xbox.read_u32(XboxHelper.PGRAPH_TEXCTL1_0 + reg_offset) >> 16
         pitch = 0
         fmt = self.xbox.read_u32(XboxHelper.PGRAPH_TEXFMT0 + reg_offset)
@@ -335,7 +341,7 @@ def _dump_texture(self, index):
         depth = 1 << depth_shift
 
         self._dbg_print(
-            "Texture %d [0x%08X, %d x %d x %d (pitch: 0x%X <ignored>), format 0x%X]"
+            "Texture %d [0x%08X, %d x %d x %d (pitch register: 0x%X), format 0x%X]"
             % (index, offset, width, height, depth, reg_pitch, fmt_color)
         )
 
@@ -390,7 +396,7 @@ def dump(img_tags, adjusted_offset, layer):
 
         return img_tags
 
-    def dump_textures(self, data, *args):
+    def dump_textures(self, _data, *_args):
         if not self.enable_texture_dumping:
             return []
 
@@ -430,18 +436,19 @@ def dump_surfaces(self, _data, *_args):
                     0x80000000 | params.depth_offset, params.depth_pitch * params.height
                 ),
             )
-        self._write(
-            "pgraph-rdi-vp-instructions.bin",
-            _read_pgraph_rdi(self.xbox, 0x100000, 136 * 4),
-        )
-        self._write(
-            "pgraph-rdi-vp-constants0.bin",
-            _read_pgraph_rdi(self.xbox, 0x170000, 192 * 4),
-        )
-        self._write(
-            "pgraph-rdi-vp-constants1.bin",
-            _read_pgraph_rdi(self.xbox, 0xCC0000, 192 * 4),
-        )
+        if self.enable_rdi:
+            self._write(
+                "pgraph-rdi-vp-instructions.bin",
+                _read_pgraph_rdi(self.xbox, 0x100000, 136 * 4),
+            )
+            self._write(
+                "pgraph-rdi-vp-constants0.bin",
+                _read_pgraph_rdi(self.xbox, 0x170000, 192 * 4),
+            )
+            self._write(
+                "pgraph-rdi-vp-constants1.bin",
+                _read_pgraph_rdi(self.xbox, 0xCC0000, 192 * 4),
+            )
 
         # FIXME: Respect anti-aliasing
         img_tags = ""
diff --git a/XboxHelper.py b/XboxHelper.py
index 5aabb47..52d33fc 100644
--- a/XboxHelper.py
+++ b/XboxHelper.py
@@ -5,7 +5,6 @@
 # pylint: disable=chained-comparison
 
 import atexit
-import struct
 from collections import namedtuple
 from typing import Optional
 from typing import Tuple
@@ -91,6 +90,9 @@ def _PGRAPH(addr):
 NV_PFIFO_CACHE1_METHOD = 0x00001800
 CACHE1_METHOD = _PFIFO(NV_PFIFO_CACHE1_METHOD)
 
+NV_PFIFO_CACHE1_DATA = 0xFD003804
+CACHE1_DATA = _PFIFO(NV_PFIFO_CACHE1_DATA)
+
 NV_PFIFO_RAMHT = 0x00000210
 RAM_HASHTABLE = _PFIFO(NV_PFIFO_RAMHT)
 
@@ -262,15 +264,14 @@ def resume_fifo_pusher(self):
         if self.delay():
             pass
 
-    def allow_populate_fifo_cache(self, wait_time=0.05):
+    def allow_populate_fifo_cache(self):
         """Temporarily enable the PFIFO pusher to populate the CACHE
 
         It is assumed that the pusher was previously paused, and it will be paused on
         exit.
         """
         self.resume_fifo_pusher()
-        if wait_time:
-            time.sleep(wait_time)
+        time.sleep(0.05)
         self.pause_fifo_pusher()
 
     def _dump_pb(self, start, end):
@@ -318,12 +319,12 @@ def print_cache_state(self, print_contents=False):
 
         if print_contents:
             print("Cache:")
-            # Each CACHE entry is a pair of 32-bit integers and there are 128 entries.
-            methods_and_data = self.xbox.read(CACHE1_METHOD, 8 * 128)
-
+            # JFR: This is intentionally read in a loop as behavior is dependent on the
+            # implementation of xboxpy's `read`.
             for i in range(128):
-                cache1_method, cache1_data = struct.unpack("<LL", methods_and_data[:8])
-                methods_and_data = methods_and_data[8:]
+
+                cache1_method = self.xbox.read_u32(CACHE1_METHOD + i * 8)
+                cache1_data = self.xbox.read_u32(CACHE1_DATA + i * 8)
 
                 output = "  [0x%02X] 0x%04X (0x%08X)" % (i, cache1_method, cache1_data)
                 pull_offset = i * 8 - pull_addr
@@ -334,7 +335,7 @@ def print_cache_state(self, print_contents=False):
                     output += " < put[%d]" % push_offset
 
                 print(output)
-        print()
+            print()
 
     def print_dma_addresses(self):
         push_addr = self.get_dma_push_address()
diff --git a/kick_fifo b/kick_fifo
index 32d1f973a50ac620ec441e5a0292e1d46faa7624..c745428983edcca1328caa9501009d49bc7fdeb8 100644
GIT binary patch
delta 14
VcmazEVeSrPnaJ+T^7{}A0{|bw1VR7+

delta 22
dcmWHEVGr$m8r&VqF_GDqBf9fV?C(P?3;<tH2#o*$

diff --git a/kick_fifo.asm b/kick_fifo.asm
index 7d3140b..1f6e7b7 100644
--- a/kick_fifo.asm
+++ b/kick_fifo.asm
@@ -9,24 +9,17 @@ bits 32
 
 kick_fifo:
 
-push ebp
-mov ebp, esp
-push ebx
-
 ; Load `expected_push` into EDX
-mov edx, dword [ebp+8]
+mov edx, dword [ebp+4]
 
 ; Avoid any other CPU stuff overwriting stuff in this risky section
 cli
 
-; if (DMA_PUSH_ADDR != expected_push) return 0xBAD0000
 mov eax, 0xBAD0000
 cmp edx, dword [DMA_PUSH_ADDR]
 jne done
 
 ; resume_fifo_pusher(xbox):
-;state = xbox.read_u32(CACHE_PUSH_STATE)
-;xbox.write_u32(CACHE_PUSH_STATE, state | DMA_PUSH_ACCESS)
 or dword [CACHE_PUSH_STATE], 0x00000001
 
 ; Do a short busy loop. Ideally this would wait forever until the push buffer becomes
@@ -40,7 +33,6 @@ wait_idle:
 dec ecx
 jz wait_failed
 
-; if ([NV_PFIFO_CACHE1_DMA_PUSH] & 0x100) goto wait_idle;
 mov ebx, dword [CACHE_PUSH_STATE]
 test ebx, 0x100
 jnz wait_idle
@@ -53,23 +45,14 @@ wait_failed:
 mov eax, 0x32555359
 
 pause_pusher:
-; pause_fifo_pusher(xbox):
-;
-;state = xbox.read_u32(CACHE_PUSH_STATE)
-;xbox.write_u32(CACHE_PUSH_STATE, state & ~DMA_PUSH_ACCESS)
 and ebx, 0xFFFFFFFE
 mov dword [CACHE_PUSH_STATE], ebx
 
-; if (DMA_PUSH_ADDR != `expected_push`) return 0xBADBAD
 cmp edx, dword [DMA_PUSH_ADDR]
 je done
 mov eax, 0xBADBAD
 
 done:
 
-pop ebx
-mov esp, ebp
-pop ebp
-
 sti
 ret 4
diff --git a/nv2a-trace.py b/nv2a-trace.py
index 3037321..417c30a 100755
--- a/nv2a-trace.py
+++ b/nv2a-trace.py
@@ -72,7 +72,7 @@ def _wait_for_stable_push_buffer_state(
         # So we pause the pusher again to validate our state
         xbox_helper.pause_fifo_pusher()
 
-        time.sleep(0.1)
+        time.sleep(1.0)
 
         if verbose:
             print("   POST RESUME")
@@ -194,7 +194,8 @@ def signal_handler(_signal, _frame):
     pixel_dumping = not args.no_pixel
     enable_texture_dumping = pixel_dumping and not args.no_texture
     enable_surface_dumping = pixel_dumping and not args.no_surface
-    enable_raw_pixel_dumping = pixel_dumping and not args.no_raw_pixel
+    enable_raw_pixel_dumping = not args.no_raw_pixel
+    enable_rdi = pixel_dumping and not args.no_rdi
 
     if args.alpha_mode == "both":
         alpha_mode = Trace.Tracer.ALPHA_MODE_BOTH
@@ -214,6 +215,7 @@ def signal_handler(_signal, _frame):
         enable_texture_dumping=enable_texture_dumping,
         enable_surface_dumping=enable_surface_dumping,
         enable_raw_pixel_dumping=enable_raw_pixel_dumping,
+        enable_rdi=enable_rdi,
         verbose=args.verbose,
         max_frames=args.max_flip,
     )
@@ -277,6 +279,12 @@ def _parse_args():
             action="store_true",
         )
 
+        parser.add_argument(
+            "--no-rdi",
+            help="Disable dumping of RDI.",
+            action="store_true",
+        )
+
         parser.add_argument(
             "--alpha-mode",
             default="drop",