surge-synthesizer / surge

Synthesizer plug-in (previously released as Vember Audio Surge)
https://surge-synthesizer.github.io/
GNU General Public License v3.0
3.13k stars 400 forks source link

Surge Linux ARM consolidating issue #2345

Closed baconpaul closed 4 years ago

baconpaul commented 4 years ago

Using this issue so we can consolidate conversations between @falkTX @hexdump0815 @baggypants and others

We had #1481 #578 #2342 and #1606 with some deep content. Those are all (or very soon will be all) closed.

Here's the state of play as we head into 1.7.0 (which we intend to ship Aug 1)

  1. I've refactored the cmake file so we can set up a toolchain and set ARM-specific options easily enough. More to do of course. But @falkTX and @hexdump0815 if you see the change in 41f6d9447581 you'll see I've made it easy to specify toolchains and flags for multiple toolchain files

  2. I've done an Ubuntu ARM cross compile of headless using mostly @falkTX merge (which uses sse2neon) in our pipeline so PRs will try and require an ARM compile going forwards

  3. I have not tested the resulting code since I don't have an ARM

  4. I have ordered a PI4 from Amazon :)

Ideally my goal is with 170 folks building for ARM/Linux would not require extensive patches to do so

So some thoughts

  1. sse2neon vs simde. Seems @baggypants and @hexdump0815 chose simde with string replacement on the functions. Is there some reason to believe one is better or worse than the other? I"m happy to make a simde_compat.h which just does define m128 simdem128 and so on if that's a better path

  2. Only @hexdump0815 has been able to make ARM make sound, and then only in rack. @Baggypants got a crash in headless. When my PI arrives perhaps I can debug some

  3. I'm all ears to how we merge and make this work. I've isolated it so I'm happy to do ARM conditional merges while we are in the 170 intel freeze period

  4. I am not an ARM expert at all. But am motivated to make this work long term since I am a mac user.

So hope you all don't mind me consolidating the conversation into one place and closing the old issues.

falkTX commented 4 years ago

(Commenting here to keep conversation all in one place, sorry for last message)

Could we try to align the allocations? For linux we always set _aligned_malloc(x,a)=malloc(x) or has this changed in recent versions?

We could try a little wrapper around posix_memalign See https://linux.die.net/man/3/posix_memalign

baconpaul commented 4 years ago

Aligned malloc is only used in one spot and yeah it is just malloc - the rest of the code use c++ alignas keyword.

That one spot was where @baggypants was crashing in January though so I bet your suggestion is good.

Oh and got an update - My pi arrives tomorrow!

baconpaul commented 4 years ago

And I think the way to do this is to move the define for aligned malloc into arch specific flags on Linux - that’s an easy refactor with the cmake changes I just pushed and then define _aligned_malloc a,b as aligned_alloc b,a

Baggypants commented 4 years ago

We've got it working on the pi by the way.

https://discourse.zynthian.org/t/vember-surge-synth-opensourced-builds-and-runs-on-linux/2656/61?u=baggypant

baconpaul commented 4 years ago

the branch in the 'baconpaulfork calledarm-convo-2345` contains this change and I got a compile. I'll wait to merge until I try it on my pi (or someone else does) in case more changes are needed.

baconpaul commented 4 years ago

@Baggypants oh cool. Could you please share the code changes you needed to make?

Baggypants commented 4 years ago

We're just using the https://github.com/falkTX/surge.git fork.

Here is the deploy script https://github.com/zynthian/zynthian-sys/blob/master/scripts/recipes/install_surge.sh And the premake5 script https://github.com/zynthian/zynthian-sys/blob/master/scripts/recipes/install_premake5.sh

baconpaul commented 4 years ago

Ahh gotcha. And yeah This branch is 4 commits ahead, 293 commits behind surge-synthesizer:master. - and will be even further behind surge-synthesizer:main. So it is worth doing something here as we approach 1.7.0. Those 293 commits contain some great new features and some really important bug fixes.

I would really love for @falkTX to not have a fork. And for projects like zynthian to not have to run sed across the code base to build it. lets try and get this stuff merged up into main!

baconpaul commented 4 years ago

(And I know the reason this hadn't happened until now was because @falkTX had had a PR hanging out forever with these changes which I hadn't merged - just it came in after our cmake shift; not being negative about the fork and so on! Just aspiring to keep it all in one codebase :) )

falkTX commented 4 years ago

The "fork" was only there for the PR, I will delete that now that you merged similar changes

baconpaul commented 4 years ago

great yes we can clean it up when we tag 1.7.0!

(If you delete it now it will break folks who use premake directly because premake won't work with the head of code anymore)

And just to reiterate - the reason for the fork was just because I hadn't gotten around to figuring out how to merge your PR and test it with cmake. Not criticizing etc.... (tone is hard on the internet as we've discussed so figure clarity is best).

falkTX commented 4 years ago

(If you delete it now it will break folks who use premake directly because premake won't work with the head of code anymore)

haha, well, too late now..

all is fine for me. hope me deleting does not cause any issues. better to focus on v1.7 stuff now, then LV2 when there is time (at least on my side)

baconpaul commented 4 years ago

ha right. Well yeah I don't know who pulls on your fork either but I bet we find out soon :)

1.7.0 is inches away from final. People moving to main today is absolutely fine. We just have some graphics and documentation work to finish the release really.

Baggypants commented 4 years ago

I made a mistake, This is the working build script https://discourse.zynthian.org/t/vember-surge-synth-opensourced-builds-and-runs-on-linux/2656/72?u=baggypants we're pulling from https://github.com/hexdump0815/surge-arm-build.git

baconpaul commented 4 years ago

Right. And that's the version with simde not the version with sse2neon

OK well my PI should arrive before end of day tomorrow so I will take either current or that version and make it something mergeable into mainline.

Thank you!

hexdump0815 commented 4 years ago

just to notes regarding your thoughts from the first post:

hexdump0815 commented 4 years ago

one more bonus for simde: it is very portable - in case you want to get surge working on open power or mips systems too one day :) ... and they do ci tests for each commit on all those systems

baconpaul commented 4 years ago

OK On my branch I've moved from sse2neon to simde but done it without a code converter. Instead I wrote https://github.com/baconpaul/surge/blob/arm-convo-2345/src/common/simde_arm_compat.h

in theory you should be able to check out surge on that branch, do a submodule update, then build (EDIT but the correct flags still won't come across on your pi - that branch is still just at I was able to do similar with my cross compiler).

My Pi will arrive soon and I'll try it before I merge.

In your patches, @hexdump0815, then it seems only other things are

  1. /usr/share to /usr/local/share
  2. the proper sub-dir name for VST3
  3. architecture flags for arm

if I get stuff running I can deal with 2 and 3 easily. Was 1 just a preference?

And yeah simde is way more complete the sse2neon. Good stuff.

Baggypants commented 4 years ago

/usr/ should only really be used by packages for the distribution you use, /usr/local/ is where you're supposed to put stuff you compile yourself from source. It's a convention.

baconpaul commented 4 years ago

Ahh right; and we are configured for a global install which is distributed by dpkg. (All the commercial daws look in /usr/lib/vst3 and stuff)

But we followed the freedesktop conventions. Right how we look in (in order)

${XDG_DATA_HOME}/surge if XDG_DATA_HOME is set ~/.local/share/surge if it exists ~/.local/share/Surge if it exists /usr/share/surge if it exists else /usr/share/Surge

I suppose we could add /usr/local/share/Surge into those cascades also but that's sort of what XDG_DATA_HOME is meant for isn't it?

hexdump0815 commented 4 years ago

the dir naming was just my (and the usual old style) dir naming for local compiled stuff ... maybe just use the same paths as for the linux on intel build - intel and arm do not differ in their basic dir structure ... thanks a lot for creating that compat header, which is of course the much cleaner solution ... i always expected it to be much larger - this is why i left it the way i used it :)

baconpaul commented 4 years ago

Ok cool! So let me do a build when my pi is up and running and then I can get it merged with the last step which is the various arch flags set up as cmake variables; and I’ll update the readme. Probably have it together before end of tomorrow for some testing

Exciting!

hexdump0815 commented 4 years ago

when testing it on the raspberry pi, maybe use two sd cards: one with the normal raspbian which should give you armv7l and one with the 64bit build of raspbian from https://downloads.raspberrypi.org/raspios_arm64/images/raspios_arm64-2020-05-28/2020-05-27-raspios-buster-arm64.zip which will give you an aarch64 build ... to start with raspbian might be the easiest as - at least for me - getting ubuntu properly running on the rpi4 was not that easy and if it builds properly in armv7l and aarch64 on raspbian it should also build well on other arm systems in armv7l and aarch64 i guess ...

baconpaul commented 4 years ago

Yeah great I will try that

For 170 my goal is “on a stock pi I can pass headless and I have cmake set up with documentation so we can add other flavors and flags.” Will be plenty to do after that. And of course I need to figure out how all this will work with Xcode 12 also.... but not yet

