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