| Message ID | d8f0f23654e3e6114a1b67db66ac2c599d31b836.1554129404.git.guillaume.tucker@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] i915/gem_create: Do not build create-clear for MIPS | expand |
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@collabora.com> > --- > 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? 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*. Apropos, compile-testing on MIPS in gitlab-CI? A compile-tested-only fallback mechanism suggestion, and a bad spot for placing the fallback functions: 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
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@collabora.com> >> --- >> 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
On 03/04/2019 08:25, Guillaume Tucker wrote: > 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@collabora.com> >>> --- >>> 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 Just to close this thread, I sent some patches earlier this week to fix the build on MIPS using the more recent __atomic_* functions from libatomic as they do have the 64-bit implementations for MIPS that were missing for the __sync_* functions (tested with gcc 8.3). I've also just sent a patch to enable Gitlab CI builds for MIPS, it's all passing: https://gitlab.freedesktop.org/gtucker/igt-gpu-tools/pipelines/40827 Guillaume
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 }
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@collabora.com> --- tests/i915/gem_create.c | 4 ++++ 1 file changed, 4 insertions(+)