hexdump0815 commented 4 years ago

btw. it not only runs headless on arm - you can nicely use it on both armv7l and aarch64 with the usual surge plugin ui ... what i can recommend for testing are the beta versions of reaper which are available for both armv7l and aarch64 and work perfectly well ...

baconpaul commented 4 years ago

Oh awesome - yes that will help me parameterize the vst3 also

baconpaul commented 4 years ago

Oh and with a new pi4 just grab the pi is from raspberry pi.org and that’s the 32 bit version - that’s my first plan. I didn’t get a second sd card in my amazon order! But will get that running then perhaps you could offer review and diffs on the build environment?

Cool. I am glad we are merging this all for 170. Thank you for all your contributions here!

nemequ commented 4 years ago

OK On my branch I've moved from sse2neon to simde but done it without a code converter. Instead I wrote https://github.com/baconpaul/surge/blob/arm-convo-2345/src/common/simde_arm_compat.h

SIMDe can do this for you; just define SIMDE_ENABLE_NATIVE_ALIASES prior to including it.

Sorry, I'm a bit behind bug mail right now and I've only given this a quick skim, but if anyone has any questions feel free to @ me, or there is a chat room.

Edit: also, if anyone wants to do a little profiling to see what functions are being used, I'd be happy to take a look and see if we can optimize them a bit. Defining SIMDE_NO_INLINE prior to including SIMDe should make SIMDe show up (though obviously performance will take a hit).

Oh, and if you're not happy about the giant SIMDe repo as a submodule, we also have a version without the tests.

baconpaul commented 4 years ago

Thank you - very useful!

baconpaul commented 4 years ago
./build/surge-headless 

surge-headless is our regtest engine for the synth engine excluding the UI
Run 'surge-headless --help' for options.

===============================================================================
All tests passed (464113 assertions in 30 test cases)

pi@raspberrypi:~/surge $ uname -a
Linux raspberrypi 5.4.42-v8+ #1319 SMP PREEMPT Wed May 20 14:18:56 BST 2020 aarch64 GNU/Linux
pi@raspberrypi:~/surge $ 

that's from a mergeable branch with a clean cmake build!

baconpaul commented 4 years ago
Screen Shot 2020-07-15 at 10 30 41 AM Screen Shot 2020-07-15 at 10 30 51 AM

Here's a screenshot of Surge running in REAPER ARM and the about screen showing the architecture. Again this is from a branch which I will be able to merge today and no changes other than checkout and cmake.

hexdump0815 commented 4 years ago

very cool! - is this on 64bit raspbian? - otherwise the build might have gotten confused by running a 32bit raspbian userland on a 64bit kernel resulting in showing aarch64 although the plugin actually is armv7l (maybe) ...

i'm a bit sick right now, but as soon as i'm more fit again, i'll give the build from your branch a try on my ubuntu armv7l and aarch64 ...

baconpaul commented 4 years ago

Yeah that's on 64bit raspbian on a pi4 I have another sd card so will try 32 bit later feel better! and thanks so much for all your help here

baconpaul commented 4 years ago

OK everything is merged. You should be able to build an LV2 and a VST3 on aarch64 and (theoretically but I haven't test it yet) arm71 with the head of the codebase. This will be included in the Aug 1 1.7.0 release too.

https://github.com/surge-synthesizer/surge#building-for-arm-platforms

those are the directions I wrote

I'm thinking I would close this issue but before I did that wanted

1: Any feedback or changes from you all 2: ~Someone "other than me" to test those instructions on a PI and see if surge works~ folks at the zynthian forums confirm that its working for them!

  1. Based on 1 and 2 to do another ARM change if we need

If any of the 3 of you get a chance to look in the next few days I would appreciate it. I will also drop this on that zynthian thread.

Best

Baggypants commented 4 years ago

Build instructions work fine for me, I used 32bit ZynthianOS which is based on 32bit Raspberry Pi OS

baconpaul commented 4 years ago

Wonderful. I'll close this issue then. We can open new ones for ARM changes going forward!

Thank you all for your help!

seclorum commented 3 years ago

This was a brilliant thread.

baconpaul commented 3 years ago

Thanks! If you look at more recent commits you can see the technique here also let us do our mac universal binary with 1.8.0!

seclorum commented 3 years ago

Yeah, this is a great instance of multi-disciplinary open source folks pushing things forward - didn't want to gravedig the thread, but I just had to say it was fun to read - almost as much fun as it is to discover Surge is already running on my Zynthian now! :)