[igt-dev] [Intel-gfx] [PATCH i-g-t 05/17] lib: Spin fast, retire early

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-07-05 16:29:32)
> 
> On 05/07/2018 13:42, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-07-05 13:33:55)
> >>
> >> On 05/07/2018 12:23, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-07-02 16:36:28)
> >>>>
> >>>> On 02/07/2018 10:07, Chris Wilson wrote:
> >>>>> When using the pollable spinner, we often want to use it as a means of
> >>>>> ensuring the task is running on the GPU before switching to something
> >>>>> else. In which case we don't want to add extra delay inside the spinner,
> >>>>> but the current 1000 NOPs add on order of 5us, which is often larger
> >>>>> than the target latency.
> >>>>>
> >>>>> v2: Don't change perf_pmu as that is sensitive to the extra CPU latency
> >>>>> from a tight GPU spinner.
> >>>>>
> >>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>>> Reviewed-by: Antonio Argenziano <antonio.argenziano at intel.com> #v1
> >>>>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com> #v1
> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>> ---
> >>>>>     lib/igt_dummyload.c       | 3 ++-
> >>>>>     lib/igt_dummyload.h       | 1 +
> >>>>>     tests/gem_ctx_isolation.c | 1 +
> >>>>>     tests/gem_eio.c           | 1 +
> >>>>>     tests/gem_exec_latency.c  | 4 ++--
> >>>>>     5 files changed, 7 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> >>>>> index 94efdf745..7beb66244 100644
> >>>>> --- a/lib/igt_dummyload.c
> >>>>> +++ b/lib/igt_dummyload.c
> >>>>> @@ -199,7 +199,8 @@ emit_recursive_batch(igt_spin_t *spin,
> >>>>>          * between function calls, that appears enough to keep SNB out of
> >>>>>          * trouble. See https://bugs.freedesktop.org/show_bug.cgi?id=102262
> >>>>>          */
> >>>>> -     batch += 1000;
> >>>>> +     if (!(opts->flags & IGT_SPIN_FAST))
> >>>>> +             batch += 1000;
> >>>>
> >>>> igt_require(!snb) or something, given the comment whose last two lines
> >>>> can be seen in the diff above?
> >>>
> >>> It's not a machine killer since we have the required fix in the kernel,
> >>> it just has interesting system latency. That latency is not specific to
> >>> snb, and whether we want to trade system latency vs gpu latency is the
> >>> reason we punt the decision to the caller.
> >>
> >> I am not sure if the comment and linked BZ is then still relevant?
> > 
> > It is, it's just too specific, and it particularly affects modesetting as
> > it doesn't defend itself against CPU starvation / timer latency (imo).
> >   
> >> If it is, it is not nice to punt the responsibility to the caller. We
> >> are exporting new API and it is hard to think callers will dig into the
> >> implementation to find it (this comment).
> > 
> > Why not? Is not the sordid history of our hw supposed to be our
> > specialist topic? :) For what has once happened before is sure to happen
> > again. (Corollary to George Santayana.)
> >   
> >> So a info/warn/skip when using the feature on a platform where it
> >> doesn't work well is I think much friendlier approach.
> > 
> > But for those who want to use it, it is just noise.
> 
> For current users yes, for new users maybe not - maybe they need to have 
> a way to notice they might be doing things unknowingly suboptimally. 
> What would accomplish that goal? igt_info too much noise? igt_debug too 
> low key? Blah..

Rather, as I see it unless it _impacts_ their test, ignorance is bliss.

Having opted into using SPIN_FAST  and their test failing on the farm
on some machines, I don't expect it to be hard for a novice to debug.
-Chris