misc: rp1-pio: two small fixes and support for cyclic output DMA

[hansverkuil]

The first two patches are small fixes, and the third adds support for cyclic DMA output. Tested and verified with pio-la and spdif encoding/decoding.

[pelwell]

This looks sensible to me. Just in case there are other use cases in future I'd prefer cyclic_tx to be just called tx, even though it is currently only used in the cyclic case. I need to warn you about #7664 which replaces your second patch, and also moves the initialisation of the cyclic property even earlier.

[hansverkuil]

It makes sense to rename cyclic_tx to tx. I'll wait until #7664 is merged, then I'll rebase and update those issues.

checkpatch complains about the Assisted-by tag: do you want me to drop those and just mention it in the commit log? Or not mention the AI use at all? I'm not sure what the guidelines are for AI use in the RPi kernel repo.

[pelwell]

I've merged #7664 now. I'm not bothered about the checkpatch complaint - the 7.2 checkpatch is happy with it - it's up to you whether you credit the AI.

[hansverkuil]

Rebased and renamed cyclic_tx to tx, which was a good change as it made the code a bit easier to understand.

[pelwell]

I'm going to paste Claude's review of the second commit verbatim, because I'll miss some nuance if I rewrite it:

1. The start of the first period is lost (real bug). For TX, the cyclic descriptor is submitted and started at config time, before anything has
   been written. The TX DREQ is asserted whenever the FIFO has space, even with the SM disabled. So the DMAC immediately reads the first
   FIFO-depth of words (plus anything it prefetches internally) from period 0, which at that point is still zeros from dma_alloc_coherent. When
   the user then fills period 0, the DMA is already past those words. The output therefore starts with about 8 zero words instead of the user's
   first words, and no underflow is reported. This contradicts the new comment about filling the whole buffer before enabling the SM. Fix: for
   TX, delay dmaengine_submit()/dma_async_issue_pending() until the first write, or until the buffer has been primed.
2. The writer doesn't catch up properly after an underflow. The callback always calls up(), so if the writer falls behind (head > tail), the
   semaphore count rises above buf_count. The writer then writes periods the DMA has already sent. When tail catches up to head, the next write
   lands in the period the DMA is currently reading, so the data is torn. It then settles one period short of full lead. It would be better to
   have the writer jump tail to head + 1 when it detects an underflow, and replace the counting semaphore with a wait on tail - head < buf_count.
3. Merged callbacks throw the counts off (affects RX too). dw-axi-dmac reports each finished period through vchan_cyclic_callback(), which just
   sets vc->cyclic and schedules a tasklet. If two periods finish before the tasklet runs, the driver gets only one callback. After that,
   head_idx and the semaphore are permanently one behind the hardware. For TX, each miss silently reduces how far ahead the writer can get, and
   underflow detection works from the wrong head. With small periods under load this is realistic. Using the residue from dmaengine_tx_status()
   to find the real position would be more robust. This problem already exists in the RX path, but TX makes it more visible.
4. The underflow check is best effort. The callback runs in a tasklet, possibly well after the DMA has started the next period. A write that
   completes in that gap isn't flagged even though stale data went out. A comment should say so.
5. The new guard in rp1_pio_sm_xfer_data() is a separate fix. It also protects cyclic RX: before this commit, the kernel API on a cyclic channel
   would index bufs[tail % buf_count] when only bufs[0] is allocated. It deserves a mention in the commit message, or its own commit.
6. Minor points:
   • dma_wmb() doesn't order against anything the DMAC can see, since the DMA never reads tail_idx. Its comment ("Pair with the DMAC's reads") is
     misleading. It does no harm, but it doesn't achieve anything either.
   • A zero-byte TX write now blocks and sends a full period of zeros, whereas RX rejects !bytes. Pick one behaviour on purpose.
   • Returning -EPIPE even though the data was queued is documented in the uapi header. Still, a caller that retries on error will send that
     period twice, so the wording could state that explicitly.
   • The re-indented dmaengine_prep_dma_cyclic() arguments still aren't aligned with the opening parenthesis (checkpatch --strict).

Existing RX issue, not caused by these commits: after -EOVERFLOW, rp1_pio_sm_rx_user() returns without advancing tail_idx. Meanwhile head - tail
keeps growing, so every later read also returns -EOVERFLOW. The stream can't recover without reconfiguring.

[pelwell]

I think the !buf_size check is effectively covered by the conditions on lines 1050-1051.

[hansverkuil]

Actually, no: this check also catches the corner case where both buf_size and buf_count are 0, which is a special corner case of the non-cyclic DMA, but for cyclic DMA this makes no sense.

[pelwell]

But that's caught by || buf_count < 2), no?

[hansverkuil]

Pushed new code (I let my AI work on the issues your AI found...). Let's see what your AI now thinks of these two patches.

Tomorrow I plan to do some actual testing for these corner cases, just to verify what all these AIs have been doing!

[pelwell]

Straight from the horse's mouth:

48904ca — fix cyclic RX flow control and overflow recovery

1. The merged-callback problem is not actually fixed. Replacing the semaphore with a waitqueue changes nothing here. If two periods finish before
   the tasklet runs, vchan_cyclic_callback() still produces only one callback, so head_idx still goes up by one and stays one behind the
   hardware for good. The semaphore count and head_idx were already in step with each other; both are wrong by the same amount. The commit
   message and the new comments claim a fix that isn't there. A real fix has to read the hardware position: dw-axi-dmac supports device_tx_status
   with burst-granularity residue (dw-axi-dmac-platform.c:1848). The callback could calculate the current period from dmaengine_tx_status()
   instead of doing head_idx++.
2. The overflow check and the recovery point are both one period out. head_idx counts completed periods, so the DMA is currently writing period
   head % N. When head - tail == N, tail % N is that same period. The existing > buf_count test therefore allows a read of the period being
   overwritten. It should be >= buf_count. For the same reason, the resync tail = head - buf_count lands on the period being overwritten. The
   oldest period that is still intact is head - buf_count + 1.
3. Minor: the DMA can overwrite the period while copy_to_user() is running. Checking head/tail again after the copy, seqlock-style, and returning
   -EOVERFLOW if it moved would detect that.

12f2a2d — cyclic DMA for output

Improvements: starting the DMA on the first write fixes the zero words at the start. The writer now skips ahead after an underflow instead of
tearing a period. Zero-byte writes are rejected, the best-effort nature of the check is documented, and the uapi note about retrying on -EPIPE is
good.

1. New bug: the first write always reports underflow. On the first write, head_idx == tail_idx == 0, so underflow = (int)(head - tail) >= 0 is
   true. The write goes to period 1, returns -EPIPE, and then dma_async_issue_pending() starts the DMA on period 0, which is still all zeros.
   That's exactly what the deferred start was meant to prevent. The underflow test needs dma->issued && in front of it.
2. The commit message overclaims. "so a write can't land on a period the DMA has already started reading" isn't true while the merged-callback
   lag (RX point 1) is unfixed. A lagging head_idx makes the writer think the DMA is further back than it is. Fixing the position tracking in the
   callback fixes both directions.
3. Minor points:
   • The new guard in rp1_pio_sm_xfer_data() is still not mentioned in the commit message.
   • dma_wmb() still has nothing to order against. The new comment ("before the DMA engine wraps back around") describes something the barrier
     doesn't provide.
   • On underflow, the DMA resends old data from earlier laps, not silence. That's worth stating in the uapi comment.
   • Nothing serialises cyclic tx_user/rx_user against themselves, so two threads using the same fd would race on tail_idx and issued. The
     non-cyclic path has the same problem, so this is low priority.

[hansverkuil]

So, another attempt: the first three patches are small, patches 4 and 5 are PIO RX improvements/fixes, and the final patch adds cyclic TX support.

It's been fairly extensively tested against all sorts of corner cases, and had one AI code review the other AI (if nothing else, this was educational!).

[pelwell]

I could almost merge this as is, but I'll give you a chance to consider making changes.

There's quite a bit of overlap between the later commits, with the last commit fixing some deficiencies in the circular buffer handling in the earlier commits. This isn't great for the reviewer, but I'm more interested in the end result.

We're definitely into diminishing returns territory with the AI reviews, but the final commit caused quite a bit of feedback from Claude/Opus 5.5, which I'll just paste here verbatim:

• Index wrapping should be its own fix, earlier in the series. The idx_wrap / rp1_pio_idx_add() change also fixes non-cyclic transfers and cyclic RX. With plain unsigned counters, % buf_count maps to the wrong slot
  after 2³² periods unless buf_count is a power of two. For cyclic RX with 128-byte periods at about 100 MB/s, that happens after about an hour and a half. As it stands, commits 4 and 5 ship that bug and this
  commit quietly fixes it.
• cb_cyclic_seq isn't needed. wait_event_interruptible() already re-checks its condition on every wake-up, and the for loop recomputes the lead anyway. The seq term only adds a loop pass that ends up back in the
  same wait.
• last_head_idx / last_head_idx_valid look redundant. After any write the lead is at most buf_count. So at the next write, lead ≤ buf_count − Δhead, and Δhead ≥ buf_count already forces lead ≤ 0, which is the
• The commit message disagrees with the minimum size. It says the DMAC "does lap the ring immediately after being issued". For TX, residue comes from CH_SAR, i.e. how far the DMAC has fetched, and fetching can only
  run ahead by the DMAC FIFO plus the PIO FIFO. That amount is exactly what RP1_PIO_CYCLIC_MIN_BUF_SIZE is meant to stay above, so a ring of three or more periods shouldn't be lapped.
  • If it really does lap, then "assume nothing" for TX undercounts by a full ring from the very first callback. The writer would then sit one lap behind without noticing.
  • I'd ask Hans which is true. I suspect the real problem was "full ring vs empty ring look the same modulo buf_count", not a DMAC lap.
• The uapi documentation is missing two things:
  • TX output doesn't start until buf_count periods have been written, so a short stream never comes out.
  • Once the writer stops, the ring replays forever until the state machine (SM) is stopped.
• RX regression risk: the 128-byte minimum now applies to RX as well. Any existing cyclic RX user with smaller periods would start getting -EINVAL, and that change is hidden in a "TX support" commit.
• Minor points:
  • The dma_wmb() comment is misleading. Nothing later publishes the data to the DMAC, so the barrier only orders writes and doesn't make anything "visible before the wrap".
  • issued = false is set twice in config.
  • The buf_size → size rename is unrelated churn.
  • RP1_PIO_CYCLIC_MIN_BUF_SIZE sits between the flag's comment and the flag's #define.

[hansverkuil]

Good comments actually. I'm closing to dropping the whole AI approach to this and do it manually. While AI works really well for pio-la, for this driver it has been quite disappointing. But, as mentioned before, it's been educational...

[pelwell]

I agree - it's very much a mixed blessing.

[hansverkuil]

Change of plans: I'll make a new branch to fix rx issues first, then later I'll work on the tx part. I think I'll do this manually rather than relying on AI.

[hansverkuil]

OK, I redid the series, rewriting the last two patches manually. While there are more things that could be done, I think this is in decent shape.

One thing that should be considered for this driver is to serialize the ioctls, at least to some extent. It's a bit out of scope of this series.

[pelwell]

I'm being shown some concerns about the lack of serialisation, but putting that aside I see three issues which I think are worth considering:

1. Dropping the dma_rmb() in RX looks wrong (commit 6):

• The reader's only check of head_idx is the unlocked wait_event condition. If that is already true, wait_event_interruptible() returns without any barrier.
• The following spin_lock is an acquire. It stops later loads moving before the lock, but it doesn't keep the earlier head_idx load ahead of the buffer loads.
• The buffer address is known from tail_idx before head_idx is checked. The CPU can therefore load the period early, from memory mapped as non-cacheable but still speculatable, before the DMA has finished writing it.
• Fix: put a dma_rmb() back between deciding the period is ready and copy_to_user().

2. The RX wait can briefly see data that isn't there (commit 6):

• The head_idx > tail_idx condition reads both indices without the lock. The callback's wrap step subtracts wrap_around from both together.
• If the reader loads head before that subtraction and tail after it, the condition is true while the ring is actually empty. Nothing re-checks it under the lock, so the reader returns the period the DMA is still writing.
• It only happens at the wrap point (about 2³¹ periods) and the window is tiny, but the fix is one line: re-check head_idx > tail_idx inside the locked section and go back to waiting if it fails.

3. The callback can run while rp1_pio_sm_dma_free() is tearing down (commits 6 and 7):

• dma_chan_terminate_all() in dw-axi-dmac doesn't clear vc->cyclic or stop the vchan tasklet. A callback that is already pending can therefore still run after the terminate.
• It can run after dma_free() has set buf_count = 0, so buf_size / buf_count and % buf_count divide by zero. On arm64 that returns 0 rather than trapping, but it is still undefined behaviour.
• Fix: use dmaengine_terminate_sync(), or have the callback return early once dma_not_running is set.