| Message ID | 20170908063824.31159-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Daniel Vetter (2017-09-08 07:38:24) > The function is perhaps a bit renamed, but if a previous tests failed > so badly it left the gpu wrecked, then we should fail, not skip. > > Testcases shouldn't ever randomly skip at least, since that just > indicates whether the feature is there or not. Pass/fail is for > whether it's working or not. Not quite, now you are randomly failing a test even before it is testing anything. Should we fail every test just because we can't open an fd? -Chris
On Fri, Sep 08, 2017 at 09:10:54AM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-09-08 07:38:24) > > The function is perhaps a bit renamed, but if a previous tests failed > > so badly it left the gpu wrecked, then we should fail, not skip. > > > > Testcases shouldn't ever randomly skip at least, since that just > > indicates whether the feature is there or not. Pass/fail is for > > whether it's working or not. > > Not quite, now you are randomly failing a test even before it is testing > anything. Should we fail every test just because we can't open an fd? Well I guess the smart choice would be to fail on -EIO and skip on -ENODEV, but given that we have a lack of hw that returns -ENODEV I'm not sure we should bake that in. We do have gt without display, not (yet, maybe it'll happen) display without GT. gma500 would be such a thing, but that one got its own driver. I'll respin. -Daniel
Quoting Daniel Vetter (2017-09-08 09:58:59) > On Fri, Sep 08, 2017 at 09:10:54AM +0100, Chris Wilson wrote: > > Quoting Daniel Vetter (2017-09-08 07:38:24) > > > The function is perhaps a bit renamed, but if a previous tests failed > > > so badly it left the gpu wrecked, then we should fail, not skip. > > > > > > Testcases shouldn't ever randomly skip at least, since that just > > > indicates whether the feature is there or not. Pass/fail is for > > > whether it's working or not. > > > > Not quite, now you are randomly failing a test even before it is testing > > anything. Should we fail every test just because we can't open an fd? > > Well I guess the smart choice would be to fail on -EIO and skip on > -ENODEV, but given that we have a lack of hw that returns -ENODEV I'm not > sure we should bake that in. We do have gt without display, not (yet, > maybe it'll happen) display without GT. gma500 would be such a thing, but > that one got its own driver. I just want to be able to quickly discern the victims from the culprit. If you report an error from the test before it was run, that I find very misleading as you start to think about all the things that could go wrong in the test and spend an hour before noticing that it wasn't run in the first place. (Whether that's a WARN or a FAIL, you still have to process a test that wasn't run.) And it is a requirement failure, we skip the test because the system isn't able to run it. It's the same issue as not being able to run a test after a massive leak. I understand that you want to differentiate the unexpected skips, but define unexpected; that's a matter of comparing history. Back to the topic of require_gem, I would like to pull gem_quiescent_gpu into here. Then at least you can do: igt_require(THOTTLE_IOCTL == 0); do_gpu_idle() igt_assert(THOTTLE_IOCTL == 0); Along with the goodness of only stalling to flush the gpu when using it (similarly for the atexit handler) -Chris
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 48750427a0c1..11ae85b26ccd 100644 --- a/lib/ioctl_wrappers.c +++ b/lib/ioctl_wrappers.c @@ -1604,7 +1604,7 @@ void igt_require_gem(int fd) if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE)) err = -errno; - igt_require_f(err == 0, "Unresponsive i915/GEM device\n"); + igt_assert_f(err == 0, "Unresponsive i915/GEM device\n"); } bool gem_has_ring(int fd, unsigned ring)
The function is perhaps a bit renamed, but if a previous tests failed so badly it left the gpu wrecked, then we should fail, not skip. Testcases shouldn't ever randomly skip at least, since that just indicates whether the feature is there or not. Pass/fail is for whether it's working or not. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Petri Latvala <petri.latvala@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- lib/ioctl_wrappers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)