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

[Imre Deak]

On ke, 2016-03-02 at 14:04 +0000, Chris Wilson wrote:
> 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.

The purpose of the enable/disable screen calls was to make sure that
the object gets unbound, otherwise we may not call i915_vma_bind()
which is where the actual HW access happened. But actually it would be
enough to call disable_all_screens_and_wait() once and then call
wait_for_suspended() instead of disable_all_screens_and_wait() in the
loop.

--Imre

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