Details
- Reviewers
• hselasky imp manu
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Would you kindly regenerate the patch with -U99999 (see "Create a Revision via Web Interface" on https://wiki.freebsd.org/action/show/Phabricator)
Thank you for taking this on, I'm quite happy to see work to support audio on RPi. There's a lot to get through here so it will take some time to review the full change.
Do you have this available (or could you make this available) as a git repo, ideally with a series of individual commits? I think some portions of the change (like the format string changes) will be easier and quicker to review, and they can be pushed to main as independent changes as review is completed.
| sys/arm/broadcom/bcm2835/bcm2835_audio.c | ||
|---|---|---|
| 30–32 ↗ | (On Diff #112180) | It's not typical to include a comment like this for a header file, unless there's something surprising/unexpected/unusual about the include. Also this comment style doesn't match style(9); I would just remove it. |
| sys/arm/broadcom/bcm2835/bcm2835_audio.c | ||
|---|---|---|
| 262 ↗ | (On Diff #112180) | It looks like we do not need to store ch in here at all, in fact -- ch is only ever set to &sc->pch. The Linux driver just puts a constant in each of the fields. |
| sys/arm/broadcom/bcm2835/vc_vchi_audioserv_defs.h | ||
| 117–118 ↗ | (On Diff #112180) | The Linux driver calls these two "cookie1" and "cookie2"; we may want to follow suit, as (it seems) they are just opaque values to the hardware. |
No problem — just happy someone's taking the time to review
Do you have this available (or could you make this available) as a git repo, ideally with a series of individual commits? I think some portions of the change (like the format string changes) will be easier and quicker to review, and they can be pushed to main as independent changes as review is completed.
I've put the patch in the branch rpi64b_audio of https://gitlab.com/devesascampos/freebsd-src.git.
Alas, I wasn't very systematic during development and only have changes aggregated by milestone (audio; bulk transfers; 32bit compat; stability) and not by type/place of change, and with bug fixes mixed with implementation of new things. In particular the first commit aggregated both changes to the audio driver and the vchiq subsystem — which are the brunt of the patch.
Can put those up but doubt they'll be very enlightening.
| sys/arm/broadcom/bcm2835/bcm2835_audio.c | ||
|---|---|---|
| 30–32 ↗ | (On Diff #112180) | Sure. Just felt a bit "dirty" adding an underscored include for something as banal as a format string. In general, being new to kernel development, I decided to be liberal with comments when something felt off—comments are easy to take out. For those a quick "not needed" is alright, and I'll remove them. |
[ref:dsb]: https://github.com/raspberrypi/linux/commit/35b7ebda57affcfd3616d39d5a727a4495b31123 [ref:sems]: https://github.com/raspberrypi/linux/commit/24a4262afb10907fce3cdbc3ae336fcf4cdaece5 [ref:sizes]: https://github.com/raspberrypi/linux/commit/e64568b8ea6c04e747e432c17ce2452652075216 [ref:sizes2]: https://github.com/raspberrypi/linux/commit/f9bee6dd24addfa00c2c8d50c25b73efbfbb28ba [ref:deadcode]: https://github.com/raspberrypi/linux/commit/14f4d72fb799a9b3170a45ab80d4a3ddad541960 [ref:sync]: https://github.com/raspberrypi/linux/commit/51c071265079319583e4c6e8c61e09660300d0bf [ref:longbulk]: https://github.com/raspberrypi/linux/commit/37f6f19a83722c9b866cecb5e455b2e16e5bbc6b
Did you just apply these changes, or reimplement them? The license on at least some of the files has changed there so we can't just use the changes directly.
| sys/arm/broadcom/bcm2835/bcm2835_audio.c | ||
|---|---|---|
| 293 ↗ | (On Diff #112180) | The kernel style is to cast the value to a uintmax_t and use %ju rather than the PRI* macros. |
| sys/contrib/vchiq/interface/vchiq_arm/vchiq_kern_lib.c | ||
| 154 | Why not %p given it's a pointer? | |
Hmm… I checked the copyrights hadn't changed in the files but didn't consider they might have changed the terms since the code was originally imported.
I didn't just take the patches and apply them directly of course since the code bases have diverted significantly; however a few of them are just mechanical stuff that was copied verbatim (maybe modulo linux-fbsd naming differences):
- "dsb" maps to "dsb(sy)" in arm64 [ref:dsb]
- some fields in structs updated to their fixed size equivalents [ref:sizes]
- some (obvious) casts [ref:sizes2]
- [ref:sync] is just the removal of two barriers
on top of that there are the numerous casts in printf-s throughout the code which I didn't copy individually but assume look exactly the same as their linux counterparts.
I got the correct cache-line size for 64bit systems from https://github.com/raspberrypi/linux/commit/c683db8860a80562a2bb5b451d77b3e471d24f36 but then just wrapped that in a #if/#else as opposed to the "capability system" they implement in the patch.
In [ref:deadcode] they remove a bunch of code; I just used that as a justification to #if 0-out the VCHIQ_MSB_BULK_TX/RX case in parse_rx_slots (which looks difficult to adapt to 64 bits).
Where I significantly and meaningfully changed code just like linux was in [ref:sems]. This entailed changing the parameter of the remote_event_* functions which were called-back with a pointer to instead receive a (fixed) pointer that serves as reference point to the offset that is communicated to/fro videocore and which represents the pointer that used to be sent instead.
(I just checked and netbsd has these changes included in the same way: cf. the remote_event_* functions in vchiq_core_.c in
https://github.com/NetBSD/src/commit/dfebdeb7ffe2eebeeda33c4d3373bf076882eca7#diff-428ed88a54c6f9f07513831e23477e4790e76b16caaaab85386ad485b85a32f4
(they even added a macro to do some address calculations like i did—i had not seen it before—but then didn't use it… but i digress))
On top of that, I saw where they had barriers in the code and tried those locations until I stopped having synchronisation issues (cf. the "not on arm64, it would seem..." comment in vchiq_kmod.c)
As for [ref:longbulk] the calculations are the same but the code is not: e.g. in the create_pagelist they have different loops for "long bulks" vs. "normal bulks" but the code here has factored things into a single loop.
I also consulted the netbsd code but don't remember copying anything directly from there.
Let me know asap if you think there might be problems here and I'd be grateful for any pointers you might have on how to navigate these types of issues.
| sys/arm/broadcom/bcm2835/bcm2835_audio.c | ||
|---|---|---|
| 293 ↗ | (On Diff #112180) | If you can put these sorts of changes (%ju, %p etc.) into a single commit we can bring that one into main and get it out of the way. |