| Message ID | 1454059293-16452-1-git-send-email-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote: > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > gem_require_ring will submit an execbuf using the provided flags and > skip the test if the ioctl fails. This test is however designed to catch > issues with the ioctl, so it should fail if the ioctl fails on a ring > that the HW possesses. > > Instead of using gem_require_ring we can use the getparam ioctl. The new > checker has been added to the test file and not to the commmon library > because this test is the only special case where we want to not use > gem_has_ring That would be gem_exec_param. -Chris
On 29/01/16 10:58, Chris Wilson wrote: > On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote: >> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> gem_require_ring will submit an execbuf using the provided flags and >> skip the test if the ioctl fails. This test is however designed to catch >> issues with the ioctl, so it should fail if the ioctl fails on a ring >> that the HW possesses. >> >> Instead of using gem_require_ring we can use the getparam ioctl. The new >> checker has been added to the test file and not to the commmon library >> because this test is the only special case where we want to not use >> gem_has_ring > That would be gem_exec_param. > -Chris I don't understand what you mean, can you elaborate a bit? What I wanted to fix here is the fact that the logic to skip the test and the test itself are identical, which means that this test can't fail. As far as I can tell gem_exec_param is trying to catch errors in the handling of invalid flags, while in this test we check for errors in the handling of valid flags instead. Thanks, Daniele
On Fri, Jan 29, 2016 at 11:16:37AM +0000, Daniele Ceraolo Spurio wrote: > > > On 29/01/16 10:58, Chris Wilson wrote: > >On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote: > >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> > >>gem_require_ring will submit an execbuf using the provided flags and > >>skip the test if the ioctl fails. This test is however designed to catch > >>issues with the ioctl, so it should fail if the ioctl fails on a ring > >>that the HW possesses. > >> > >>Instead of using gem_require_ring we can use the getparam ioctl. The new > >>checker has been added to the test file and not to the commmon library > >>because this test is the only special case where we want to not use > >>gem_has_ring > >That would be gem_exec_param. > >-Chris > > I don't understand what you mean, can you elaborate a bit? For the purposes of checking that the kernel honours the ABI, the tests belong in gem_exec_params. For the purposes of CI, a testing going from PASS -> SKIP is just as indicative of a problem as test going from PASS -> FAIL or any other state. > What I wanted to fix here is the fact that the logic to skip the > test and the test itself are identical, which means that this test > can't fail. As far as I can tell gem_exec_param is trying to catch > errors in the handling of invalid flags, while in this test we check > for errors in the handling of valid flags instead. Basically the logic is repeated, that is not an issue for its purpose. -Chris
On 29/01/16 11:35, Chris Wilson wrote: > On Fri, Jan 29, 2016 at 11:16:37AM +0000, Daniele Ceraolo Spurio wrote: >> >> On 29/01/16 10:58, Chris Wilson wrote: >>> On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote: >>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> >>>> gem_require_ring will submit an execbuf using the provided flags and >>>> skip the test if the ioctl fails. This test is however designed to catch >>>> issues with the ioctl, so it should fail if the ioctl fails on a ring >>>> that the HW possesses. >>>> >>>> Instead of using gem_require_ring we can use the getparam ioctl. The new >>>> checker has been added to the test file and not to the commmon library >>>> because this test is the only special case where we want to not use >>>> gem_has_ring >>> That would be gem_exec_param. >>> -Chris >> I don't understand what you mean, can you elaborate a bit? > For the purposes of checking that the kernel honours the ABI, the tests > belong in gem_exec_params. > > For the purposes of CI, a testing going from PASS -> SKIP is just as > indicative of a problem as test going from PASS -> FAIL or any other > state. The difference would be that the CI system still reports that BAT succeeded if one or more tests go from PASS to SKIP (e.g. http://lists.freedesktop.org/archives/intel-gfx/2016-January/086586.html). > >> What I wanted to fix here is the fact that the logic to skip the >> test and the test itself are identical, which means that this test >> can't fail. As far as I can tell gem_exec_param is trying to catch >> errors in the handling of invalid flags, while in this test we check >> for errors in the handling of valid flags instead. > Basically the logic is repeated, that is not an issue for its purpose. > -Chris This patch can be dropped then. Thanks, Daniele
On Fri, Jan 29, 2016 at 11:58:14AM +0000, Daniele Ceraolo Spurio wrote: > > > On 29/01/16 11:35, Chris Wilson wrote: > >On Fri, Jan 29, 2016 at 11:16:37AM +0000, Daniele Ceraolo Spurio wrote: > >> > >>On 29/01/16 10:58, Chris Wilson wrote: > >>>On Fri, Jan 29, 2016 at 09:21:33AM +0000, daniele.ceraolospurio@intel.com wrote: > >>>>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >>>> > >>>>gem_require_ring will submit an execbuf using the provided flags and > >>>>skip the test if the ioctl fails. This test is however designed to catch > >>>>issues with the ioctl, so it should fail if the ioctl fails on a ring > >>>>that the HW possesses. > >>>> > >>>>Instead of using gem_require_ring we can use the getparam ioctl. The new > >>>>checker has been added to the test file and not to the commmon library > >>>>because this test is the only special case where we want to not use > >>>>gem_has_ring > >>>That would be gem_exec_param. > >>>-Chris > >>I don't understand what you mean, can you elaborate a bit? > >For the purposes of checking that the kernel honours the ABI, the tests > >belong in gem_exec_params. > > > >For the purposes of CI, a testing going from PASS -> SKIP is just as > >indicative of a problem as test going from PASS -> FAIL or any other > >state. > > The difference would be that the CI system still reports that BAT > succeeded if one or more tests go from PASS to SKIP (e.g. http://lists.freedesktop.org/archives/intel-gfx/2016-January/086586.html). Fortunately, the results are sometimes even read. > >>What I wanted to fix here is the fact that the logic to skip the > >>test and the test itself are identical, which means that this test > >>can't fail. As far as I can tell gem_exec_param is trying to catch > >>errors in the handling of invalid flags, while in this test we check > >>for errors in the handling of valid flags instead. > >Basically the logic is repeated, that is not an issue for its purpose. > >-Chris > > This patch can be dropped then. But can be refactored for gem_exec_param! -Chris
diff --git a/tests/gem_exec_basic.c b/tests/gem_exec_basic.c index 3f91b78..9a5de64 100644 --- a/tests/gem_exec_basic.c +++ b/tests/gem_exec_basic.c @@ -25,6 +25,30 @@ IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl rings."); +static bool has_ring(int fd, unsigned ring) +{ + switch (ring & I915_EXEC_RING_MASK) { + case 0: + case I915_EXEC_RENDER: + return true; + + case I915_EXEC_BSD: + if (ring & 3<<13) + return gem_has_bsd2(fd); + else + return gem_has_bsd(fd); + + case I915_EXEC_BLT: + return gem_has_blt(fd); + + case I915_EXEC_VEBOX: + return gem_has_vebox(fd); + } + + igt_assert_f(0, "invalid exec flag 0x%x\n", ring); + return false; +} + static void noop(int fd, unsigned ring) { uint32_t bbe = MI_BATCH_BUFFER_END; @@ -32,7 +56,11 @@ static void noop(int fd, unsigned ring) struct drm_i915_gem_exec_object2 exec; int ret; - gem_require_ring(fd, ring); + /* we can't use gem_require_ring here because otherwise the test will + * skip if there is a bug with the flags, but we want to fail in that + * situation + */ + igt_require(has_ring(fd, ring)); memset(&exec, 0, sizeof(exec)); exec.handle = gem_create(fd, 4096);