diff mbox series

RFC: Make igts for cross-driver stuff mandatory?

Message ID 20181019085049.25482-1-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show
Series RFC: Make igts for cross-driver stuff mandatory? | expand

Commit Message

Daniel Vetter Oct. 19, 2018, 8:50 a.m. UTC
Hi all,

This is just to collect feedback on this idea, and see whether the
overall dri-devel community stands on all this. I think the past few
cross-vendor uapi extensions all came with igts attached, and
personally I think there's lots of value in having them: A
cross-vendor interface isn't useful if every driver implements it
slightly differently.

I think there's 2 questions here:

- Do we want to make such testcases mandatory?

- If yes, are we there yet, or is there something crucially missing
  still?

And of course there's a bunch of details to figure out. Like we
probably want to also recommend the selftests/unit-tests in
drivers/gpu/drm/selftest, since fairly often that's a much more
effective approach to checking the details than from userspace.

Feedback and thoughts very much appreciated.

Cheers, Daniel
---
 Documentation/gpu/drm-uapi.rst | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Liviu Dudau Oct. 25, 2018, 9:58 a.m. UTC | #1
On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> Hi all,

Hi,

(Replying from my personal address as the work email seems to have let
this one go to /dev/null)

> 
> This is just to collect feedback on this idea, and see whether the
> overall dri-devel community stands on all this. I think the past few
> cross-vendor uapi extensions all came with igts attached, and
> personally I think there's lots of value in having them: A
> cross-vendor interface isn't useful if every driver implements it
> slightly differently.

I would like to also get some clarification on where we are standing on 
"tests vs 'real code'" stanza. Does making igt tests mandatory replace
the need for 'real code' or does it add to the list of requirements? If
the later, then I think the bar rises in terms of showing igts' 
usefulness / benefits.

> 
> I think there's 2 questions here:
> 
> - Do we want to make such testcases mandatory?

I'm a bit reluctant to make it so by fiat. I think that showing the
usefulness of having igts tests to newcomers (by adding with this patch
some more information about why IGT is a good place to add your testing to)
and getting more mature drivers to get tested under IGT on a regular basis
would make adoption of IGT for testing a community standard.

> 
> - If yes, are we there yet, or is there something crucially missing
>   still?

I'm just getting back into IGT by refreshing the writeback patches, but
by looking at the commit log I get the impression that there aren't that
many patches that target drivers other than Intel's. Are all the non-Intel
patches so generic that one doesn't need to specify a target driver for
those changes (in which case great, but then why is Intel's so different?),
or are the others not bothered with IGT support?

At the moment I'm a bit on the fence on this. Not having spent too much
time with IGT in the last 6 months, I'm probably closer to a newcomer in
my attitude towards IGT and at the moment I'm not clear on how to answer the
"Why?" and "What is in it for me?" questions.

Best regards,
Liviu

> 
> And of course there's a bunch of details to figure out. Like we
> probably want to also recommend the selftests/unit-tests in
> drivers/gpu/drm/selftest, since fairly often that's a much more
> effective approach to checking the details than from userspace.
> 
> Feedback and thoughts very much appreciated.
> 
> Cheers, Daniel
> ---
>  Documentation/gpu/drm-uapi.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 4b4bf2c5eac5..91cf6e4b6303 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -238,6 +238,13 @@ DRM specific patterns. Note that ENOTTY has the slightly unintuitive meaning of
>  Testing and validation
>  ======================
>  
> +Testing Requirements for userspace API
> +--------------------------------------
> +
> +New cross-driver userspace interface extensions, like new IOCTL, new KMS
> +properties, new files in sysfs or anything else that constitutes an API change
> +need to have driver-agnostic testcases in IGT for that feature.
> +
>  Validating changes with IGT
>  ---------------------------
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Oct. 25, 2018, 10:31 a.m. UTC | #2
On Thu, Oct 25, 2018 at 11:58 AM Liviu Dudau <liviu@dudau.co.uk> wrote:
> On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> > Hi all,
>
> Hi,
>
> (Replying from my personal address as the work email seems to have let
> this one go to /dev/null)
>
> >
> > This is just to collect feedback on this idea, and see whether the
> > overall dri-devel community stands on all this. I think the past few
> > cross-vendor uapi extensions all came with igts attached, and
> > personally I think there's lots of value in having them: A
> > cross-vendor interface isn't useful if every driver implements it
> > slightly differently.
>
> I would like to also get some clarification on where we are standing on
> "tests vs 'real code'" stanza. Does making igt tests mandatory replace
> the need for 'real code' or does it add to the list of requirements? If
> the later, then I think the bar rises in terms of showing igts'
> usefulness / benefits.

It would be in addition. The "real code" userspace is for validating
the overall design. Igt (and unit tests) are more for checking all the
details, error handling, and having a regression test suite going
forward.

> > I think there's 2 questions here:
> >
> > - Do we want to make such testcases mandatory?
>
> I'm a bit reluctant to make it so by fiat. I think that showing the
> usefulness of having igts tests to newcomers (by adding with this patch
> some more information about why IGT is a good place to add your testing to)
> and getting more mature drivers to get tested under IGT on a regular basis
> would make adoption of IGT for testing a community standard.

There's 2 motivations here for me:
- Most of the generic features in the past 1-2 did come with igts, but
sometimes those igts have been treated as 2nd class and forgotten. I
think it's at least the right time to discuss whether we should make
them an integral part of uapi development.

- The other bit is that our intel CI is catching a lot of regressions,
and people have started to cc: intel-gfx to get their non-intel
patches validated too. So all these igts we have do seem to provide
real value in keeping things working.

> > - If yes, are we there yet, or is there something crucially missing
> >   still?
>
> I'm just getting back into IGT by refreshing the writeback patches, but
> by looking at the commit log I get the impression that there aren't that
> many patches that target drivers other than Intel's. Are all the non-Intel
> patches so generic that one doesn't need to specify a target driver for
> those changes (in which case great, but then why is Intel's so different?),
> or are the others not bothered with IGT support?

So this discussion isn't about igt overall, but just about
cross-driver features. The tests we have in igt for intel are a lot
more:
- We also validate all the intel-specific bits with igts.
- We validate all the interactions between generic stuff and
intel-specific uapi with igts.
- We validate hw features with igt.

And finally we've made igts mandatory for all things intel more than 5
years ago. So there's considerable head start.

The other side is that I'm only suggesting that we make igts mandatory
for cross-driver interfaces. Naturally the tests for those aren't
aimed at a specific driver, but use the DRIVER_ANY flag. And the work
needed to make that work on a specific driver is usually rather small
- e.g. vmwgfx (which doesn't support GEM at all) required very small
changes to get things going. Now we do have some tests for other
drivers (vc4 and a few amdgpu), but not anything else. So I'd say the
lack of non-intel targetted patches is a good thing.

> At the moment I'm a bit on the fence on this. Not having spent too much
> time with IGT in the last 6 months, I'm probably closer to a newcomer in
> my attitude towards IGT and at the moment I'm not clear on how to answer the
> "Why?" and "What is in it for me?" questions.

Well all the usual reasons for testing:
- Only way to make sure different implementations are working correct.
- Only real way to make sure regressions are caught before everything
is on fire.

Of course this gets a lot better if there's also CI running them
(which we do for intel, and are open to anyone submitting stuff to
it), but just having the tests is already a big step.

Cheers, Daniel

> Best regards,
> Liviu
>
> >
> > And of course there's a bunch of details to figure out. Like we
> > probably want to also recommend the selftests/unit-tests in
> > drivers/gpu/drm/selftest, since fairly often that's a much more
> > effective approach to checking the details than from userspace.
> >
> > Feedback and thoughts very much appreciated.
> >
> > Cheers, Daniel
> > ---
> >  Documentation/gpu/drm-uapi.rst | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 4b4bf2c5eac5..91cf6e4b6303 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -238,6 +238,13 @@ DRM specific patterns. Note that ENOTTY has the slightly unintuitive meaning of
> >  Testing and validation
> >  ======================
> >
> > +Testing Requirements for userspace API
> > +--------------------------------------
> > +
> > +New cross-driver userspace interface extensions, like new IOCTL, new KMS
> > +properties, new files in sysfs or anything else that constitutes an API change
> > +need to have driver-agnostic testcases in IGT for that feature.
> > +
> >  Validating changes with IGT
> >  ---------------------------
> >
> > --
> > 2.19.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
>              /`\
>             / : |
>    _.._     | '/
>  /`    \    | /
> |  .-._ '-"` (
> |_/   /   o  o\
>       |  == () ==
>        \   -- /                       ______________________________________
>        / ---<_              ________|                                      |_______
>       |      \\             \       |  I would like to fix the world but   |      /
>       | |     \\__           \      |   no one gives me the source code.   |     /
>       / ;     |.__)          /      |______________________________________|     \
>      (_/.-.   ;             /__________)                                (_________\
>     { `|   \_/
>      '-\   / |
>         | /  |
>        /  \  '-.
>        \__|-----'
Sean Paul Oct. 25, 2018, 12:51 p.m. UTC | #3
On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> Hi all,
> 
> This is just to collect feedback on this idea, and see whether the
> overall dri-devel community stands on all this. I think the past few
> cross-vendor uapi extensions all came with igts attached, and
> personally I think there's lots of value in having them: A
> cross-vendor interface isn't useful if every driver implements it
> slightly differently.
> 
> I think there's 2 questions here:
> 
> - Do we want to make such testcases mandatory?
> 

Yes, more testing == better code.


> - If yes, are we there yet, or is there something crucially missing
>   still?

In my experience, no. Last week while trying to replicate an intel-gfx CI
failure, I tried compiling igt for one of my (intel) chromebooks. It seems like
cross-compilation (or, in my case, just specifying
prefix/ld_library_path/sbin_path) is broken on igt. If we want to impose
restrictions across the entire subsystem, we need to make sure that everyone can
build and deploy igt easily.

I managed to hack around everything and get it working, but I still haven't
tried switching out the toolchain. Once we have some GitLab CI to validate
cross-compilation, then we can consider making IGT mandatory.

It's possible that I'm just a meson n00b and didn't use the right incantation,
so maybe it already works, but then we need better documentation.

I've pasted my horrible hacks below, I also didn't have libunwind, so removed
its usage.

Sean


/snip


From ab8c7d274c32559295b38d6ceeaabded14b207d4 Mon Sep 17 00:00:00 2001
From: Sean Paul <seanpaul@chromium.org>
Date: Thu, 25 Oct 2018 08:40:28 -0400
Subject: [PATCH] igt: Hacks to compile in CrOS chroot

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 lib/igt_core.c            | 78 ---------------------------------------
 lib/meson.build           |  1 -
 meson.build               |  4 +-
 tests/gem_userptr_blits.c |  2 +
 tools/meson.build         |  7 ----
 5 files changed, 5 insertions(+), 87 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 23bb858f..ca65d7cc 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -71,8 +71,6 @@
 #include "igt_sysrq.h"
 #include "igt_rc.h"
 
-#define UNW_LOCAL_ONLY
-#include <libunwind.h>
 #include <elfutils/libdwfl.h>
 
 #ifdef HAVE_LIBGEN_H
@@ -1209,63 +1207,6 @@ static void write_stderr(const char *str)
 
 static void print_backtrace(void)
 {
-	unw_cursor_t cursor;
-	unw_context_t uc;
-	int stack_num = 0;
-
-	Dwfl_Callbacks cbs = {
-		.find_elf = dwfl_linux_proc_find_elf,
-		.find_debuginfo = dwfl_standard_find_debuginfo,
-	};
-
-	Dwfl *dwfl = dwfl_begin(&cbs);
-
-	if (dwfl_linux_proc_report(dwfl, getpid())) {
-		dwfl_end(dwfl);
-		dwfl = NULL;
-	} else
-		dwfl_report_end(dwfl, NULL, NULL);
-
-	igt_info("Stack trace:\n");
-
-	unw_getcontext(&uc);
-	unw_init_local(&cursor, &uc);
-	while (unw_step(&cursor) > 0) {
-		char name[255];
-		unw_word_t off, ip;
-		Dwfl_Module *mod = NULL;
-
-		unw_get_reg(&cursor, UNW_REG_IP, &ip);
-
-		if (dwfl)
-			mod = dwfl_addrmodule(dwfl, ip);
-
-		if (mod) {
-			const char *src, *dwfl_name;
-			Dwfl_Line *line;
-			int lineno;
-			GElf_Sym sym;
-
-			line = dwfl_module_getsrc(mod, ip);
-			dwfl_name = dwfl_module_addrsym(mod, ip, &sym, NULL);
-
-			if (line && dwfl_name) {
-				src = dwfl_lineinfo(line, NULL, &lineno, NULL, NULL, NULL);
-				igt_info("  #%d %s:%d %s()\n", stack_num++, src, lineno, dwfl_name);
-				continue;
-			}
-		}
-
-		if (unw_get_proc_name(&cursor, name, 255, &off) < 0)
-			igt_info("  #%d [<unknown>+0x%x]\n", stack_num++,
-				 (unsigned int) ip);
-		else
-			igt_info("  #%d [%s+0x%x]\n", stack_num++, name,
-				 (unsigned int) off);
-	}
-
-	if (dwfl)
-		dwfl_end(dwfl);
 }
 
 static const char hex[] = "0123456789abcdef";
@@ -1420,25 +1361,6 @@ xprintf(const char *fmt, ...)
 
 static void print_backtrace_sig_safe(void)
 {
-	unw_cursor_t cursor;
-	unw_context_t uc;
-	int stack_num = 0;
-
-	write_stderr("Stack trace: \n");
-
-	unw_getcontext(&uc);
-	unw_init_local(&cursor, &uc);
-	while (unw_step(&cursor) > 0) {
-		char name[255];
-		unw_word_t off;
-
-		if (unw_get_proc_name(&cursor, name, 255, &off) < 0)
-			xstrlcpy(name, "<unknown>", 10);
-
-		xprintf(" #%d [%s+0x%x]\n", stack_num++, name,
-				(unsigned int) off);
-
-	}
 }
 
 void __igt_fail_assert(const char *domain, const char *file, const int line,
diff --git a/lib/meson.build b/lib/meson.build
index 7e2c9b7a..26cef0c6 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -59,7 +59,6 @@ lib_deps = [
 	libkmod,
 	libprocps,
 	libudev,
-	libunwind,
 	libdw,
 	pciaccess,
 	pthreads,
diff --git a/meson.build b/meson.build
index eff35585..86ad6602 100644
--- a/meson.build
+++ b/meson.build
@@ -102,7 +102,6 @@ build_info += 'With libdrm: ' + ','.join(libdrm_info)
 pciaccess = dependency('pciaccess', version : '>=0.10')
 libkmod = dependency('libkmod')
 libprocps = dependency('libprocps', required : true)
-libunwind = dependency('libunwind', required : true)
 libdw = dependency('libdw', required : true)
 ssl = dependency('openssl', required : true)
 pixman = dependency('pixman-1', required : true)
@@ -217,12 +216,15 @@ prefix = get_option('prefix')
 bindir = get_option('bindir')
 datadir = join_paths(get_option('datadir'), 'igt-gpu-tools')
 includedir = get_option('includedir')
+fullincdir = join_paths(prefix, includedir)
 libdir = get_option('libdir')
 libexecdir = join_paths(get_option('libexecdir'), 'igt-gpu-tools')
 amdgpudir = join_paths(libexecdir, 'amdgpu')
 mandir = get_option('mandir')
 pkgconfigdir = join_paths(libdir, 'pkgconfig')
 
+inc = [ include_directories(fullincdir, join_paths( fullincdir, 'libdrm')), inc ]
+
 if get_option('use_rpath')
 	# Set up runpath for the test executables towards libigt.so.
 	# The path should be relative to $ORIGIN so the library is
diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 909dd19d..92d5a6e1 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -1069,6 +1069,7 @@ static void store_dword_rand(int i915, unsigned int engine,
 
 static void test_readonly(int i915)
 {
+#if 0
 	unsigned char orig[SHA_DIGEST_LENGTH];
 	uint64_t aperture_size;
 	uint32_t whandle, rhandle;
@@ -1178,6 +1179,7 @@ static void test_readonly(int i915)
 
 	munmap(space, total);
 	munmap(pages, sz);
+#endif
 }
 
 static jmp_buf sigjmp;
diff --git a/tools/meson.build b/tools/meson.build
index 79f36aa9..5e990cd7 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -90,11 +90,6 @@ install_subdir('registers', install_dir : datadir,
 		 'Makefile', 'Makefile.in', 'Makefile.am',
 	       ])
 
-shared_library('intel_aubdump', 'aubdump.c',
-	       dependencies : [ lib_igt_chipset, dlsym ],
-	       name_prefix : '',
-	       install : true)
-
 executable('intel_gpu_top', 'intel_gpu_top.c',
 	   install : true,
 	   install_rpath : bindir_rpathdir,
@@ -104,7 +99,5 @@ conf_data = configuration_data()
 conf_data.set('prefix', prefix)
 conf_data.set('exec_prefix', '${prefix}')
 conf_data.set('libdir', join_paths('${prefix}', libdir))
-configure_file(input : 'intel_aubdump.in', output : 'intel_aubdump',
-               configuration : conf_data, install_dir : bindir)
 
 subdir('null_state_gen')
Eric Anholt Oct. 25, 2018, 4:35 p.m. UTC | #4
Sean Paul <sean@poorly.run> writes:

> On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
>> Hi all,
>> 
>> This is just to collect feedback on this idea, and see whether the
>> overall dri-devel community stands on all this. I think the past few
>> cross-vendor uapi extensions all came with igts attached, and
>> personally I think there's lots of value in having them: A
>> cross-vendor interface isn't useful if every driver implements it
>> slightly differently.
>> 
>> I think there's 2 questions here:
>> 
>> - Do we want to make such testcases mandatory?
>> 
>
> Yes, more testing == better code.
>
>
>> - If yes, are we there yet, or is there something crucially missing
>>   still?
>
> In my experience, no. Last week while trying to replicate an intel-gfx CI
> failure, I tried compiling igt for one of my (intel) chromebooks. It seems like
> cross-compilation (or, in my case, just specifying
> prefix/ld_library_path/sbin_path) is broken on igt. If we want to impose
> restrictions across the entire subsystem, we need to make sure that everyone can
> build and deploy igt easily.
>
> I managed to hack around everything and get it working, but I still haven't
> tried switching out the toolchain. Once we have some GitLab CI to validate
> cross-compilation, then we can consider making IGT mandatory.
>
> It's possible that I'm just a meson n00b and didn't use the right incantation,
> so maybe it already works, but then we need better documentation.
>
> I've pasted my horrible hacks below, I also didn't have libunwind, so removed
> its usage.

I've also had to cut out libunwind for cross-compiling on many
occasions.  Worst library.
Chunming Zhou Oct. 26, 2018, 3:49 a.m. UTC | #5
Make igt for cross-driver, I think you should rename it first, not an intel specific. NO company wants their employee working on other company stuff.
You can rename it to DGT(drm graphics test), and published following  libdrm, or directly merge to libdrm, then everyone  can use it and develop it in same page, which is only my personal opinion. 
Daniel Vetter Oct. 26, 2018, 8:32 a.m. UTC | #6
On Fri, Oct 26, 2018 at 5:50 AM Zhou, David(ChunMing)
<David1.Zhou@amd.com> wrote:
>
> Make igt for cross-driver, I think you should rename it first, not an intel specific. NO company wants their employee working on other company stuff.
> You can rename it to DGT(drm graphics test), and published following  libdrm, or directly merge to libdrm, then everyone  can use it and develop it in same page, which is only my personal opinion.

We renamed it ot  IGT gpu tools, that was even enough for ARM folks.
If this is seriously what AMD expects before considering, I'm not sure
what to say.

Alex, Christian, is this the official AMD stance that you can't touch
stuff because of the letter i?
-Daniel


> Regards,
> David
>
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Eric
> > Anholt
> > Sent: Friday, October 26, 2018 12:36 AM
> > To: Sean Paul <sean@poorly.run>; Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: IGT development <igt-dev@lists.freedesktop.org>; Intel Graphics
> > Development <intel-gfx@lists.freedesktop.org>; DRI Development <dri-
> > devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org
> > Subject: Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff
> > mandatory?
> >
> > Sean Paul <sean@poorly.run> writes:
> >
> > > On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> > >> Hi all,
> > >>
> > >> This is just to collect feedback on this idea, and see whether the
> > >> overall dri-devel community stands on all this. I think the past few
> > >> cross-vendor uapi extensions all came with igts attached, and
> > >> personally I think there's lots of value in having them: A
> > >> cross-vendor interface isn't useful if every driver implements it
> > >> slightly differently.
> > >>
> > >> I think there's 2 questions here:
> > >>
> > >> - Do we want to make such testcases mandatory?
> > >>
> > >
> > > Yes, more testing == better code.
> > >
> > >
> > >> - If yes, are we there yet, or is there something crucially missing
> > >>   still?
> > >
> > > In my experience, no. Last week while trying to replicate an intel-gfx
> > > CI failure, I tried compiling igt for one of my (intel) chromebooks.
> > > It seems like cross-compilation (or, in my case, just specifying
> > > prefix/ld_library_path/sbin_path) is broken on igt. If we want to
> > > impose restrictions across the entire subsystem, we need to make sure
> > > that everyone can build and deploy igt easily.
> > >
> > > I managed to hack around everything and get it working, but I still
> > > haven't tried switching out the toolchain. Once we have some GitLab CI
> > > to validate cross-compilation, then we can consider making IGT mandatory.
> > >
> > > It's possible that I'm just a meson n00b and didn't use the right
> > > incantation, so maybe it already works, but then we need better
> > documentation.
> > >
> > > I've pasted my horrible hacks below, I also didn't have libunwind, so
> > > removed its usage.
> >
> > I've also had to cut out libunwind for cross-compiling on many occasions.
> > Worst library.
Chunming Zhou Oct. 26, 2018, 8:48 a.m. UTC | #7
On 2018年10月26日 16:32, Daniel Vetter wrote:
> On Fri, Oct 26, 2018 at 5:50 AM Zhou, David(ChunMing)
> <David1.Zhou@amd.com> wrote:
>> Make igt for cross-driver, I think you should rename it first, not an intel specific. NO company wants their employee working on other company stuff.
>> You can rename it to DGT(drm graphics test), and published following  libdrm, or directly merge to libdrm, then everyone  can use it and develop it in same page, which is only my personal opinion.
> We renamed it ot  IGT gpu tools, that was even enough for ARM folks.
> If this is seriously what AMD expects before considering, I'm not sure
> what to say.
>
> Alex, Christian, is this the official AMD stance that you can't touch
> stuff because of the letter i?
Nope, as I said last, this is just my personal thought. And I'm not sure 
what opinion of others.

-David
> -Daniel
>
>
>> Regards,
>> David
>>
>>> -----Original Message-----
>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Eric
>>> Anholt
>>> Sent: Friday, October 26, 2018 12:36 AM
>>> To: Sean Paul <sean@poorly.run>; Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: IGT development <igt-dev@lists.freedesktop.org>; Intel Graphics
>>> Development <intel-gfx@lists.freedesktop.org>; DRI Development <dri-
>>> devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff
>>> mandatory?
>>>
>>> Sean Paul <sean@poorly.run> writes:
>>>
>>>> On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
>>>>> Hi all,
>>>>>
>>>>> This is just to collect feedback on this idea, and see whether the
>>>>> overall dri-devel community stands on all this. I think the past few
>>>>> cross-vendor uapi extensions all came with igts attached, and
>>>>> personally I think there's lots of value in having them: A
>>>>> cross-vendor interface isn't useful if every driver implements it
>>>>> slightly differently.
>>>>>
>>>>> I think there's 2 questions here:
>>>>>
>>>>> - Do we want to make such testcases mandatory?
>>>>>
>>>> Yes, more testing == better code.
>>>>
>>>>
>>>>> - If yes, are we there yet, or is there something crucially missing
>>>>>    still?
>>>> In my experience, no. Last week while trying to replicate an intel-gfx
>>>> CI failure, I tried compiling igt for one of my (intel) chromebooks.
>>>> It seems like cross-compilation (or, in my case, just specifying
>>>> prefix/ld_library_path/sbin_path) is broken on igt. If we want to
>>>> impose restrictions across the entire subsystem, we need to make sure
>>>> that everyone can build and deploy igt easily.
>>>>
>>>> I managed to hack around everything and get it working, but I still
>>>> haven't tried switching out the toolchain. Once we have some GitLab CI
>>>> to validate cross-compilation, then we can consider making IGT mandatory.
>>>>
>>>> It's possible that I'm just a meson n00b and didn't use the right
>>>> incantation, so maybe it already works, but then we need better
>>> documentation.
>>>> I've pasted my horrible hacks below, I also didn't have libunwind, so
>>>> removed its usage.
>>> I've also had to cut out libunwind for cross-compiling on many occasions.
>>> Worst library.
>
>
Alex Deucher Oct. 26, 2018, 2:41 p.m. UTC | #8
On Fri, Oct 26, 2018 at 4:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Fri, Oct 26, 2018 at 5:50 AM Zhou, David(ChunMing)
> <David1.Zhou@amd.com> wrote:
> >
> > Make igt for cross-driver, I think you should rename it first, not an intel specific. NO company wants their employee working on other company stuff.
> > You can rename it to DGT(drm graphics test), and published following  libdrm, or directly merge to libdrm, then everyone  can use it and develop it in same page, which is only my personal opinion.
>
> We renamed it ot  IGT gpu tools, that was even enough for ARM folks.
> If this is seriously what AMD expects before considering, I'm not sure
> what to say.
>
> Alex, Christian, is this the official AMD stance that you can't touch
> stuff because of the letter i?

We don't have any restrictions.

Alex

> -Daniel
>
>
> > Regards,
> > David
> >
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Eric
> > > Anholt
> > > Sent: Friday, October 26, 2018 12:36 AM
> > > To: Sean Paul <sean@poorly.run>; Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: IGT development <igt-dev@lists.freedesktop.org>; Intel Graphics
> > > Development <intel-gfx@lists.freedesktop.org>; DRI Development <dri-
> > > devel@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org
> > > Subject: Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff
> > > mandatory?
> > >
> > > Sean Paul <sean@poorly.run> writes:
> > >
> > > > On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> > > >> Hi all,
> > > >>
> > > >> This is just to collect feedback on this idea, and see whether the
> > > >> overall dri-devel community stands on all this. I think the past few
> > > >> cross-vendor uapi extensions all came with igts attached, and
> > > >> personally I think there's lots of value in having them: A
> > > >> cross-vendor interface isn't useful if every driver implements it
> > > >> slightly differently.
> > > >>
> > > >> I think there's 2 questions here:
> > > >>
> > > >> - Do we want to make such testcases mandatory?
> > > >>
> > > >
> > > > Yes, more testing == better code.
> > > >
> > > >
> > > >> - If yes, are we there yet, or is there something crucially missing
> > > >>   still?
> > > >
> > > > In my experience, no. Last week while trying to replicate an intel-gfx
> > > > CI failure, I tried compiling igt for one of my (intel) chromebooks.
> > > > It seems like cross-compilation (or, in my case, just specifying
> > > > prefix/ld_library_path/sbin_path) is broken on igt. If we want to
> > > > impose restrictions across the entire subsystem, we need to make sure
> > > > that everyone can build and deploy igt easily.
> > > >
> > > > I managed to hack around everything and get it working, but I still
> > > > haven't tried switching out the toolchain. Once we have some GitLab CI
> > > > to validate cross-compilation, then we can consider making IGT mandatory.
> > > >
> > > > It's possible that I'm just a meson n00b and didn't use the right
> > > > incantation, so maybe it already works, but then we need better
> > > documentation.
> > > >
> > > > I've pasted my horrible hacks below, I also didn't have libunwind, so
> > > > removed its usage.
> > >
> > > I've also had to cut out libunwind for cross-compiling on many occasions.
> > > Worst library.
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Alex Deucher Oct. 26, 2018, 3:28 p.m. UTC | #9
On Fri, Oct 19, 2018 at 4:51 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> Hi all,
>
> This is just to collect feedback on this idea, and see whether the
> overall dri-devel community stands on all this. I think the past few
> cross-vendor uapi extensions all came with igts attached, and
> personally I think there's lots of value in having them: A
> cross-vendor interface isn't useful if every driver implements it
> slightly differently.
>
> I think there's 2 questions here:
>
> - Do we want to make such testcases mandatory?
>
> - If yes, are we there yet, or is there something crucially missing
>   still?
>
> And of course there's a bunch of details to figure out. Like we
> probably want to also recommend the selftests/unit-tests in
> drivers/gpu/drm/selftest, since fairly often that's a much more
> effective approach to checking the details than from userspace.
>
> Feedback and thoughts very much appreciated.

As long as we can fix up any cross platform issues, this seems reasonable to me.

Alex

>
> Cheers, Daniel
> ---
>  Documentation/gpu/drm-uapi.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 4b4bf2c5eac5..91cf6e4b6303 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -238,6 +238,13 @@ DRM specific patterns. Note that ENOTTY has the slightly unintuitive meaning of
>  Testing and validation
>  ======================
>
> +Testing Requirements for userspace API
> +--------------------------------------
> +
> +New cross-driver userspace interface extensions, like new IOCTL, new KMS
> +properties, new files in sysfs or anything else that constitutes an API change
> +need to have driver-agnostic testcases in IGT for that feature.
> +
>  Validating changes with IGT
>  ---------------------------
>
> --
> 2.19.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Dave Airlie Oct. 30, 2018, 2:17 a.m. UTC | #10
On Fri, 19 Oct 2018 at 18:51, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> Hi all,
>
> This is just to collect feedback on this idea, and see whether the
> overall dri-devel community stands on all this. I think the past few
> cross-vendor uapi extensions all came with igts attached, and
> personally I think there's lots of value in having them: A
> cross-vendor interface isn't useful if every driver implements it
> slightly differently.
>
> I think there's 2 questions here:
>
> - Do we want to make such testcases mandatory?

Yes I think if at all practical it probably makes sense to have some
mandatory test cases for all cross-vendor features, or features that
might become cross vendor in the future.

>
> - If yes, are we there yet, or is there something crucially missing
>   still?

I think the does igt build in all the places needed is the main one,
I've no idea what a baseline IGT test run looks like on non-intel hw,
how useful is it?

Acked-by: Dave Airlie <airlied@redhat.com>
Daniel Vetter Oct. 30, 2018, 8:54 a.m. UTC | #11
On Tue, Oct 30, 2018 at 12:17:30PM +1000, Dave Airlie wrote:
> On Fri, 19 Oct 2018 at 18:51, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > Hi all,
> >
> > This is just to collect feedback on this idea, and see whether the
> > overall dri-devel community stands on all this. I think the past few
> > cross-vendor uapi extensions all came with igts attached, and
> > personally I think there's lots of value in having them: A
> > cross-vendor interface isn't useful if every driver implements it
> > slightly differently.
> >
> > I think there's 2 questions here:
> >
> > - Do we want to make such testcases mandatory?
> 
> Yes I think if at all practical it probably makes sense to have some
> mandatory test cases for all cross-vendor features, or features that
> might become cross vendor in the future.

I've created a few patches to test that in gitlab CI. I think the only
thing left now is CI'ing sysroot builds, but I don't know how to do that
myself.

> > - If yes, are we there yet, or is there something crucially missing
> >   still?
> 
> I think the does igt build in all the places needed is the main one,
> I've no idea what a baseline IGT test run looks like on non-intel hw,
> how useful is it?

We're in the process of moving i915 tests into a tests/i915/ subfolder. I
think after that we could try to them on some hardware (my long term plan
is to use vkms for that and put it into gitlab CI with qemu). We have
accidentally run igts on amdgpu instead of i915 on KBL-G (and our CI found
at least one bug in one of my refactor series), so stuff works :-)

Coverage is a bit a mixed bag I think, but that's always the case when you
retrofit a testsuite.
-Daniel

> 
> Acked-by: Dave Airlie <airlied@redhat.com>
diff mbox series

Patch

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 4b4bf2c5eac5..91cf6e4b6303 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -238,6 +238,13 @@  DRM specific patterns. Note that ENOTTY has the slightly unintuitive meaning of
 Testing and validation
 ======================
 
+Testing Requirements for userspace API
+--------------------------------------
+
+New cross-driver userspace interface extensions, like new IOCTL, new KMS
+properties, new files in sysfs or anything else that constitutes an API change
+need to have driver-agnostic testcases in IGT for that feature.
+
 Validating changes with IGT
 ---------------------------