Skip to content

[4.2] fix cyclic dma setup and i2s parameters #1193

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.

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

Closed
wants to merge 3 commits into from

Conversation

HiassofT
Copy link
Contributor

@HiassofT HiassofT commented Nov 8, 2015

I found out why I got clicks with I2S soundcards after the lite DMA channel transfer size limit was bumped from 32k to 64k-4 bytes:

The frame number calculation in the cyclic DMA setup code was wrong, so fewer frames than needed were allocated and the last frame was larger than it should be.

With the old limit this bug went unnoticed as the frame length was still below the maximum size the DMA controller could handle. But with the larger limit we exceeded what the DMA controller could do.

I've also adjusted the maximum period size in the i2s driver to match the capabilities of the DMA controller. This ensures that the actual DMA frames will match the requested buffer/period layout.

This fix should go in 4.3 as well, after (or with) the huge atags PR.

I'll create a separate PR for 4.1 with backported fixes for the 2708 drivers.

@popcornmix
Copy link
Collaborator

@pelwell @notro @koalo any objections?

@pelwell
Copy link
Contributor

pelwell commented Nov 9, 2015

It looks OK as far as it goes.

I did notice that a few lines after the period_bytes_max initialisation is .buffer_bytes_max = 128 * PAGE_SIZE. Where does that limit come from?

@HiassofT
Copy link
Contributor Author

HiassofT commented Nov 9, 2015

I did notice that a few lines after the period_bytes_max initialisation is .buffer_bytes_max = 128 * PAGE_SIZE. Where does that limit come from?

That was in the original commit by @koalo and the value looks rather
sane, twice the original period_bytes_max and at 512k enough to hold
approx 1/3rd of a second at 192kHz 32bit stereo, so I didn't touch that.

The only constraints on buffer_bytes_max are that it has to be at least
max_period_size (smaller wouldn't make sense) and ideally it should be
twice the period_bytes_max or larger. Other than that it's only a
limit on how much DMA memory an application can allocate.

Large buffer sizes can be beneficial though because they can reduce
the risk of over/underruns.

If we'd like we could bump that limit and/or use a more readable
value lize SZ_512K.

One thing that's still puzzling me is that the transfer-limit /
frame-splitting code worked without causing noticable issues.

Basically ALSA expects a buffer divided into equally-sized periods
and to get a notification at each period boundary.

But when we re-arrange the buffer into other fragments/periods
than requested the notifications are no longer sent at period
boundaries.

Maybe there's some sanity-checking in the ALSA code but still
the splitting code doesn't look quite right to me. Other DMA
drivers just return an error when requesting a too large
period size, but they also don't have different limits on
lite/normal channels.

At least, with period_bytes_max now set up correctly we no
longer run into that situation.

The calculation of the number of required frames was wrong which
could lead to the last frame being longer than the requested period
length and even the maximum supported transfer size.

eg when requesting a 88208 bytes buffer with a period len of 22052
(the defaults when playing a 44.1kHz stereo 16bit file with aplay)
the code would allocate 3 frames, two with 22052 bytes and the
last one with 44104 bytes instead of the expected 4 frames with
22052 bytes each.

Signed-off-by: Matthias Reichl <[email protected]>
This reverts commit 0bbc1e2.

The underlying problem (cyclic DMA frame calculation) has been
fixed so this workaround can be reverted.

Signed-off-by: Matthias Reichl <[email protected]>
bcm2835-dma supports a maximum transfer length of 64k-4 bytes on
the lite channels. period_bytes_max should reflect this limit.

Also use SZ_xxx constants for buffer/prealloc size. The usage of
PAGE_SIZE is misleading here, the buffer sizes are not related
to the page size.

Signed-off-by: Matthias Reichl <[email protected]>
@HiassofT
Copy link
Contributor Author

superseded by #1233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants