[igt-dev] [PATCH i-g-t 1/1] i915_pm_rpm: gem-execbuf-stress-extra-wait faster

[Chris Wilson]

Quoting Caz Yokoyama (2019-03-07 18:21:03)
> On Tue, 2019-03-05 at 23:29 +0000, Chris Wilson wrote:
> > Quoting Caz Yokoyama (2019-03-05 23:22:06)
> > > On Tue, 2019-03-05 at 17:22 +0000, Chris Wilson wrote:
> > > > Quoting Caz Yokoyama (2019-03-06 00:06:55)
> > > > > Less iterate because WAIT_EXTRA adds extra 5 sec delay for each
> > > > > iteration.
> > > > > This test is similar to gem-execbuf-stress except for
> > > > > WAIT_EXTRA.
> > > > > It runs 86 sec. while gem-execbuf-stress runs 38 sec.
> > > > > i.e. 38s + iteration(rounds,10) * 5s = 88s
> > > > 
> > > > Why does it wait for 5s? There's no second stage to runtime
> > > > suspend,
> > > > but
> > > > if there was, add a debug interface to force it.
> > > 
> > > I believe no because teardown_environment() is the onlye statement
> > > after igt_subtest("gem-execbuf-stress-extra-wait").
> > 
> > I mean in the kernel. I can't think what the extra 5s does, my guess
> > would be that it was to try and induce some HW change. (But by the
> > time
> > runtime suspend completes, the HW *should* be as powered down as we
> > can
> > achieve.)
> > 
> > Something that may help a bit is by adding something like
> > pm_runtime_force_suspend() to a debug path, like DROP_SUSPEND.
> Are you advicing to replace WAIT_EXTRA by DROP_SUSPEND? By DROP_IDLE,
> queueued requests are dropped, therefore, wait_for_suspended() becomes
> shorter. By DROP_SUSPEND, gpu goes to suspended state and no need to
> call wait_for_suspended(). The purpose of the test is "let's see what
> happens when we force many suspend/resume cycles". So it is OK to force
> to suspend. Do I understand your comment correctly?

Yes. The sleep is more or less speculation that the HW does it's own
multilevel powergating behind the scenes, and while bogosities such as
the dmc exist, I don't think that there is any magic in the runtime
suspend paths. Hence sleeping past completion of runtime suspend has
little value and can be safely replaced by an explicit forced suspend
(with the knowledge that if HW does get more complicated in future, we
can put the extra debug logic there). The only case where sleeping would
be useful is to confirm the automatic suspend timers work, but that is a
simple testcase of the timer function, orthogonal to the various "check
that we suspend and wakeup correctly around these tasks" we are doing
here.
-Chris