Comment 6 for bug 388388

Dave Martin (dave-martin-arm) wrote :

Hi there, I've just been taking a quick look at this myself... looks like you saw and fixed the same ftbfs I got.

For the NEON autodetection code, I have some concerns:

  * SIGILL may be bad for libx264, and may cause unpredictable results for ths usual reasons, especially if the library may be used in a multithreaded process where the signal handler disposition for SIGILL could be left in an execpected state.

  * Using SIGILL to detect NEON will misdetect in some cases, particularly on the older Babbage boards where we explicitly want to lie about the hardware features. Of less concern to Ubuntu, it will also unpredictably misdetect if the kernel is built with CONFIG_NEON=n but the hardware has NEON (in this configuration, you can get a true or false test result, depending on what code ran since the last context switch). the test for neon should be ported to hwcaps in the usual way, maybe falling back on SIGILL at build time if the right headers and defines aren't available (<linux/auxvec.h> and <asm/hwcap.h> present and HWCAP_NEON defined).

  * The x264_cpu_fast_neon_mrc_test() code is likely to be unsafe -- the read of the performance monitor User Enable Register (PMUSERENR) may cause a SIGILL on Dove and other architecture lisensee platforms (since the performance monitor counters are an optional part of the architecture and may not be present at all). The test will also do nothing useful on most kernels (including lucid imx51), since non-standard patches are required in order to enable userspace access to the performance monitor counters anyway (in any case, there is no protection to stop multiple processes/threads accessing the counters at the same time and causing incorrect results). Note also that although we don't do it for Ubuntu now, the code is supposed to support buliding for pre-v7 architectures, where the PMUSERENR read will likely SIGILL.

This test should be stubbed out and assumed to fail in the Ubuntu build, since currently it always fails on the current imx51 kernel and may SIGILL on Dove (can someone with hardware test it?). Optionally though, we could read /proc/cpuinfo and attempt to make a more intelligent decision based on whether we recognise the CPU implementer/architecture/variant/part/revision fields.