Re: [PATCH i-g-t] igt-runner fact checking

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

Hi Lucas,

Thank you for your reply.

On 07.11.2024 16:55, Lucas De Marchi wrote:
> +Ryszard to know if the CI part would be doable.
> 
> On Thu, Nov 07, 2024 at 08:18:52AM +0100, Peter Senna Tschudin wrote:
>>>> +    scan_kernel_loaded_kmods(list, last_test);
>>>
>>>
>>> So... igt_facts() hardcodes what is the info collected. Maybe we
>>> should rather plug this into the hooks framework as part of
>>> "built-in hooks".
>>
>> I do not follow what you are trying to communicate here. Also, please see what Zbigniew said here*. Facts are not test requirements, but the line is blurry. In the context here a fact is something in the environment that can change but that should not. An example is the disappearing GPU.
> 
> $ ./build/runner/igt_runner --help | grep -A1 hook
>   --hook HOOK_STR
>                         Forward HOOK_STR to the --hook option of each test.
>   --help-hook
>                         Show detailed usage information for --hook.
> 
> So... today we already have "generic support" for running arbitrary
> things before/after each test/subtest/dynamic-subtest.
> 
>     $ cat << EOF > testlist.txt
>     igt@xe_exec_basic@once-basic
>     EOF
>     $ chmod +x lspci.sh
>     $ sudo ./build/runner/igt_runner \
>         --hook 'post-subtest:lspci -vv -d 8086:*:03xx' \
>         --test-list testlist.txt build/tests/ results/
>     $ head -n4 results/0/out.txt
>     03:00.0 VGA compatible controller: Intel Corporation DG2 [Arc A580] (rev 08) (prog-if 00 [VGA controller])
>         Subsystem: Intel Corporation Device 1425
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> 
> So, instead of creating a separate thing called "igt_facts", what I'm
> proposing is that we build this on top of the hooks and these "facts"
> are simply built-in hooks that are commonly used. We may even keep them
> as scripts rather than translate to C.

I fail to understand why one would want to do that. It feels hackish and misses the point. I would support your suggestion if we were talking about a temporary debug tool.

The igt-facts are not meant to be temporary, I dislike the overhead, and I dislike the hackish nature of this mechanism. Can you tell me what are the benefits of using these hooks? This is an honest question, I do not understand.

> 
> What I'd like is something liked this (with whatever name we decide):
> 
>     sudo ./build/runner/igt_runner --builtin-hooks lspci,drm-cards

This is far from what I am proposing in purpose and in architecture.

> 
> which is a shorthand or equivalent to:
> 
>     sudo ./build/runner/igt_runner \
>         --hook post-subtest:$PWD/scripts/built-in-hooks/lspci.sh \
>         --hook post-subtest:$PWD/scripts/built-in-hooks/drm-cards.sh \
>         --test-list testlist.txt build/tests/ results/

For me this feels like a bad idea. It is probably an order of magnitude slower to run, and it is also granular in a way that I fail to understand. igt-facts should not be selected at runtime by the user because we do not know when the facts will change. Again, think of the disappearing GPU: it is a 1 in 1'000'000 kind of event, but is super valuable to know when it happens.

> 
> The cherry on top would be for CI to parse the cover letter and
> enable those ondemand, so we only do that in CI when we want to:
> 
> /ci-built-in-hooks: lspci,drm-cards
> 
> Could also be:
> 
> /ci-hook: 'post-subtest:lspci -vv -d 8086:*:03xx'

Very different from what I am proposing and I fail to understand how this can have potential to add value. Sorry :/

> 
> (note that the current way we have is "Test-with:", but that is
> problematic as it's then parsed as a git-trailer and added to the
> patches by some tools)
> 
> Lucas De Marchi

[...]