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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@chris-wilson.co.uk>
>>>>> Reviewed-by: Antonio Argenziano <antonio.argenziano@intel.com> #v1
>>>>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v1
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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..

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev