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

sse4: build in support instead of making a shared library #16168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lrusak
Copy link
Contributor

@lrusak lrusak commented May 18, 2019

This removes creating the sse4 shared library and dlopening it when using it. This just builds the method into core.

@lrusak lrusak added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Platform: Linux v19 Matrix labels May 18, 2019
@lrusak lrusak added this to the Matrix 19.0-alpha 1 milestone May 18, 2019
@lrusak lrusak requested a review from Rechi May 18, 2019 18:23
@wsnipex
Copy link
Member

wsnipex commented May 19, 2019

so SSE4 is now mandatory for vaapi?

@lrusak
Copy link
Contributor Author

lrusak commented May 19, 2019

so SSE4 is now mandatory for vaapi?

This is a really good point that I totally overlooked. I can change this to be optional for VAAPI however according to Wikipedia SSE4.1 was first used in Intel Penryn processors which came out in 2008. Are we limiting ourselves by requiring SSE4.1 for VAAPI?

@MilhouseVH
Copy link
Contributor

Are we limiting ourselves by requiring SSE4.1 for VAAPI?

I'd say so. If Nvidia users are being switched to Nouveau and VAAPI then I think you'll find a number of users with older pre-SSE4.1 CPUs (think: Atoms) but still potentially capable GPUs that might now be excluded by making SSE4.1 mandatory for VAAPI. AMD also only added SSE4.1 in 2011 (Bulldozer).

@lrusak
Copy link
Contributor Author

lrusak commented May 20, 2019

Are we limiting ourselves by requiring SSE4.1 for VAAPI?

I'd say so. If Nvidia users are being switched to Nouveau and VAAPI then I think you'll find a number of users with older pre-SSE4.1 CPUs (think: Atoms) but still potentially capable GPUs that might now be excluded by making SSE4.1 mandatory for VAAPI. AMD also only added SSE4.1 in 2011 (Bulldozer).

Nvidia won't work on VAAPI without reclocking support in nouveau, so I consider that a non issue

@wsnipex
Copy link
Member

wsnipex commented May 20, 2019

it's unlikely that someone uses vaapi without a sse4.1 capable CPU as long as nouveau doesn't get reclocking.
But if we decide to require sse4.1 than the build system has to reflect this dependency

@MilhouseVH
Copy link
Contributor

Nvidia won't work on VAAPI without reclocking support in nouveau, so I consider that a non issue

I thought reclocking wasn't an issue for all Nvidia cards - mostly a problem for "modern" GPUs which may be less likely to be used with older CPUs. The thing is there may still be a subset of users that could use Nouveau/VAAPI with an SSE4.1-less CPUs (yes, I'm one of them - Nouveau/VAAPi with ION2 and Atom D525 worked OK).

LibreELEC:~ # cat /proc/cpuinfo
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 28
model name      : Intel(R) Atom(TM) CPU D525   @ 1.80GHz
stepping        : 10
microcode       : 0x107
cpu MHz         : 119.438
cache size      : 512 KB
physical id     : 0
siblings        : 4
core id         : 0
cpu cores       : 2
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 10
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts rep_good nopl cpuid aperfmperf pni dtes64 monitor ds_cpl tm2 ssse3 cx16 xtpr pdcm movbe lahf_lm dtherm
bugs            :
bogomips        : 3592.53
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

Making SSE4.1 mandatory to simplify the build at the risk of excluding some users seems a little premature IMHO, particularly if it adds another potential hurdle for users migrating to Nouveau/VAAPI.

@fritsch
Copy link
Member

fritsch commented May 20, 2019 via email

@lrusak
Copy link
Contributor Author

lrusak commented May 21, 2019

it's unlikely that someone uses vaapi without a sse4.1 capable CPU as long as nouveau doesn't get reclocking.
But if we decide to require sse4.1 than the build system has to reflect this dependency

Yep I get your point. First we should decide if it should be optional or not.

@lrusak
Copy link
Contributor Author

lrusak commented May 21, 2019

Nvidia won't work on VAAPI without reclocking support in nouveau, so I consider that a non issue

I thought reclocking wasn't an issue for all Nvidia cards - mostly a problem for "modern" GPUs which may be less likely to be used with older CPUs. The thing is there may still be a subset of users that could use Nouveau/VAAPI with an SSE4.1-less CPUs (yes, I'm one of them - Nouveau/VAAPi with ION2 and Atom D525 worked OK).

Making SSE4.1 mandatory to simplify the build at the risk of excluding some users seems a little premature IMHO, particularly if it adds another potential hurdle for users migrating to Nouveau/VAAPI.

So now you are trying to keep support for something that is secondary to something that hasn't even been removed yet? Where is the line?

I don't think we should specifically cater to hardware that is 10+ years old

@MilhouseVH
Copy link
Contributor

All I will say is that removing support for existing, and working, CPU hardware by enforcing SSE4 under the guise of a "code cleanup" is (IMHO) a very poor trade at this time.

Once you are certain that nobody is using non-SSE4 capable hardware then sure, resurrect this PR, but until then I would suggest leaving the SSE4 support as it is (ie. dynamic, which is working fine) or remove the SSE4 requirement entirely if it's no longer required.

The fact is that x86_64 CPUs from the end of the last decade are still "good enough" for many users so there may need be a long tail where x86_64 CPU feature support is concerned. This is unlike GPU support which has tended to date and become obsolete much more rapidly.

Geez, I'm typing this post on a Windows 7 PC with a quad-core (8 cores with SMT) i7-870 CPU launched Q3 2009 and I have absolutely no intention of updating the hardware until the CPU physically dies - not because I'm a cheap-skate, but because it works just fine for what it needs to do! 😄

I should note the i7-870 does support SSE 4.2, but the Atom D525 which launched a year later in Q2 2010 does not (it does at least support SSE3). Thanks to Intel market segmentation, for all I know there may be other - more recent - Atom-based CPUs currently running Kodi with a VAAPI-based GPU that also do not support SSE4.1,

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label May 25, 2019
@lrusak lrusak closed this May 27, 2019
@Rechi Rechi removed the v19 Matrix label May 27, 2019
@Rechi Rechi removed this from the Matrix 19.0-alpha 1 milestone May 27, 2019
@lrusak lrusak reopened this Feb 6, 2020
@lrusak lrusak force-pushed the sse4-no-dll branch 2 times, most recently from 6e24bef to 0e8dfc4 Compare February 6, 2020 16:11
@lrusak lrusak added this to the Matrix 19.0-alpha 1 milestone Feb 6, 2020
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the current code guidelines.

xbmc/platform/linux/sse4/SSE4.cpp Outdated Show resolved Hide resolved
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 6, 2020
@lrusak
Copy link
Contributor Author

lrusak commented Feb 6, 2020

This now requires the build machine to have SSE4 support when building VAAPI, however, it doesn't require the machine the code is running on to have SSE4 when using VAAPI. I'm not sure if I should make that explicit or if it's fine how it is.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 7, 2020
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 8, 2020
@lrusak
Copy link
Contributor Author

lrusak commented Feb 8, 2020

@MilhouseVH is the new behaviour acceptable to you?

@MilhouseVH
Copy link
Contributor

To be honest it seems a bit weird to put a SSE4 dependency on the build host when the generated code doesn't have a SSE4 dependency, but at least build hosts are more likely to support SSE4 than clients so this alternative should be less of a problem (my FX-8350 build host supports SSE4.2, my Atom D525 Kodi client does not but then I'd never consider using the latter to build Kodi!)

@lrusak
Copy link
Contributor Author

lrusak commented Feb 14, 2020

@MilhouseVH updated to allow building and using vaapi on machines that don't support sse4.1

@phunkyfish phunkyfish added PR Cleanup: Needs Review This PR is good and ready for a review. Someone please review. PR Cleanup: Merge Candidate Candidate for merging after PR cleanup and removed PR Cleanup: Needs Review This PR is good and ready for a review. Someone please review. labels Mar 9, 2020
@phunkyfish
Copy link
Contributor

@lrusak this appears ready to go in, can it?

@lrusak
Copy link
Contributor Author

lrusak commented Mar 21, 2020

should be good to go if someone approves

@phunkyfish
Copy link
Contributor

@wsnipex @MilhouseVH @Rechi are you good with this now?

I'd approve but I have zero domain knowledge here!

@MilhouseVH
Copy link
Contributor

It's been in my LE builds for a while. There was a mention of a possible SSE4.1 issue in #17531 which may be worth investigating?

@DaveTBlake DaveTBlake removed this from the Matrix 19.0-alpha 3 milestone Oct 30, 2020
@DaveTBlake DaveTBlake added v20 Nexus and removed PR Cleanup: Merge Candidate Candidate for merging after PR cleanup v19 Matrix labels Oct 30, 2020
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 24, 2022
@jenkins4kodi
Copy link
Contributor

@lrusak this needs a rebase

@fuzzard fuzzard removed the v20 Nexus label Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: Linux Rebase needed PR that does not apply/merge cleanly to current base branch Type: Cleanup non-breaking change which removes non-working or unmaintained functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants