diff mbox series

[i-g-t] i915/gem_create: Do not build create-clear for MIPS

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

Commit Message

Guillaume Tucker April 1, 2019, 2:39 p.m. UTC
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(+)

Comments

Petri Latvala April 2, 2019, 8:35 a.m. UTC | #1
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
Guillaume Tucker April 3, 2019, 7:25 a.m. UTC | #2
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
Guillaume Tucker June 5, 2019, 8:46 p.m. UTC | #3
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 mbox series

Patch

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
 }