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

[Peter Senna Tschudin]

Hi Lucas,

Thank you! I somehow missed your message, so V3 went out before I read it. Please see my comments below and let me know how to move forward.

Thank you!

On 06.11.2024 15:13, Lucas De Marchi wrote:

>> diff --git a/lib/igt_facts.c b/lib/igt_facts.c
>> new file mode 100644
>> index 000000000..90b362c5e
>> --- /dev/null
>> +++ b/lib/igt_facts.c
>> @@ -0,0 +1,443 @@
>> +// SPDX-License-Identifier: MIT
>> +
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
> 
> we shouldn't be copying this in new files. Just SPDX + Copyright is
> sufficient.
Fixed in V3.
 
>> + *
>> + */
>> +
>> +#include <stdio.h>
>> +#include <glib.h>
>> +#include <libudev.h>
>> +#include "igt_core.h"
>> +#include "igt_kmod.h"
>> +#include "igt_facts.h"
>> +#include "igt_device_scan.h"
>> +
>> +
>> +/* Report new facts and fact changes
>> + * - new: new_value is NULL
>> + * - change: new_value is not NULL
>> + * - delete: delete is true
>> + */
>> +static void report_fact_change(igt_fact *fact, const char *new_value,
>> +                               const char *last_test, bool delete)
>> +{
>> +    struct timespec uptime_ts;
>> +    char *uptime = NULL;
>> +
>> +    if (last_test == NULL)
>> +        last_test = g_strdup("before any test");
> 
> why are we using glib? I only see functions that could be replaced by
> libc like strdup() and asprintf()

The why is "safety", but I dropped this in V3 as I made a mistake: last_test is static for a good reason.


[...]

> 
>> +
>> +void igt_facts(igt_fact_list *list, const char *last_test)
>> +{
>> +    scan_pci_for_gpus(list, last_test);
> 
> ok, this uses udev to scan for vga/display
> 
>> +    scan_pci_drm_cards(list, last_test);
> 
> this seems rather to be scan_drm_cards, i.e. any device that has a
> loaded driver registered with drm subsystem. This would also show
> vgem, usb, etc

Nah, I ask udev to give me only drm cards that are associated with PCI GPUs. I tested in a place where we have a drm card for a GPU that is a platform device and the code(V2 at least) ignores it. How can I test vgem, usb, etc?

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

So I suggest we start with the hardcoded simple code to assess the potential value of these facts. If the facts turns out to be useful in helping us debug corner cases, then we evaluate how to extend it.

Any other fact you want to add? Asking because I miss this code in our CI to debug two issues that are on my queue...

> 
> This way it's easier to control what and when info is collected,
> allowing developers (and CI) to pick and choose what gets executed.

The original idea was to have static and dynamic facts. The dynamic idea suggested a convenient mechanism for each test to define facts as needed. Zbigniew did not appreciate the potential to bloating.

> 
> +Gustavo
> 
> Lucas De Marchi

* - https://patchwork.freedesktop.org/patch/621849/?series=140566&rev=1