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

cmake: set -DWITH_CPU and -DWITH_ARCH for kodi #1128

Closed
wants to merge 1 commit into from
Closed

cmake: set -DWITH_CPU and -DWITH_ARCH for kodi #1128

wants to merge 1 commit into from

Conversation

MilhouseVH
Copy link
Contributor

This change came about after discussion with @wsnipex and xbmc/xbmc#11343

It turns out that with the current LE build system, when building Kodi, we set -DWITH_CPU=$TARGET_ARCH for Generic, and -DWITH_ARCH=$TARGET_ARCH for everything else.

This has the effect that for RPi/RPi2 the CPU is not correctly determined:
RPi:


-- Core system type: rbpi
-- Platform:
-- CPU: arm, ARCH: arm
-- Cross-Compiling: TRUE

RPi2:

-- Core system type: rbpi
-- Platform:
-- CPU: arm, ARCH: arm
-- Cross-Compiling: TRUE

Generic:

-- Core system type: linux
-- Platform:
-- CPU: x86_64, ARCH: x86_64-linux
-- Cross-Compiling: TRUE

If we completely remove the -DWITH_CPU and -DWITH_ARCH from kodi/package.mk then Generic builds normally, but the RPi/RPi2 builds fail as now ARCH is not determined and CPU is instead detected as arm:

-- Core system type: rbpi
-- Platform: 
-- CPU: arm, ARCH: 
-- Cross-Compiling: TRUE

arm is coming from CMAKE_SYSTEM_PROCESSOR in the toolchain file. Despite sounding like it should be set to the processor type, this property is apparently set correctly as the target platform architecture.

If we then always set -DWITH_CPU=$TARGET_CPU then RPi/RPi2 builds will succeed (xbmc/xbmc#11343 will ensure the ARCH is set correctly, but not the CPU), but now Generic fails with invalid CPU - this is because TARGET_CPU on Generic is x86-64 which is bogus. For some reason our TARGET_CPU on Generic has a hyphen, not an underscore.

With the change in this PR cmake will always know both CPU AND ARCH every time we build with cmake, and - unlike when CPU is unknown - all suitable optimisations will be applied by cmake now that it knows both CPU and ARCH.

RPi:


-- Core system type: rbpi
-- Platform:
-- CPU: arm1176jzf-s, ARCH: arm
-- Cross-Compiling: TRUE

RPi2:

-- Core system type: rbpi
-- Platform:
-- CPU: cortex-a7, ARCH: arm
-- Cross-Compiling: TRUE

Generic:

-- Core system type: linux
-- Platform:
-- CPU: x86_64, ARCH: x86_64
-- Cross-Compiling: TRUE

On RPi and RPi2, the files and filenames in before/after images are indentical.

On Generic, there are 4 files that change name - #0103i is the new build that includes this PR:.

MISSING FROM  #0103i: 5,704         ./usr/lib/kodi/system/libsse4-x86_64-linux.so
MISSING FROM  #0103i: 27,136        ./usr/lib/kodi/system/libexif-x86_64-linux.so
MISSING FROM  #0103i: 68,440        ./usr/lib/kodi/system/libcpluff-x86_64-linux.so
MISSING FROM  #0103i: 200,712       ./usr/lib/kodi/system/players/VideoPlayer/libdvdnav-x86_64-linux.so
NEW FILE IN   #0103i: 5,704         ./usr/lib/kodi/system/libsse4-x86_64.so
NEW FILE IN   #0103i: 27,136        ./usr/lib/kodi/system/libexif-x86_64.so
NEW FILE IN   #0103i: 68,440        ./usr/lib/kodi/system/libcpluff-x86_64.so
NEW FILE IN   #0103i: 200,712       ./usr/lib/kodi/system/players/VideoPlayer/libdvdnav-x86_64.so

In the new build, the four files no longer include the -linux platform suffix. So far I have not seen any problem with this change.

Question

Why do we have TARGET_CPU=x86-64 (with hyphen)? Should we change this to underscore instead and avoid this headache in future?

@stefansaraev
Copy link
Contributor

stefansaraev commented Jan 3, 2017

my two cents on this.

  1. you should not have to care about -DWITH_CPU for kodi.
  2. WITH_CPU usage in kodi's cmake does not make too much sense to me tbh..
  3. kodi should stop treating rbpi like it is a complete different platform. it is linux, like everything else.

EDIT: make sure -DWITH_ARCH= is always correctly passed to kodi's cmake. remove DWITH_CPU and be done with it.

@MilhouseVH
Copy link
Contributor Author

Yes, in theory we shouldn't, but in practice we do. If we don't pass WITH_ARCH then RPi fails with a duff CPU. If we pass WITH_CPU (and give Kodi our real TARGET_CPU) then Kodi chokes on that because our TARGET_CPU is bogus.

At a minimum we could give Kodi just CPU (modified to work around Generic), but giving it both CPU and ARCH simply avoids any potential for confusion.

EDIT: make sure -DWITH_ARCH= is always correctly passed to kodi's cmake. remove DWITH_CPU and be done with it.

We did that, giving Kodi only WITH_ARCH, and Kodi then doesn't know the CPU (for RPi/RPi2) so it doesn't apply any optimisations.

I spent a couple of hours debugging this with @wsnipex this morning, testing the various combinations - nothing, WITH_ARCH, WITH_CPU - and giving Kodi both WITH_CPU and WITH_ARCH works in all cases and puts this issue to bed once and for all. Hopefully. Probably. Well, until next time anyway.

@MilhouseVH
Copy link
Contributor Author

Needless to say this needs to be tested for all platforms other than RPi/RPi2/Generic, although I'm not expecting any issues as we were already setting WITH_ARCH for everything but Generic, so the only difference now is that WITH_CPU is also being passed which should prevent misidentification of the CPU when selecting optimisations.

@stefansaraev
Copy link
Contributor

as said, kodi should stop treating rbpi like it is a complete different platform, this is something to be fixed upstream, not by workarounds in distro buildsystems.

@MilhouseVH
Copy link
Contributor Author

@stefansaraev any thoughts on why the Generic TARGET_CPU is seemingly incompatible with accepted usage, ie. we have x86-64 (with a dash/hyphen) when most distributions (and Kodi) seem to prefer x86_64 (with underscore, as it is for TARGET_ARCH). Changing this to be consistent with Kodi and other distributions would be one less pain point - maybe something for LE9...

@stefansaraev
Copy link
Contributor

stefansaraev commented Jan 3, 2017

it is used with gcc's -march (config/arch.x86_64). I am pretty sure x86_64 is not accepted as a cpu type for -march so you better dont touch that. The generic x86-64 -march is -march=x86-64.

do echo "" | gcc -v -E - 2>&1 | grep -- -march on x86_64 (EDIT: not gentoo of course!) host and see.

what "other distributions" ?

@stefansaraev
Copy link
Contributor

stefansaraev commented Jan 3, 2017

I think I was the one who added x86_64 to openelec few years back. at that time TARGET_CPU=x86-64 was only supposed to be used as a generic -march= optimization flag. no idea if now any other packages in LE (mis)-use it (the one I know is ffmpeg, and last time I checked it was doing it right)

however, as said, after quick look at kodi cmake files, WITH_CPU usage seems inconsistent to me (what does arm1176jzf-s vs arm64-v8a vs arm and so on means?). on rpi the only thing it is doing is disabling neon, but you already have -mfpu=vfp in your CFLAGS. I know, rpi is so special.

@MilhouseVH
Copy link
Contributor Author

do echo "" | gcc -v -E - 2>&1 | grep -- -march on x86_64 (EDIT: not gentoo of course!) host and see.

Thanks for that - interesting, and yet confusing. :)

neil@nm-linux:~/projects$ uname -a
Linux nm-linux 4.4.0-57-generic #78-Ubuntu SMP Fri Dec 9 23:50:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
neil@nm-linux:~/projects$ echo "" | gcc -v -E - 2>&1 | grep -- -march
COLLECT_GCC_OPTIONS='-v' '-E' '-mtune=generic' '-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/5/cc1 -E -quiet -v -imultiarch x86_64-linux-gnu - -mtune=generic -march=x86-64 -fstack-protector-strong -Wformat -Wformat-security
COLLECT_GCC_OPTIONS='-v' '-E' '-mtune=generic' '-march=x86-64'

So -march=x86-64 (with hyphen) is what we use for TARGET_CPU... yet our TARGET_ARCH=x86_64 (with underscore)... argh! :)

what "other distributions" ?

Well, so I've been told: "it's x86_64 _everywhere_" (where "it" refers to TARGET_CPU).

I think I was the one who added x86_64 to openelec few years back.

To be honest it's not often (if ever?) been an issue to my knowledge, and is only now an issue because of trying to work around the Kodi CPU/ARCH detection which kinda sorta works. Except when it doesn't. If the hack in this PR works I'd be more than happy to find more interesting things to worry about, as I don't think there's much chance of a fix upstream (assuming one is even necessary, which is maybe 50/50).

@lrusak
Copy link
Member

lrusak commented Jan 4, 2017

I don't really agree with this. I don't think we should change our build system to work around a kodi quirk.

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Jan 4, 2017

Then we continue to tell Kodi (and maybe other packages) that our CPU is arm and not arm1176jzf-s or cortex-a7 etc.

All this change really does is to pass to cmake the correct CPU (for non-Generic) and correct ARCH (for Generic). If you'd rather not pass the CPU for Generic (as there's obviously disagreement over what it should be, hyphen or underscore), then I can remove that. But passing the correct CPU and ARCH for ARM builds seems better than compiling for an arm CPU, surely?

Upstream (Kodi) won't change anything, so I'm told. So either we do, by patching Kodi, or modifying our build system to pass all the right details to cmake (ie. this PR), or we continue building for random and non-existent CPUs.

@stefansaraev
Copy link
Contributor

stefansaraev commented Jan 4, 2017

-DWITH_CPU and -DWITH_ARCH are not cmake global options. they are package specific, so they dont belong in global *_CMAKE_OPTS. you can do the change in kodi/package.mk. thats my last 2 cents :)

@MilhouseVH
Copy link
Contributor Author

OK if that's the case... I'll close this. Clearly I've been given bum advice.

@MilhouseVH MilhouseVH closed this Jan 4, 2017
@MilhouseVH
Copy link
Contributor Author

By the way thanks all that contributed to this conversation.

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Jan 4, 2017

Reopening with one last spin of the dice. If this isn't acceptable, then close.

Edit: Title updated.

@MilhouseVH MilhouseVH reopened this Jan 4, 2017
@MilhouseVH MilhouseVH changed the title cmake: always set -DWITH_CPU and -DWITH_ARCH for target cmake: set -DWITH_CPU and -DWITH_ARCH for kodi Jan 4, 2017
@stefansaraev
Copy link
Contributor

I am either blind or stupid. I cant find a cmake file in kodi git where specific optimizations are done based on -DWITH_CPU. can't find rpi specifics either (besides disabling neon, and you already have an option to pass -DENABLE_NEON=OFF/ON). can you point me to a cmake file where such optimizations (C*FLAGS and so on) are done?

@MilhouseVH
Copy link
Contributor Author

Not yet merged: xbmc/xbmc#11352

@stefansaraev
Copy link
Contributor

stefansaraev commented Jan 4, 2017

a proposed PR does not mean it is done right (EDIT: it adds more to the mess you were told that wont be changed). now it makes -DWITH_CPU non optional?

  • rbpi is still treated like a complete different platform.
  • what does CPU=armeabi-v7a means. and what CPU=arm ??
  • you do already have -mtune=arm1176jzf-s for rpi0/1 project.
  • you do already have -mfpu=neon* in C/CXXFLAGS in LE. I can't comment on -mvectorize-with-neon-quad. before this PR: if you passed -DENABLE_NEON - -mvectorize-with-neon-quad is used. after: well. CPU has to be same as ARCH (arm)

EDIT: however, this PR now looks fine to me, if you want it that much for little to no gain :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants