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

[Tvrtko Ursulin]

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?

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).

So a info/warn/skip when using the feature on a platform where it 
doesn't work well is I think much friendlier approach.

If the comment is not relevant, or needs to be adjusted to reflect the 
changed reality (bugfix you mention), then either removed or adjusted.

Regards,

Tvrtko