[Intel-gfx] [PATCH i-g-t] tests/pm_rpm: Fix CRASH on machines that lack LLC

[Chris Wilson]

On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote:
> On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote:
> > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote:
> > > On machines that lack an LLC the pm-caching subtest will
> > > terminate with sigbus and thus CRASH during the
> > > I915_CACHING_CACHED iteration.  This patch adds a check for
> > > this condition and skips that iteration.
> > 
> > you can delete the got_caching assertion and
> > enable_one_screen_and_wait() as well, they are not exercising the
> > associated code.
> 
> Hmmm.  How about the matching disable_all_screens_and_wait()?
> Also, isn't the got_caching assertion meant to check that
> when we enable GEM caching we actually get the mode we requested,
> and if so, do we test for this elsewhere? Or are you saying that
> this test doesn't achieve this purpose?

This is not a test for set-caching API, but on whether we do device
accesses without rpm. get-caching doesn't touch the device at all (and
never ever should) so is irrelevant for the test.
> 
> > > 
> > > Signed-off-by: David Weinehall <david.weinehall at linux.intel.com>
> > > ---
> > >  tests/pm_rpm.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> > > index 2aa6c1018aa2..c25252eafad0 100644
> > > --- a/tests/pm_rpm.c
> > > +++ b/tests/pm_rpm.c
> > > @@ -1813,6 +1813,16 @@ static void pm_test_caching(void)
> > >  	gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE);
> > >  
> > >  	for (i = 0; i < ARRAY_SIZE(cache_levels); i++) {
> > > +		/*
> > > +		 * Skip the I915_CACHING_CACHED test
> > > +		 * if we lack an LLC cache
> > > +		 */
> > > +		if (cache_levels[i] == I915_CACHING_CACHED &&
> > > +		    !gem_has_llc(drm_fd)) {
> > > +			igt_debug("!gem_has_llc(); skipping\n");
> > > +			continue;
> > > +		}
> > 
> > No. For the purposes of the test you actually want to call
> > gem_set_caching(fd, handle, NONE).
> 
> Wouldn't that case already be exercised in the first iteration of this
> test?

Not really. The test is that given a vma bound into the ggtt that we
then change the cache level on, do we take the rpm around the gsm
access. To exercise the code we the vma bound into the ggtt, that is
what the *ptr does. Then we need it to change cache level to exercise
how we handle the vma inside set-cache-level. That is the crux of the
test.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre