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

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

[David Weinehall]

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.

Signed-off-by: David Weinehall <david.weinehall@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;
+		}
+
 		memset(gem_buf, 16 << i, gtt_obj_max_size);
 
 		disable_all_screens_and_wait(&ms_data);
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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.
> 
> Signed-off-by: David Weinehall <david.weinehall@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).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

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?

> > 
> > Signed-off-by: David Weinehall <david.weinehall@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?


Kind regards, DAvid
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote:
> 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.

Actually no, that's why you have the memset/*ptr - that is what forces
the vma to get bound. And to exercise the set-cache-level, you need it
bound into at a different cache-level, hence the suggestion to call
set-cache-level again - respecting the rules of using the GGTT.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

On Wed, Mar 02, 2016 at 02:04:58PM +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.

Right. How about the disable/enable screens bit? Stay or go?

> > 
> > > > 
> > > > Signed-off-by: David Weinehall <david.weinehall@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.

Thanks for the explanation!


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ke, 2016-03-02 at 14:37 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote:
> > 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.
> 
> Actually no, that's why you have the memset/*ptr - that is what
> forces
> the vma to get bound. And to exercise the set-cache-level, you need
> it
> bound into at a different cache-level, hence the suggestion to call
> set-cache-level again - respecting the rules of using the GGTT.

Yes I see that it gets bound, but is guaranteed that it gets unbound
during the next set-cache-level call? As I understand it will only
happen if i915_gem_valid_gtt_space() returns false.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 02, 2016 at 04:41:54PM +0200, Imre Deak wrote:
> On ke, 2016-03-02 at 14:37 +0000, Chris Wilson wrote:
> > On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote:
> > > 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.
> > 
> > Actually no, that's why you have the memset/*ptr - that is what
> > forces
> > the vma to get bound. And to exercise the set-cache-level, you need
> > it
> > bound into at a different cache-level, hence the suggestion to call
> > set-cache-level again - respecting the rules of using the GGTT.
> 
> Yes I see that it gets bound, but is guaranteed that it gets unbound
> during the next set-cache-level call? As I understand it will only
> happen if i915_gem_valid_gtt_space() returns false.

No. But that is beside the point. It is either unbound or rewritten
during the set-cache-level ioctl, either way we write through the GSM if
it is currently bound. (And it will only be unbound in fairly rare
circumstances.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ke, 2016-03-02 at 14:49 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 04:41:54PM +0200, Imre Deak wrote:
> > On ke, 2016-03-02 at 14:37 +0000, Chris Wilson wrote:
> > > On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote:
> > > > 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.
> > > 
> > > Actually no, that's why you have the memset/*ptr - that is what
> > > forces
> > > the vma to get bound. And to exercise the set-cache-level, you
> > > need
> > > it
> > > bound into at a different cache-level, hence the suggestion to
> > > call
> > > set-cache-level again - respecting the rules of using the GGTT.
> > 
> > Yes I see that it gets bound, but is guaranteed that it gets
> > unbound
> > during the next set-cache-level call? As I understand it will only
> > happen if i915_gem_valid_gtt_space() returns false.
> 
> No. But that is beside the point. It is either unbound or rewritten
> during the set-cache-level ioctl, either way we write through the GSM
> if
> it is currently bound. (And it will only be unbound in fairly rare
> circumstances.)

Ah right, I missed that, thanks for explaining. But we still need to
make sure the device is suspended before the set-cache-level call, that
is an initial disable_all_screens() and then wait_for_suspended()
before calling set-cache-level.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 02, 2016 at 05:01:19PM +0200, Imre Deak wrote:
> Ah right, I missed that, thanks for explaining. But we still need to
> make sure the device is suspended before the set-cache-level call, that
> is an initial disable_all_screens() and then wait_for_suspended()
> before calling set-cache-level.

To me it would be clearer then to split it up into a disable_all_screens()
before the loop and wait_for_suspend() inside before the second
set-cache, and drop the enable_one_screen(). (That way the test is
focusing on the set-cache-level itself and not confusing the
reader/coverage with the display power blocks).

You also want the wait_for_suspended() before the memset, to ensure the
fault handler is also rpm away, but that is checked elsewhere so not
essential.

Hmm, this file has a noticeable lack of GEM domain management. E.g.
gem_mmap_subtest() will only work by happenstance on !llc with
gtt_mmap=false.

To test the shrinker and eviction code, you can use i915_drop_caches.
First populate the GGTT using a GTT mmap, then
igt_drop_caches_set(DROP_BOUND); (Unbind looks best to be exercised
through gem_close, though adding DROP_EVICT_GGTT seems sane).

I don't see how you detect the number of illegal accesses whilst
suspended? Should we not have a counter exposed through debugfs?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

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 and uses I915_CACHING_NONE instead.

v2: Various improvements based on feedback from Chris Wilson

Signed-off-by: David Weinehall@linux.intel.com
---
 tests/pm_rpm.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 2aa6c1018aa2..add8e2eb457f 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1800,7 +1800,7 @@ static void pm_test_caching(void)
 	uint32_t handle;
 	uint8_t *gem_buf;
 
-	uint32_t i, got_caching;
+	uint32_t i;
 	uint32_t gtt_obj_max_size = (16 * 1024);
 	uint32_t cache_levels[3] = {
 		I915_CACHING_NONE,
@@ -1808,42 +1808,31 @@ static void pm_test_caching(void)
 		I915_CACHING_DISPLAY,           /* eDRAM caching */
 	};
 
+	disable_all_screens(&ms_data);
 
 	handle = gem_create(drm_fd, gtt_obj_max_size);
 	gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE);
 
 	for (i = 0; i < ARRAY_SIZE(cache_levels); i++) {
-		memset(gem_buf, 16 << i, gtt_obj_max_size);
+		igt_assert(wait_for_suspended());
 
-		disable_all_screens_and_wait(&ms_data);
+		memset(gem_buf, 16 << i, gtt_obj_max_size);
 
 		igt_debug("Setting cache level %u\n", cache_levels[i]);
 
-		gem_set_caching(drm_fd, handle, cache_levels[i]);
-
-		got_caching = gem_get_caching(drm_fd, handle);
-
-		igt_debug("Got back %u\n", got_caching);
-
-		/*
-		 * Allow fall-back to CACHING_NONE in case the platform does
-		 * not support it.
-		 */
-		if (cache_levels[i] == I915_CACHING_DISPLAY)
-			igt_assert(got_caching == I915_CACHING_NONE ||
-				   got_caching == I915_CACHING_DISPLAY);
-		else
-			igt_assert(got_caching == cache_levels[i]);
-
-		enable_one_screen_and_wait(&ms_data);
+		/* If we lack an LLC cache we use I915_CACHING_NONE instead */
+		if (cache_levels[i] == I915_CACHING_CACHED &&
+		    !gem_has_llc(drm_fd)) {
+			igt_debug("!gem_has_llc(); using I915_CACHING_NONE\n");
+			gem_set_caching(drm_fd, handle, I915_CACHING_NONE);
+		} else
+			gem_set_caching(drm_fd, handle, cache_levels[i]);
 	}
 
 	igt_assert(munmap(gem_buf, gtt_obj_max_size) == 0);
 	gem_close(drm_fd, handle);
 }
 
-
-
 static void fences_subtest(bool dpms)
 {
 	int i;
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

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 and uses I915_CACHING_NONE instead.

v2: Various improvements based on feedback from Chris Wilson

v3: Fix incorrect Signed-off-by: line

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 tests/pm_rpm.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 2aa6c1018aa2..add8e2eb457f 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1800,7 +1800,7 @@ static void pm_test_caching(void)
 	uint32_t handle;
 	uint8_t *gem_buf;
 
-	uint32_t i, got_caching;
+	uint32_t i;
 	uint32_t gtt_obj_max_size = (16 * 1024);
 	uint32_t cache_levels[3] = {
 		I915_CACHING_NONE,
@@ -1808,42 +1808,31 @@ static void pm_test_caching(void)
 		I915_CACHING_DISPLAY,           /* eDRAM caching */
 	};
 
+	disable_all_screens(&ms_data);
 
 	handle = gem_create(drm_fd, gtt_obj_max_size);
 	gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE);
 
 	for (i = 0; i < ARRAY_SIZE(cache_levels); i++) {
-		memset(gem_buf, 16 << i, gtt_obj_max_size);
+		igt_assert(wait_for_suspended());
 
-		disable_all_screens_and_wait(&ms_data);
+		memset(gem_buf, 16 << i, gtt_obj_max_size);
 
 		igt_debug("Setting cache level %u\n", cache_levels[i]);
 
-		gem_set_caching(drm_fd, handle, cache_levels[i]);
-
-		got_caching = gem_get_caching(drm_fd, handle);
-
-		igt_debug("Got back %u\n", got_caching);
-
-		/*
-		 * Allow fall-back to CACHING_NONE in case the platform does
-		 * not support it.
-		 */
-		if (cache_levels[i] == I915_CACHING_DISPLAY)
-			igt_assert(got_caching == I915_CACHING_NONE ||
-				   got_caching == I915_CACHING_DISPLAY);
-		else
-			igt_assert(got_caching == cache_levels[i]);
-
-		enable_one_screen_and_wait(&ms_data);
+		/* If we lack an LLC cache we use I915_CACHING_NONE instead */
+		if (cache_levels[i] == I915_CACHING_CACHED &&
+		    !gem_has_llc(drm_fd)) {
+			igt_debug("!gem_has_llc(); using I915_CACHING_NONE\n");
+			gem_set_caching(drm_fd, handle, I915_CACHING_NONE);
+		} else
+			gem_set_caching(drm_fd, handle, cache_levels[i]);
 	}
 
 	igt_assert(munmap(gem_buf, gtt_obj_max_size) == 0);
 	gem_close(drm_fd, handle);
 }
 
-
-
 static void fences_subtest(bool dpms)
 {
 	int i;
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 02, 2016 at 05:50:28PM +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 and uses I915_CACHING_NONE instead.
> 
> v2: Various improvements based on feedback from Chris Wilson
> 
> Signed-off-by: David Weinehall@linux.intel.com
> ---
>  tests/pm_rpm.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index 2aa6c1018aa2..add8e2eb457f 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -1800,7 +1800,7 @@ static void pm_test_caching(void)
>  	uint32_t handle;
>  	uint8_t *gem_buf;
>  
> -	uint32_t i, got_caching;
> +	uint32_t i;
>  	uint32_t gtt_obj_max_size = (16 * 1024);
>  	uint32_t cache_levels[3] = {
>  		I915_CACHING_NONE,
> @@ -1808,42 +1808,31 @@ static void pm_test_caching(void)
>  		I915_CACHING_DISPLAY,           /* eDRAM caching */
>  	};
>  
> +	disable_all_screens(&ms_data);
>  
>  	handle = gem_create(drm_fd, gtt_obj_max_size);
>  	gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE);
>  
>  	for (i = 0; i < ARRAY_SIZE(cache_levels); i++) {
> -		memset(gem_buf, 16 << i, gtt_obj_max_size);
> +		igt_assert(wait_for_suspended());
>  
> -		disable_all_screens_and_wait(&ms_data);
> +		memset(gem_buf, 16 << i, gtt_obj_max_size);
>  
>  		igt_debug("Setting cache level %u\n", cache_levels[i]);
>  
> -		gem_set_caching(drm_fd, handle, cache_levels[i]);
> -
> -		got_caching = gem_get_caching(drm_fd, handle);
> -
> -		igt_debug("Got back %u\n", got_caching);
> -
> -		/*
> -		 * Allow fall-back to CACHING_NONE in case the platform does
> -		 * not support it.
> -		 */
> -		if (cache_levels[i] == I915_CACHING_DISPLAY)
> -			igt_assert(got_caching == I915_CACHING_NONE ||
> -				   got_caching == I915_CACHING_DISPLAY);
> -		else
> -			igt_assert(got_caching == cache_levels[i]);
> -
> -		enable_one_screen_and_wait(&ms_data);
> +		/* If we lack an LLC cache we use I915_CACHING_NONE instead */
> +		if (cache_levels[i] == I915_CACHING_CACHED &&
> +		    !gem_has_llc(drm_fd)) {
> +			igt_debug("!gem_has_llc(); using I915_CACHING_NONE\n");
> +			gem_set_caching(drm_fd, handle, I915_CACHING_NONE);
> +		} else
> +			gem_set_caching(drm_fd, handle, cache_levels[i]);
>  	}
>  
>  	igt_assert(munmap(gem_buf, gtt_obj_max_size) == 0);
>  	gem_close(drm_fd, handle);
>  }

My argument is that:
unsigned default_level;

disable_all_screens(&ms_data);

handle = gem_create();
default_level = gem_get_caching();
ptr = gem_mmap__gtt()

for_each_level() {
	igt_assert(wait_for_suspended());
	gem_set_caching(default_level);

	/* Ensure we bind the vma into the GGTT */
	memset(ptr, 0, OBJECT_SIZE);

	/* Now try changing the cache-level on the bound object.
	 * This will either unlikely unbind the object from the GGTT,
	 * or more likely just change the PTEs inside the GGTT. Either
	 * way the driver must take the rpm wakelock around the GSM
	 * access.
	 */

	igt_assert(wait_for_suspended());
	gem_set_caching(this_level);

	//igt_assert_eq(intel_detect_and_clear_invalid_rpm_access(), 0);
}

is clearer to follow.

