Skip to content

fix(runtime): finalize packet-builder DMAtag QWC once at close, not on every append #173

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

Draft
smmathews wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(runtime): finalize packet-builder DMAtag QWC once at close, not on every append #173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion ps2xRuntime/src/lib/Kernel/Stubs/GS.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ namespace ps2_stubs
}

writePacketBuilderCurrent(rdram, runtime, stateAddr, currentAddr);
// Finalize the pending DMAtag QWC exactly once here; see refreshPacketBuilderPendingCount.
refreshPacketBuilderPendingCount(rdram, runtime, stateAddr);
writeGuestBytes(rdram,
runtime,
stateAddr + 8u,
Expand Down Expand Up @@ -235,6 +237,14 @@ namespace ps2_stubs
sizeof(temp));
}

// Finalizes the pending block's outer DMAtag QWC exactly once, here at
// close. Real hardware's DMAC reads a source-chain tag's QWC from memory
// at tag fetch (kick time, after close), not at append time -- and guest
// libraries that link their own packet builder terminate a block by
// ADDING their qword count onto a zero-seeded QWC field, so setting it
// eagerly per append would make that add land on a non-zero value and
// double the count. Shared by the sceGifPk* and sceVif1Pk* builder
// families, which populate the same outer-DMAtag QWC slot.
void refreshPacketBuilderPendingCount(uint8_t *rdram, PS2Runtime *runtime, uint32_t stateAddr)
{
uint32_t currentAddr = 0u;
Expand Down Expand Up @@ -264,10 +274,10 @@ namespace ps2_stubs
writeGuestU32(rdram, runtime, pendingCountAddr, countWord);
}

// Advances the write cursor only; does not touch the QWC (see refreshPacketBuilderPendingCount).
void writePacketBuilderCurrent(uint8_t *rdram, PS2Runtime *runtime, uint32_t stateAddr, uint32_t currentAddr)
{
writeGuestU32(rdram, runtime, stateAddr, currentAddr);
refreshPacketBuilderPendingCount(rdram, runtime, stateAddr);
}

uint32_t reservePacketBuilderWords(uint8_t *rdram, PS2Runtime *runtime, uint32_t stateAddr, uint32_t wordCount)
Expand Down
96 changes: 96 additions & 0 deletions ps2xTest/src/ps2_gs_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3324,6 +3324,102 @@ void register_ps2_gs_tests()
"TRXDIR payload must encode dir=0 (host-to-local)");
});

tc.Run("GIF packet-builder appends leave the pending CNT DMAtag QWC untouched until close", [](TestCase &t)
{
std::vector<uint8_t> rdram(PS2_RAM_SIZE, 0u);
PS2Runtime runtime;
R5900Context ctx{};

constexpr uint32_t stateAddr = 0x1000u;
constexpr uint32_t baseAddr = 0x2000u;

setRegU32(ctx, 4, stateAddr);
setRegU32(ctx, 5, baseAddr);
ps2_stubs::sceGifPkInit(rdram.data(), &ctx, &runtime);

// Open a CNT DMAtag at baseAddr with a zero-seeded QWC field.
setRegU32(ctx, 4, stateAddr);
setRegU32(ctx, 5, 0u); // count word
setRegU32(ctx, 6, 0u); // extra word
setRegU32(ctx, 7, 0u); // tag low; sceGifPkCnt ORs in the CNT id, QWC stays 0
ps2_stubs::sceGifPkCnt(rdram.data(), &ctx, &runtime);

uint32_t pendingCountAddr = 0u;
std::memcpy(&pendingCountAddr, rdram.data() + stateAddr + 8u, sizeof(pendingCountAddr));
t.Equals(pendingCountAddr, baseAddr,
"sceGifPkCnt should record the pending DMAtag address at stateAddr+8");

uint32_t seededTagLo = 0u;
std::memcpy(&seededTagLo, rdram.data() + baseAddr, sizeof(seededTagLo));
t.Equals(seededTagLo & 0xFFFFu, 0u, "CNT DMAtag QWC field must start zero-seeded");

// Append three A+D register qwords (16 bytes each) onto the open block.
for (uint32_t i = 0u; i < 3u; ++i)
{
setRegU32(ctx, 4, stateAddr);
setRegU32(ctx, 5, 0x50u + i); // register descriptor (value irrelevant to QWC)
setRegU64(ctx, 6, 0u); // register payload
ps2_stubs::sceGifPkAddGsAD(rdram.data(), &ctx, &runtime);
}

uint32_t currentAddr = 0u;
std::memcpy(&currentAddr, rdram.data() + stateAddr, sizeof(currentAddr));
t.Equals(currentAddr, baseAddr + 16u + 3u * 16u,
"three A+D appends should advance the write cursor by three quadwords past the DMAtag");

// The pending DMAtag's QWC must still be ZERO mid-block. Finalizing
// the count is the block-close's job (terminatePacketBuilderState, or
// a guest's own terminate), not the per-append cursor advance. An
// eager per-append refresh would show 3 here and, against a guest
// terminate that adds onto this field, double the real count.
uint32_t midBlockTagLo = 0u;
std::memcpy(&midBlockTagLo, rdram.data() + baseAddr, sizeof(midBlockTagLo));
t.Equals(midBlockTagLo, 0x10000000u,
"pending CNT DMAtag must remain id=CNT with QWC=0 mid-block; appends must not patch it");
});

tc.Run("GIF packet-builder finalizes the CNT DMAtag QWC once at close, to the true qword count", [](TestCase &t)
{
std::vector<uint8_t> rdram(PS2_RAM_SIZE, 0u);
PS2Runtime runtime;
R5900Context ctx{};

constexpr uint32_t stateAddr = 0x1000u;
constexpr uint32_t baseAddr = 0x2000u;

setRegU32(ctx, 4, stateAddr);
setRegU32(ctx, 5, baseAddr);
ps2_stubs::sceGifPkInit(rdram.data(), &ctx, &runtime);

setRegU32(ctx, 4, stateAddr);
setRegU32(ctx, 5, 0u);
setRegU32(ctx, 6, 0u);
setRegU32(ctx, 7, 0u);
ps2_stubs::sceGifPkCnt(rdram.data(), &ctx, &runtime);

for (uint32_t i = 0u; i < 3u; ++i)
{
setRegU32(ctx, 4, stateAddr);
setRegU32(ctx, 5, 0x50u + i);
setRegU64(ctx, 6, 0u);
ps2_stubs::sceGifPkAddGsAD(rdram.data(), &ctx, &runtime);
}

// Close the block through our own terminate path.
setRegU32(ctx, 4, stateAddr);
ps2_stubs::sceGifPkTerminate(rdram.data(), &ctx, &runtime);

uint32_t finalTagLo = 0u;
std::memcpy(&finalTagLo, rdram.data() + baseAddr, sizeof(finalTagLo));
t.Equals(finalTagLo, 0x10000003u,
"closing the block must finalize the CNT DMAtag to id=CNT with QWC=3, leaving the rest of the word untouched");

// stateAddr+8 (the pending-count slot) must be cleared after close.
uint32_t pendingAfterClose = 0u;
std::memcpy(&pendingAfterClose, rdram.data() + stateAddr + 8u, sizeof(pendingAfterClose));
t.Equals(pendingAfterClose, 0u, "terminate must clear the pending-count slot at stateAddr+8");
});

tc.Run("sceGsResetGraph frees its temporary GIF packet", [](TestCase &t)
{
PS2Runtime runtime;
Expand Down
18 changes: 13 additions & 5 deletions ps2xTest/src/ps2_memory_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1353,7 +1353,7 @@ void register_ps2_memory_tests()
"qwc-zero compact VIF1 chain should clear the STR bit after drain");
});

tc.Run("VIF1 packet builders keep chain qwc live before terminate", [](TestCase &t)
tc.Run("VIF1 packet builders finalize chain qwc at terminate", [](TestCase &t)
{
PS2Memory mem;
t.IsTrue(mem.initialize(), "PS2Memory initialize should succeed");
Expand Down Expand Up @@ -1394,9 +1394,17 @@ void register_ps2_memory_tests()
setRegU32(ctx, 4, kStateAddr);
ps2_stubs::sceVif1PkCloseDirectCode(rdram, &ctx, nullptr);

// Close the outer chain the way a real title does before kicking the
// DMA: the DMAC reads a CNT tag's QWC from memory when it fetches the
// tag, so the builder must finalize that field at terminate. (The QWC
// is filled in at close, not incrementally as data is appended.)
std::memset(&ctx, 0, sizeof(ctx));
setRegU32(ctx, 4, kStateAddr);
ps2_stubs::sceVif1PkTerminate(rdram, &ctx, nullptr);

uint32_t dmaTagWord = 0u;
std::memcpy(&dmaTagWord, rdram + kBaseAddr, sizeof(dmaTagWord));
t.Equals(dmaTagWord & 0xFFFFu, 1u, "live packet head qwc should reflect one qword before terminate");
t.Equals(dmaTagWord, 0x10000001u, "packet head tag should be id=CNT with QWC=1 after terminate");

std::vector<std::vector<uint8_t>> captured;
mem.setGifPacketCallback([&](const uint8_t *data, uint32_t sizeBytes)
Expand All @@ -1409,8 +1417,8 @@ void register_ps2_memory_tests()

mem.processPendingTransfers();

t.Equals(captured.size(), static_cast<size_t>(1u), "live VIF1 packet should emit one GIF packet");
t.Equals(captured[0].size(), static_cast<size_t>(16u), "live VIF1 packet should emit one qword");
t.Equals(captured.size(), static_cast<size_t>(1u), "terminated VIF1 packet should emit one GIF packet");
t.Equals(captured[0].size(), static_cast<size_t>(16u), "terminated VIF1 packet should emit one qword");

bool payloadOk = true;
for (uint32_t i = 0; i < 16u; ++i)
Expand All @@ -1421,7 +1429,7 @@ void register_ps2_memory_tests()
break;
}
}
t.IsTrue(payloadOk, "live VIF1 packet payload should reach the GIF callback");
t.IsTrue(payloadOk, "terminated VIF1 packet payload should reach the GIF callback");
});

tc.Run("VIF1 DMA chain latches terminal tag bits in CHCR", [](TestCase &t)
Expand Down