On 02/04/2019 09:35, Petri Latvala wrote:
> On Mon, Apr 01, 2019 at 04:39:24PM +0200, Guillaume Tucker wrote:
>> The MIPS architecture doesn't provide the hardware atomics that are
>> required for the "create-clear" sub-test such as
>> __sync_add_and_fetch(). As a simple and pragmatic solution, disable
>> this sub-test when building for MIPS. A better approach would be to
>> add a fallback implementation for these operations.
>>
>> Fixes: 6727e17c00b2 ("i915/gem_create: Verify that all new objects are clear")
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@xxxxxxxxxxxxx>
>> ---
>> tests/i915/gem_create.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/i915/gem_create.c b/tests/i915/gem_create.c
>> index 2a861ca8a7ec..8a48496e6c19 100644
>> --- a/tests/i915/gem_create.c
>> +++ b/tests/i915/gem_create.c
>> @@ -142,6 +142,7 @@ static void invalid_nonaligned_size(int fd)
>> gem_close(fd, handle);
>> }
>>
>> +#if !defined(__mips__) /* MIPS doesn't provide the required hardware atomics */
>> static uint64_t get_npages(uint64_t *global, uint64_t npages)
>> {
>> uint64_t try, old, max;
>> @@ -208,6 +209,7 @@ static void always_clear(int i915, int timeout)
>> for (int i = 0; i < ncpus; i++)
>> pthread_join(thread[i], NULL);
>> }
>> +#endif /* !defined(__mips__) */
>>
>> igt_main
>> {
>> @@ -231,6 +233,8 @@ igt_main
>> igt_subtest("create-invalid-nonaligned")
>> invalid_nonaligned_size(fd);
>>
>> +#if !defined(__mips__)
>> igt_subtest("create-clear")
>> always_clear(fd, 30);
>> +#endif
>> }
>
>
> It's a bit ugly. I wonder how much work a fallback mechanism would be?
Sorry I should have sent this as RFC.
> The test is i915 specific and using those on non-x86 architectures
> sounds silly. We could limit building tests/i915/* only if
> host_machine.cpu_family() is x86 or x86_64. But that requires
> revisiting this issue if ever the day comes when i915 can be used on
> other architectures *cough*.
Right, I thought it might not be worth implementing fallback
functions if there is no MIPS hardware on which this test can be
run. Still it would be a shame to leave i-g-t failing to build
on MIPS.
> Apropos, compile-testing on MIPS in gitlab-CI?
This issue was actually hit while building the KernelCI root file
system with i-g-t tests. We're starting to add some MIPS
platforms, so running the generic DRM/KMS tests on them might
start to make sense at some point (cc khilman).
And yes I guess it seems worth considering adding MIPS to
Gitlab-CI as it only appears to be covering x86, armhf and
arm64 (although I did fix an armhf build issue a few weeks ago).
> A compile-tested-only fallback mechanism suggestion, and a bad spot
> for placing the fallback functions:
Thanks, I agree that does look like a sustainable way forward.
We don't quite have a MIPS platform to test that yet in KernelCI,
so hopefully QEMU can be used to test a fallback implementation.
I guess adding placeholder functions as in your example with
igt_assert_f() statements would just add some technical debt with
little added benefit, so I'll work on a v2 with something that
works. Meanwhile we'll just skip i-g-t KernelCI builds on MIPS.
> diff --git a/meson.build b/meson.build
> index 557400a5..0552e858 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -246,6 +246,9 @@ endif
> have = cc.has_function('memfd_create', prefix : '''#include <sys/mman.h>''', args : '-D_GNU_SOURCE')
> config.set10('HAVE_MEMFD_CREATE', have)
>
> +have_atomics = cc.compiles('void f() { int x, y; __sync_add_and_fetch(&x, y); }')
> +config.set10('HAVE_BUILTIN_ATOMICS', have_atomics)
> +
> add_project_arguments('-D_GNU_SOURCE', language : 'c')
> add_project_arguments('-include', 'config.h', language : 'c')
>
> diff --git a/tests/i915/gem_create.c b/tests/i915/gem_create.c
> index 2a861ca8..615bb475 100644
> --- a/tests/i915/gem_create.c
> +++ b/tests/i915/gem_create.c
> @@ -62,6 +62,18 @@ IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
> " that includes allocation of object from stolen memory"
> " and shmem.");
>
> +#if !HAVE_BUILTIN_ATOMICS
> +int __sync_add_and_fetch(void *ptr, uint64_t val)
> +{
> + igt_assert_f(false, "Don't have builtin atomics\n");
> +}
> +
> +int __sync_val_compare_and_swap(void *ptr, uint64_t old, uint64_t new)
> +{
> + igt_assert_f(false, "Don't have builtin atomics\n");
> +}
> +#endif
> +
> #define CLEAR(s) memset(&s, 0, sizeof(s))
> #define PAGE_SIZE 4096
Thanks,
Guillaume
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
