SPI Block maxsize #124
SPI Block maxsize #124
Conversation
fschrempf
force-pushed
the
xfer_block_size
branch
from
d8641a6 to
7a9e005
Compare
|
Rebased to current master |
| PyObject *obj; | ||
| PyObject *seq; | ||
| const uint32_t spi_blk_maxsize = get_xfer_block_size(); | ||
| uint8_t buf[spi_blk_maxsize]; |
There was a problem hiding this comment.
It looks like you are using a VLA (variable length array) here. While this might be supported and probably works, it is something that a lot of people think is bad practice for various reasons. I think one of the following options might be more suited:
- Use dynamic buffer allocation
- Use a static buffer with size
XFER_MAX_BLOCK_SIZEas we don't need to care too much about saving memory
There was a problem hiding this comment.
You're right about VLAs—this does drop C89 support.
My goals were to:
- avoid oversized fixed stack buffers while keeping portability,
- avoid malloc/free in the I/O hot path (too slow on Raspberry Pi ↔ SoC transfers).
With a capped VLA:
- stack use matches the actual need (typically <4KB, max 65KB),
- growth is safely bounded,
- no malloc overhead, best performance.
If strict C89 compliance is required, VLA needs to be addressed, but IIRC it's not possible to just request XFER_MAX_BLOCK_SIZE of stack on some devices.
There was a problem hiding this comment.
Good points!
but IIRC it's not possible to just request XFER_MAX_BLOCK_SIZE of stack on some devices.
I can't really judge that. I thought it would be safe to allocate a 64 KiB array on the stack on any Linux system, but that might be wrong. If we think it is not safe to do so we should probably avoid this case altogether. No matter if it is caused by a fixed stack buffer or through a VLA that can potentially have the same size.
avoid malloc/free in the I/O hot path (too slow on Raspberry Pi ↔ SoC transfers).
Same here, I can't really judge if the malloc in the hot path would impact the performance in practice. The xfer functions already use dynamic buffers for RX and TX. But I agree that avoiding it would be nice.
What about using a buffer on the heap that is tied to the lifetime of the spidev Python object? This buffer could then later be used by all transfer functions (writebytes, xfer, etc.)? This would result in:
- no malloc overhead in hot path
- no risk of hitting any stack limits
- no C89 compliance issues (if this is a problem at all)
- no problems due to VLAs apparently being optional in C11 and therefore not having a guarantee that compilers will support them in the future
| PyObject *seq; | ||
| const uint32_t spi_blk_maxsize = get_xfer_block_size(); | ||
| uint8_t buf[spi_blk_maxsize]; | ||
| char wrmsg_text[spi_blk_maxsize]; |
There was a problem hiding this comment.
Why does the message string buffer have the same size as the data buffer? This looks wrong.
There was a problem hiding this comment.
Yes you are correct. Previously it was set to 4096, which come to think about it, might be a bit over sized. Perhaps
#define ERR_MSG_SIZE 256
char wrmsg_text[ERR_MSG_SIZE];
is enough?
HI @fschrempf Thanks for taking a look at this. It was a few years ago I made this, and the reason was some performance issues I had between the host and device. |
2 participants
The SPI driver should, if possible, use as large buffer as possible. Currently, only
xfer3is adhering to the requested max blk size specified in/sys/module/spidev/parameters/bufsizbut is sending it in chunks. For all otherxferfunctions, the block size is limited to 4096.I believe that
xfer3was introduced to fix this, however, it is still limited to 4096 chunks.This PR aims to clarify
xfer3behavior and allow for arbitrary xfer buffer size.