Skip to content

Commit ee26260

Browse files
dkulpclaude
andcommitted
fix(pru): memcpyToPRU's block loop was dead and overran short destinations
memcpy_ldnp() opened with int sz = sztotal - sztotal & 64; which parses as (sztotal - sztotal) & 64, i.e. always zero. Two consequences. Every copy fell through to the byte loop, so the ldnp/stnp path the function exists for - the optimized memcpy faults on the uncacheable PRU mappings - has never actually run. And the asm block is a do-while, so it always executed one iteration and stored 64 bytes at the destination regardless of the length asked for, writing past the end of anything shorter. Mask the low bits instead, skip the block loop when there is nothing for it, and only take it when both operands are 16 byte aligned, since an unaligned vector access to an uncacheable mapping faults. Head and tail go through the byte loop as before. Exactly sztotal bytes are written and no more. Callers are unaffected. Every existing call is either a whole number of 64 byte units or at least 64 bytes long, and for any length of 64 or more the old and new code write the same bytes over the same range - the stray store just repeated the first block. Only shorter copies change, and there the bytes that are no longer written were uninitialized stack. The BBShiftPanel comments describing the copy as spilling into the next 64 byte unit no longer hold; the padding they explain is kept, and now writes that slot outright rather than relying on the overrun to do it. Verified two ways. A/B harness over 66816 combinations of length 0-260 and source/destination alignment: the old code wrote past the end in 16368 of them, the new code in none, and neither ever got the copied bytes wrong. Removing the new byte loop and re-running the aligned, block sized cases still copies correctly, which is what shows the ldnp path is now the one doing the work rather than the test passing on the fallback. On hardware, the BBShiftString command table lands in PRU data RAM byte identical to before. BBShiftPanel and BBB48String are compile and equivalence checked only, not run on panels or a 48 string cape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 857d998 commit ee26260

2 files changed

Lines changed: 27 additions & 10 deletions

File tree

src/non-gpl/BBShiftPanel/BBShiftPanel.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ void BBShiftPanelManager::sendPanelInitPackets() {
440440
return;
441441
}
442442
// the PRU data RAM cannot take a plain memcpy (uncacheable segment,
443-
// SIGBUS on aarch64) - memcpyToPRU handles it, in 64 byte chunks
443+
// SIGBUS on aarch64) - memcpyToPRU handles it
444444
uint32_t buf[16] = { 0 };
445445
buf[0] = (uint32_t)regs.size() | (((uint32_t)rowLen) << 16);
446446
for (size_t i = 0; i < regs.size(); i++) {
@@ -2064,10 +2064,11 @@ void BBShiftPanelManager::writeFM6373SeqWord(int idx) {
20642064
uint8_t buf[192 * 2];
20652065
int len = outputRegData(0, buf, rw, gw, bw, m_numOutputSlots);
20662066
if (slotSize & 63) {
2067-
// memcpyToPRU copies in 64 byte chunks, so the write spills into the
2068-
// slot after the rotating one and has to rewrite it with its own
2069-
// (constant) value: the commit word for a five slot chip, the extra
2070-
// pre-commit word for a six slot one.
2067+
// A slot that is not a whole number of 64 byte units shares a unit
2068+
// with the one after it, so extend the write to cover that slot with
2069+
// its own (constant) value rather than leave a partial unit: the
2070+
// commit word for a five slot chip, the extra pre-commit word for a
2071+
// six slot one.
20712072
uint16_t next = seq->slots == 6 ? seq->extraWord : 0x0055;
20722073
len = outputRegData(len, buf, next, next, next, m_numOutputSlots);
20732074
}
@@ -2096,9 +2097,10 @@ void BBShiftPanelManager::writeDP3364SeqWord(int idx) {
20962097
uint8_t buf[192 * 2];
20972098
int len = outputRegData(0, buf, rw, gw, bw, m_numOutputSlots);
20982099
if (slotSize & 63) {
2099-
// memcpyToPRU copies in 64 byte chunks; pad with a second copy of the
2100-
// word so the write length is a multiple of 64. Nothing reads the
2101-
// padding - DP3364S only sends one word per frame.
2100+
// A slot that is not a whole number of 64 byte units shares a unit
2101+
// with the one after it, so pad with a second copy of the word to keep
2102+
// the write on a unit boundary. Nothing reads the padding - DP3364S
2103+
// only sends one word per frame.
21022104
len = outputRegData(len, buf, rw, gw, bw, m_numOutputSlots);
21032105
}
21042106
pru->memcpyToPRU((uint8_t*)&pruData->registers[0], buf, len);

src/util/BBBPruUtils.cpp

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -446,12 +446,27 @@ void BBBPru::memcpyToPRU(uint8_t* dst, uint8_t* src, size_t sz) {
446446
// The optimized memset and memcpy on Arm64 will segfault
447447
// when doing certain sized operations to un-cacheable
448448
// ram segments. Need to use ldnp/stnp instructions
449+
//
450+
// The block loop moves whole 64 byte units and needs 16 byte aligned operands
451+
// (the PRU memories map uncached, where an unaligned vector access faults), so
452+
// the head/tail and anything that does not qualify go through the byte loop
453+
// instead - slower, but correct for every size and alignment. Exactly sztotal
454+
// bytes are written and no more.
449455
void memcpy_ldnp(volatile unsigned char* dst, volatile unsigned char* src, int sztotal) {
450-
int sz = sztotal - sztotal & 64;
451-
456+
// NB: "sztotal - sztotal & 64" parses as "(sztotal - sztotal) & 64", i.e.
457+
// always zero, which left the block loop below writing one fixed 64 byte
458+
// store - past the end for anything shorter - and everything else crawling
459+
// through the byte loop. Mask off the low bits instead.
460+
int sz = 0;
461+
if (sztotal >= 64 && ((((uintptr_t)dst | (uintptr_t)src) & 15) == 0)) {
462+
sz = sztotal & ~63;
463+
}
452464
for (int x = sz; x < sztotal; x++) {
453465
dst[x] = src[x];
454466
}
467+
if (sz == 0) {
468+
return;
469+
}
455470
asm volatile(
456471
"NEONCopyPLD: \n"
457472
"sub %[dst], %[dst], #64 \n"

0 commit comments

Comments
 (0)