| Message ID | 20181011123027.12203-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] tests/gem_set_tiling_vs_pwrite: Skip on unknown swizzling | expand |
Quoting Tvrtko Ursulin (2018-10-11 13:30:27) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Same as in commit 78071c2fa53d ("igt/gem_tiled_partial_pwrite_pread: Check > for known swizzling"), to be able to compare the bo against the test > pattern we need to skip the test if the swizzling is not compatible. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102575 > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > Eeek confidence level low - is this correct? Actually this is a slightly more subtle problem, https://patchwork.freedesktop.org/patch/240327/ -Chris
On 11/10/2018 13:34, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-10-11 13:30:27) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Same as in commit 78071c2fa53d ("igt/gem_tiled_partial_pwrite_pread: Check >> for known swizzling"), to be able to compare the bo against the test >> pattern we need to skip the test if the swizzling is not compatible. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102575 >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> Eeek confidence level low - is this correct? > > Actually this is a slightly more subtle problem, > https://patchwork.freedesktop.org/patch/240327/ So assuming your patch is in, this IGT cannot actually work if any swizzling is in effect? Or without your patch, it should skip based on known_swizzling? Or is even that not enough? Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-10-11 13:42:31) > > On 11/10/2018 13:34, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-10-11 13:30:27) > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> Same as in commit 78071c2fa53d ("igt/gem_tiled_partial_pwrite_pread: Check > >> for known swizzling"), to be able to compare the bo against the test > >> pattern we need to skip the test if the swizzling is not compatible. > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102575 > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> --- > >> Eeek confidence level low - is this correct? > > > > Actually this is a slightly more subtle problem, > > https://patchwork.freedesktop.org/patch/240327/ > > So assuming your patch is in, this IGT cannot actually work if any > swizzling is in effect? With the kernel patch, pwrite is a pure linear write to physical address, not guaranteeing layout on the gpu (the gpu sees it through the tile + swizzle combo). So set-tiling no longer affects pwrite and the test should see identical in/out. > Or without your patch, it should skip based on > known_swizzling? Or is even that not enough? Without the patch, we would need to take swizzling into account and so skip as we cannot know the swizzling in this case. The test illustrates that currently we do the swizzling asymmetrically, our API is bust. The test further illustrates that at times we cannot even know when swizzling is required (due to L-shape eccentricities and the swizzling changing within an object) and so the concept of handling swizzling on the API boundary on behalf of the user is unworkable. -Chris
On 11/10/2018 13:49, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-10-11 13:42:31) >> >> On 11/10/2018 13:34, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-10-11 13:30:27) >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> Same as in commit 78071c2fa53d ("igt/gem_tiled_partial_pwrite_pread: Check >>>> for known swizzling"), to be able to compare the bo against the test >>>> pattern we need to skip the test if the swizzling is not compatible. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102575 >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> --- >>>> Eeek confidence level low - is this correct? >>> >>> Actually this is a slightly more subtle problem, >>> https://patchwork.freedesktop.org/patch/240327/ >> >> So assuming your patch is in, this IGT cannot actually work if any >> swizzling is in effect? > > With the kernel patch, pwrite is a pure linear write to physical > address, not guaranteeing layout on the gpu (the gpu sees it through the > tile + swizzle combo). So set-tiling no longer affects pwrite and the > test should see identical in/out. Yes of course.. >> Or without your patch, it should skip based on >> known_swizzling? Or is even that not enough? > > Without the patch, we would need to take swizzling into account and so > skip as we cannot know the swizzling in this case. The test illustrates > that currently we do the swizzling asymmetrically, our API is bust. The > test further illustrates that at times we cannot even know when swizzling > is required (due to L-shape eccentricities and the swizzling changing > within an object) and so the concept of handling swizzling on the API > boundary on behalf of the user is unworkable. Sounds believable to me. So I am happy to review that patch on the technical level, just ask you to gather some more acks. Sounds like a plan? Is there any risk that there were no bug reports due the affected platform being old? Regards, Tvrtko
diff --git a/tests/gem_set_tiling_vs_pwrite.c b/tests/gem_set_tiling_vs_pwrite.c index f0126b6484b7..3be908dbd103 100644 --- a/tests/gem_set_tiling_vs_pwrite.c +++ b/tests/gem_set_tiling_vs_pwrite.c @@ -42,6 +42,18 @@ IGT_TEST_DESCRIPTION("Check set_tiling vs pwrite coherency."); #define OBJECT_SIZE (1024*1024) #define TEST_STRIDE (1024*4) +static bool known_swizzling(int fd, uint32_t handle) +{ + struct drm_i915_gem_get_tiling arg = { + .handle = handle, + }; + + if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_GET_TILING, &arg)) + return false; + + return arg.phys_swizzle_mode == arg.swizzle_mode; +} + /** * Testcase: Check set_tiling vs pwrite coherency */ @@ -66,6 +78,12 @@ igt_simple_main gem_set_tiling(fd, handle, I915_TILING_X, TEST_STRIDE); + /* + * As we want to compare our template pattern against + * the target bo, we need consistent swizzling on both. + */ + igt_require(known_swizzling(fd, handle)); + /* touch it */ gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT); *ptr = 0xdeadbeef;