Skip to content
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

piolib: support transfer lengths that don't fit in 16 bits #107

Open
jepler opened this issue Dec 16, 2024 · 6 comments
Open

piolib: support transfer lengths that don't fit in 16 bits #107

jepler opened this issue Dec 16, 2024 · 6 comments

Comments

@jepler
Copy link

jepler commented Dec 16, 2024

struct rp1_pio_sm_xfer_data_args {
    uint16_t sm;
    uint16_t dir;
    uint16_t data_bytes;
    void *data;
};

The structure used for data transfers is limited to a 16-bit count of data_bytes.

Our application would like to do larger transfers. Right now we are working around this limitation by submitting several buffers of data sequentially, but it would be preferable if we could submit all the data in one go:

constexpr size_t MAX_XFER = 32768;

void pio_sm_xfer_data_large(PIO pio, int sm, int direction, size_t size, uint32_t *databuf) {
    while(size) {
        size_t xfersize = std::min(size_t{MAX_XFER}, size);
        int r = pio_sm_xfer_data(pio, sm, direction, xfersize, databuf);
        if (r) {
            perror("pio_sm_xfer_data (reboot may be required)");
            abort();
        }
        size -= xfersize;
        databuf += xfersize / sizeof(*databuf);
    }
}```
@pelwell
Copy link
Collaborator

pelwell commented Dec 16, 2024

That's a lot of data - what's your use case?

I'm not against in principle, it's just a matter of not breaking anything in the process. It might be that, due to the alignment requirements, switching to uint32_t would just work.

@jepler
Copy link
Author

jepler commented Dec 16, 2024

I'm driving HUB75 matrices with PIO. Ideally I'd submit a whole frame with one ioctl, but even a 64x32 matrix with 10 bitplanes exceeds 64kB in the encoding I'm using. And folks like to use several of these matrices in daisy chain mode.

Right now I'm just submitting 32kB at a time and trying to determine whether some artifacts I'm seeing are due to a flaw in my code or are due to a pause in the bitstream between ioctls. (a smaller 32x16 display that just needs one data submission per frame doesn't have these artifacts)

@jepler
Copy link
Author

jepler commented Dec 16, 2024

(@ladyada ping for interest)

pelwell added a commit to pelwell/utils that referenced this issue Dec 16, 2024
In many places, the number of bytes in a data transfer is stored as a
32-bit (or greater) value, but the ioctl API to the kernel stores it in
a 16-bit field, alongside which is a 16-bit gap due to the alignment
requirements of the pointer that follows.

Increase the size of data_bytes to a uint32_t, so that it can handle
larger transfers. Since using different sizes as either end can lead
to unpredictable results, a separate IOCTL is used for larger
transfers, allowing the kernel support to be determined and
backwards compatibility to be maintained.

See: raspberrypi#107

Signed-off-by: Phil Elwell <[email protected]>
pelwell added a commit to pelwell/utils that referenced this issue Dec 16, 2024
In many places, the number of bytes in a data transfer is stored as a
32-bit (or greater) value, but the ioctl API to the kernel stores it in
a 16-bit field, alongside which is a 16-bit gap due to the alignment
requirements of the pointer that follows.

Increase the size of data_bytes to a uint32_t, so that it can handle
larger transfers. Since using different sizes as either end can lead
to unpredictable results, a separate IOCTL is used for larger
transfers, allowing the kernel support to be determined and
backwards compatibility to be maintained.

See: raspberrypi#107

Signed-off-by: Phil Elwell <[email protected]>
pelwell added a commit to pelwell/utils that referenced this issue Dec 16, 2024
In many places, the number of bytes in a data transfer is stored as a
32-bit (or greater) value, but the ioctl API to the kernel stores it in
a 16-bit field, alongside which is a 16-bit gap due to the alignment
requirements of the pointer that follows.

Increase the size of data_bytes to a uint32_t, so that it can handle
larger transfers. Since using different sizes as either end can lead
to unpredictable results, a separate IOCTL is used for larger
transfers, allowing the kernel support to be determined and
backwards compatibility to be maintained.

See: raspberrypi#107

Signed-off-by: Phil Elwell <[email protected]>
pelwell added a commit to pelwell/linux that referenced this issue Dec 16, 2024
Add a separate IOCTL for larger transfer with a 32-bit data_bytes
field.

See: raspberrypi/utils#107

Signed-off-by: Phil Elwell <[email protected]>
@pelwell
Copy link
Collaborator

pelwell commented Dec 16, 2024

There are now two PRs for this: one for piolib (#108), and one for the kernel driver (raspberrypi/linux#6543). Both are needed for larger transfers to work. The library should simply fail to do larger transfers with a non-updated kernel.

pelwell added a commit to pelwell/linux that referenced this issue Dec 16, 2024
Add a separate IOCTL for larger transfer with a 32-bit data_bytes
field.

See: raspberrypi/utils#107

Signed-off-by: Phil Elwell <[email protected]>
@jepler
Copy link
Author

jepler commented Dec 17, 2024

Thanks!

pelwell added a commit to raspberrypi/linux that referenced this issue Dec 17, 2024
Add a separate IOCTL for larger transfer with a 32-bit data_bytes
field.

See: raspberrypi/utils#107

Signed-off-by: Phil Elwell <[email protected]>
pelwell added a commit to pelwell/utils that referenced this issue Dec 17, 2024
In many places, the number of bytes in a data transfer is stored as a
32-bit (or greater) value, but the ioctl API to the kernel stores it in
a 16-bit field, alongside which is a 16-bit gap due to the alignment
requirements of the pointer that follows.

Increase the size of data_bytes to a uint32_t, so that it can handle
larger transfers. Since using different sizes as either end can lead
to unpredictable results, a separate IOCTL is used for larger
transfers, allowing the kernel support to be determined and
backwards compatibility to be maintained.

See: raspberrypi#107

Signed-off-by: Phil Elwell <[email protected]>
pelwell added a commit that referenced this issue Dec 17, 2024
In many places, the number of bytes in a data transfer is stored as a
32-bit (or greater) value, but the ioctl API to the kernel stores it in
a 16-bit field, alongside which is a 16-bit gap due to the alignment
requirements of the pointer that follows.

Increase the size of data_bytes to a uint32_t, so that it can handle
larger transfers. Since using different sizes as either end can lead
to unpredictable results, a separate IOCTL is used for larger
transfers, allowing the kernel support to be determined and
backwards compatibility to be maintained.

See: #107

Signed-off-by: Phil Elwell <[email protected]>
@pelwell
Copy link
Collaborator

pelwell commented Dec 17, 2024

That's all been merged now. The driver change missed yesterdays rpi-update release, but you can install a kernel containing the updated driver using either sudo rpi-update pulls/6543 or sudo rpi-updates rpi-6.6.y.

pelwell added a commit to raspberrypi/linux that referenced this issue Dec 17, 2024
Add a separate IOCTL for larger transfer with a 32-bit data_bytes
field.

See: raspberrypi/utils#107

Signed-off-by: Phil Elwell <[email protected]>
pelwell added a commit to raspberrypi/linux that referenced this issue Dec 17, 2024
Add a separate IOCTL for larger transfer with a 32-bit data_bytes
field.

See: raspberrypi/utils#107

Signed-off-by: Phil Elwell <[email protected]>
popcornmix pushed a commit to raspberrypi/linux that referenced this issue Dec 20, 2024
Add a separate IOCTL for larger transfer with a 32-bit data_bytes
field.

See: raspberrypi/utils#107

Signed-off-by: Phil Elwell <[email protected]>
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

No branches or pull requests

2 participants