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.