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

Unnecessary enforcement of 32-boundaries in oni_read_frame() #16

Open
jonnew opened this issue Dec 31, 2023 · 1 comment
Open

Unnecessary enforcement of 32-boundaries in oni_read_frame() #16

jonnew opened this issue Dec 31, 2023 · 1 comment

Comments

@jonnew
Copy link
Collaborator

jonnew commented Dec 31, 2023

In this line we round our frame data reads up to the next 32-bit boundary.

rsize += rsize % sizeof(oni_fifo_dat_t);

AFAIK, this was implemented way back in the day when Xillybus was being called directly in the library, with a 32-bit wide read FIFO used to stream data, and therefore anything other than 32-bit reads could corrupt the stream. However, now we are doing block reads which can be set to any byte size so long as they are big enough to store the largest frame produced in the device table. Is there still a reason we are enforcing this? If the hardware does not produce 32-bit aligned data, this rounding operation will result in a hard crash, but this seems totally unnecessary. In fact, I discovered this limitation because I'm modifying the onidriver_test.c to remove some of its limitations. In doing so I forgot to make sure it produced 32-bit aligned data. Rather than forcing this, I removed this step and now return rsize and everything worked fine.

@aacuevas
Copy link
Collaborator

aacuevas commented Jan 2, 2024

To be fair, while it might have originated from xillybus requirements, it now is more related to the general ONIX pipeline than any specific driver. This is because, for performance, we just pack the pipeline endpoint into 32-bit words. (Part of it is still 16-bit words, but we are actively working towards making the whole pipeline work natively at 32bit).

However, while this is true of all our current hardware, it is not necessarily true for any possible ONI implementation. That said, it is still good to cater for the possibility of some hardware having specific word sizes, as the opposite will entail unnecessary buffering (e.g.: asking for 5 bytes from a device only capable of streaming 16-bit words).

I think the proper solution would be to let the onidrivers to specify their read word size and use it instead of a fixed size. Of course, if this results in padding being required, it will be the hardware responsibility (as it is now, since it is the ONIX hardware which adds padding zeroes)

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