RFC: Make igts for cross-driver stuff mandatory?

Submitted by Daniel Vetter on Oct. 19, 2018, 8:50 a.m.

Details

Message ID 20181019085049.25482-1-daniel.vetter@ffwll.ch
State New
Headers show
Series "RFC: Make igts for cross-driver stuff mandatory?" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Daniel Vetter Oct. 19, 2018, 8:50 a.m.
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(+)

Patch hide | download patch | download mbox

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
 ---------------------------
 

Comments

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
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.   |     /
>       / ;     |.__)          /      |______________________________________|     \
>      (_/.-.   ;             /__________)                                (_________\
>     { `|   \_/
>      '-\   / |
>         | /  |
>        /  \  '-.
>        \__|-----'
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
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>
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>