[igt-dev] [PATCH i-g-t 1/6] test/perf: Drop caches when closing perf stream

[Lionel Landwerlin]

On 05/03/2020 01:04, Umesh Nerlige Ramappa wrote:
> On Thu, Mar 05, 2020 at 12:05:55AM +0200, Lionel Landwerlin wrote:
>> On 04/03/2020 19:51, Umesh Nerlige Ramappa wrote:
>>> On Wed, Mar 04, 2020 at 10:45:55AM +0200, Lionel Landwerlin wrote:
>>>> On 04/03/2020 00:57, Umesh Nerlige Ramappa wrote:
>>>>> On Tue, Mar 03, 2020 at 02:38:08PM -0800, Umesh Nerlige Ramappa 
>>>>> wrote:
>>>>>> Running ./build/tests/perf will run all the perf subtests in a 
>>>>>> sequence.
>>>>>> When running tests in a sequence, subsequent tests may not run 
>>>>>> with a
>>>>>> clean slate. For resources that are lazily released, drop caches in
>>>>>> __perf_close.
>>>>>
>>>>> Hi Lionel, Chris,
>>>>>
>>>>> I notice an issue on TGL when running the entire suite of perf 
>>>>> tests.  In my setup, the polling test was failing with invalid 
>>>>> reports being seen in the beginning of the OA buffer. This issue 
>>>>> is seen more prominently with the newly added subtests which call 
>>>>> perf_open and perf_close a couple of times (say 
>>>>> blocking-with-interrupt).
>>>>>
>>>>> What I see in some runs is that the second test would result in a 
>>>>> bunch of unlanded reports in the beginning of the OA buffer. 
>>>>> Assuming that we are already waiting for the NOA config with a 
>>>>> noa_wait bo, I tried to look into this further.
>>>>>
>>>>> free_oa_buffer is called to free the oa_buffer bo and this work is 
>>>>> deferred by the driver. If a test is called before this free 
>>>>> completes, we see the issue. Just to test out this theory, if I 
>>>>> comment out the free_oa_buffer entirely, I see that the tests pass 
>>>>> without any issues since new gtt memory is being allocated each time.
>>>>>
>>>>> I guess the deferred free and the new allocation of the OA buffer 
>>>>> for subsequent test has something missing. Maybe TLBs not being 
>>>>> dropped? I imagine the OA unit might write valid reports somewhere 
>>>>> based on what it sees in the TLBs and cpu is looking for them 
>>>>> elsewhere (until the free completes). Just a theory though. Let me 
>>>>> know what you think.
>>>>>
>>>>> For now igt_drop_caches_set(DROP_FREED) is what is helping and 
>>>>> hence this patch.
>>>>
>>>>
>>>> Hey Umesh,
>>>>
>>>>
>>>> I guess this could be fixed by this commit :
>>>>
>>>>
>>>> commit 4b4e973d5eb89244b67d3223b60f752d0479f253
>>>> Author: Chris Wilson <chris at chris-wilson.co.uk>
>>>> Date:   Mon Mar 2 08:57:57 2020 +0000
>>>>
>>>>     drm/i915/perf: Reintroduce wait on OA configuration completion
>>>>
>>>> If you can give this commit a try or rebase on drm-tip it would be 
>>>> great to confirm.
>>>
>>> I thought this commit was ensuring that the noa_wait is executed 
>>> completely before we enable the OA buffer captures. That still does 
>>> not explain why the issue goes away for me when I comment out 
>>> free_oa_buffer.
>>
>>
>> If noa_wait is not waited upon, either from CPU or GPU, then we 
>> enable OA while the configuration is not completely applied.
>>
>> Hence the invalid data at the beginning of the buffer.
>>
>>
>> Are you saying this commit didn't help?
>>
> No, I haven't tried it yet. I lost my reservation on the TGL machine 
> :(, so waiting for another one.
>
> What I meant is that - not freeing the OA buffer (only for 
> experimenting) results in a new gtt offset everytime we allocate the 
> OA buffer. When I do this, I don't see any invalid OA reports. This is 
> without the commit you mention above. If waiting for the NOA config to 
> complete were indeed the issue, I should have seen it even with my 
> experiment. Right?


I see, thanks that's a useful experiment.

Only thing I can think of would be HEAD/TAIL register writes that didn't 
land before the OA unit was turned on.

I'll dig into the code.


Is this only on Gen12?


Thanks,


-Lionel


>
> Thanks,
> Umesh
>
>>
>> Thanks,
>>
>>
>> -Lionel
>>
>>
>>>
>>> Thanks,
>>> Umesh
>>>
>>>>
>>>> Otherwise we might need more digging to figure what's going on.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>>
>>>> -Lionel
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Umesh
>>>>>
>>>>>>
>>>>>> Signed-off-by: Umesh Nerlige Ramappa 
>>>>>> <umesh.nerlige.ramappa at intel.com>
>>>>>> ---
>>>>>> tests/perf.c | 7 ++++++-
>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tests/perf.c b/tests/perf.c
>>>>>> index 5e818030..189c6aa1 100644
>>>>>> --- a/tests/perf.c
>>>>>> +++ b/tests/perf.c
>>>>>> @@ -244,6 +244,12 @@ __perf_close(int fd)
>>>>>>         close(pm_fd);
>>>>>>         pm_fd = -1;
>>>>>>     }
>>>>>> +
>>>>>> +    /* When running tests in a sequence, subsequent tests may 
>>>>>> not run with a
>>>>>> +     * clean slate. For resources that are lazily released, 
>>>>>> cleanup here.
>>>>>> +     */
>>>>>> +    if (drm_fd >= 0 && !getgid() && !getuid())
>>>>>> +        gem_quiescent_gpu(drm_fd);
>>>>>> }
>>>>>>
>>>>>> static int
>>>>>> @@ -3993,7 +3999,6 @@ test_rc6_disable(void)
>>>>>>     igt_assert_eq(n_events_end - n_events_start, 0);
>>>>>>
>>>>>>     __perf_close(stream_fd);
>>>>>> -    gem_quiescent_gpu(drm_fd);
>>>>>>
>>>>>>     n_events_start = rc6_residency_ms();
>>>>>>     nanosleep(&(struct timespec){ .tv_sec = 1, .tv_nsec = 0 }, 
>>>>>> NULL);
>>>>>> -- 
>>>>>> 2.20.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> igt-dev mailing list
>>>>>> igt-dev at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>>>>
>>>>
>>