We should also add partial-vma exercising. For that, change OBJECT_SIZE
to be greater than the mappable aperture.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ke, 2016-03-02 at 15:28 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 05:01:19PM +0200, Imre Deak wrote:
> [...]
> Hmm, this file has a noticeable lack of GEM domain management. E.g.
> gem_mmap_subtest() will only work by happenstance on !llc with
> gtt_mmap=false.
> 
> To test the shrinker and eviction code, you can use i915_drop_caches.
> First populate the GGTT using a GTT mmap, then
> igt_drop_caches_set(DROP_BOUND); (Unbind looks best to be exercised
> through gem_close, though adding DROP_EVICT_GGTT seems sane).
> 
> I don't see how you detect the number of illegal accesses whilst
> suspended? Should we not have a counter exposed through debugfs?

Ok, I'll add JIRA task(s) for these;)

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

On machines that lack an LLC the pm-caching subtest will
terminate with sigbus and thus CRASH during the
I915_CACHING_CACHED iteration. To work around this we reset
the caching to I915_CACHING_NONE before doing memory access.

v2: Various improvements based on feedback from Chris Wilson

v3: Fix incorrect Signed-off-by: line

v4: Further improvements based on feedback from Chris Wilson

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 tests/pm_rpm.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 2aa6c1018aa2..c57bf11d4e4e 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1800,7 +1800,8 @@ static void pm_test_caching(void)
 	uint32_t handle;
 	uint8_t *gem_buf;
 
-	uint32_t i, got_caching;
+	uint32_t i;
+	uint32_t default_cache_level;
 	uint32_t gtt_obj_max_size = (16 * 1024);
 	uint32_t cache_levels[3] = {
 		I915_CACHING_NONE,
@@ -1808,42 +1809,34 @@ static void pm_test_caching(void)
 		I915_CACHING_DISPLAY,           /* eDRAM caching */
 	};
 
+	disable_all_screens(&ms_data);
 
 	handle = gem_create(drm_fd, gtt_obj_max_size);
+	default_cache_level = gem_get_caching(drm_fd, handle);
 	gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE);
 
 	for (i = 0; i < ARRAY_SIZE(cache_levels); i++) {
-		memset(gem_buf, 16 << i, gtt_obj_max_size);
+		igt_assert(wait_for_suspended());
+		gem_set_caching(drm_fd, handle, default_cache_level);
 
-		disable_all_screens_and_wait(&ms_data);
+		/* Ensure we bind the vma into the GGTT */
+		memset(gem_buf, 16 << i, gtt_obj_max_size);
 
+		/* Now try changing the cache-level on the bound object.
+		 * This will either unlikely unbind the object from the GGTT,
+		 * or more likely just change the PTEs inside the GGTT. Either
+		 * way the driver must take the rpm wakelock around the GSM
+		 * access.
+		 */
 		igt_debug("Setting cache level %u\n", cache_levels[i]);
-
+		igt_assert(wait_for_suspended());
 		gem_set_caching(drm_fd, handle, cache_levels[i]);
-
-		got_caching = gem_get_caching(drm_fd, handle);
-
-		igt_debug("Got back %u\n", got_caching);
-
-		/*
-		 * Allow fall-back to CACHING_NONE in case the platform does
-		 * not support it.
-		 */
-		if (cache_levels[i] == I915_CACHING_DISPLAY)
-			igt_assert(got_caching == I915_CACHING_NONE ||
-				   got_caching == I915_CACHING_DISPLAY);
-		else
-			igt_assert(got_caching == cache_levels[i]);
-
-		enable_one_screen_and_wait(&ms_data);
 	}
 
 	igt_assert(munmap(gem_buf, gtt_obj_max_size) == 0);
 	gem_close(drm_fd, handle);
 }
 
-
-
 static void fences_subtest(bool dpms)
 {
 	int i;
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 02, 2016 at 07:01:28PM +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. To work around this we reset
> the caching to I915_CACHING_NONE before doing memory access.
> 
> v2: Various improvements based on feedback from Chris Wilson
> 
> v3: Fix incorrect Signed-off-by: line
> 
> v4: Further improvements based on feedback from Chris Wilson
> 
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>

Looks good to me, I'll let Imre double check if I haven't broke
something subtle in the test setup.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ke, 2016-03-02 at 17:20 +0000, Chris Wilson wrote:
> On Wed, Mar 02, 2016 at 07:01:28PM +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. To work around this we reset
> > the caching to I915_CACHING_NONE before doing memory access.
> > 
> > v2: Various improvements based on feedback from Chris Wilson
> > 
> > v3: Fix incorrect Signed-off-by: line
> > 
> > v4: Further improvements based on feedback from Chris Wilson
> > 
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> 
> Looks good to me, I'll let Imre double check if I haven't broke
> something subtle in the test setup.

It looks ok to me, so:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx