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

[Caz Yokoyama]

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

> > > Something else that can be employed to speed up this test would
> > > be
> > > igt_drop_caches_set(DROP_IDLE) (before waiting for runtime
> > > suspend).
> > 
> > Yes, it does speed up to 56sec from 88sec. Not only gem-execbuf-
> > stress-
> > extra-wait, but also other 2 tests speed up. For example, gem-
> > execbuf-
> > stress speed up to 9sec from 38sec. Thank you.
> > 
> > How do you want to have patches? Only
> > igt_drop_caches_set(DROP_IDLE)?
> > Both igt_drop_caches_set(DROP_IDLE) and rounds / 2 in a single
> > patch?
> > Separate 2 patches?
> 
> 2 patches. Adding DROP_IDLE is definitely a functional change (that
> should have no impact on behaviour) that wants to be separate. Doing
> exactly the same operations faster is one thing; doing fewer
> operations
> altogether requires some more handwaving as why you think that is
> justified, and why that is important enough to take a risk.
> -Chris