Re: [PATCH i-g-t v3 3/5] scripts/hooks: Example guc log copy script and allowlist

[Gustavo Sousa <gustavo.sousa@intel.com>]

Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:

> On Tue, Apr 14, 2026 at 05:33:08PM -0300, Gustavo Sousa wrote:
>> Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:
>> 
>> > We need to explicitly extract guc logs for failed tests so add hook
>> > script which does it after subtest/dynsubtest execution. As copying to
>> > attachments takes a lot of time do it only for failing subtests.
>> >
>> > Adding this hook script to CI will extend execution time much and
>> > using allowlist is necessary. Instead of allowlist argument in the
>> > runner (previous attempt) now script filters such list on its own and
>> > decides when to move forward and do some post-test actions. Such
>> > approach is more flexible but moves decision to script what causes
>> > it is more prone buggy.
>> >
>> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> > Cc: Ryszard Knop <ryszard.knop@intel.com>
>> > Cc: Krzysztof Karas <krzysztof.karas@intel.com>
>> > ---
>> > v2: install hook to datadir/hooks
>> > v3: migrate allowlist from runner to script
>> > ---
>> >  scripts/hooks/guc_copy.allowlist  |  2 ++
>> >  scripts/hooks/guc_copy_on_fail.sh | 47 +++++++++++++++++++++++++++++++
>> >  scripts/meson.build               |  3 ++
>> >  3 files changed, 52 insertions(+)
>> >  create mode 100644 scripts/hooks/guc_copy.allowlist
>> >  create mode 100755 scripts/hooks/guc_copy_on_fail.sh
>> >
>> > diff --git a/scripts/hooks/guc_copy.allowlist b/scripts/hooks/guc_copy.allowlist
>> > new file mode 100644
>> > index 0000000000..04cb3f8298
>> > --- /dev/null
>> > +++ b/scripts/hooks/guc_copy.allowlist
>> 
>> Do we want really want the allowlist to be part distributed with IGT? I
>> think we should default it to no allowlist and then CI (or whoever is
>> using the hook) defines it if that's desired.
>
> I think we want this allowlist in igt. If there's a test which requires
> to copy guc logs due to failure developer who will analyze this can
> change allowlist without requesting that from CI folks. We already have
> blocklist management in igt source and moving this allowlist management
> to CI would split this resposibility. I would keep such management in
> one place - is igt source code good place for this - I'm not sure, but
> at the moment it's there.

Understood.  Yeah, I guess it is fine to keep CI-managed allowlist in
IGT as well, then.

Since we have CI blocklists in the tests/intel-ci folder, maybe that's
where this hook's allowlist should live as well?  Maybe in the future we
could move intel-ci to belong to the root of the project, since it would
contain more than testlists.

Now, I don't think executables generated by IGT apply the blocklists
automatically, but CI passes the blocklists as argument as part of their
workflow, right?

If that's the case, I think it would make sense for the hook to do the
same.  In other words, my suggestion is that we can have a CI-managed
allowlist file, but not use it by default -- CI would pass it when using
the hook.

>
>> 
>> > @@ -0,0 +1,2 @@
>> > +igt@xe_compute@compute-square
>> > +igt@xe_exec_basic@once-basic@ccs0
>> > diff --git a/scripts/hooks/guc_copy_on_fail.sh b/scripts/hooks/guc_copy_on_fail.sh
>> > new file mode 100755
>> > index 0000000000..595cf5205e
>> > --- /dev/null
>> > +++ b/scripts/hooks/guc_copy_on_fail.sh
>> 
>> Maybe guc_log_on_fail.sh? I think it is more descriptive than
>> guc_copy_*, since the latter is a bit vague about what is being copied.
>
> I think copy_guc_log_on_fail.sh would describe its intent better.

Ack.

>
>> 
>> > @@ -0,0 +1,47 @@
>> > +#!/bin/bash
>> > +
>> > +# Hook script for copying guc.log.
>> > +# Suggested usage with:
>> > +# --hook 'post-subtest:scripts/hooks/guc_copy_on_fail.sh' --hook 'post-dyn-subtest:scripts/hooks/guc_copy_on_fail.sh'
>> > +
>> > +# Copy only for failed subtests as this is time-consuming
>> > +if [ "${IGT_HOOK_RESULT}" != "FAIL" ]; then
>> > +	exit 0
>> > +fi
>> > +
>> > +# Process and copy guc logs only for those specified in allowlist
>> > +ALLOWLIST="guc_copy.allowlist"
>> > +scriptdir=$(dirname $(realpath $0))
>> 
>> It is safer to use "$0" above.
>
> Fix me if I'm wrong but $0 won't contain absolute path if script
> was executed using relative path like "./scripts/xyz.sh"

I mean to use "$0" (rather than $0) as argument of realpath.  Actually,
I think the safest would be:

  scriptdir=$(dirname "$(realpath "$0")")

>
>> 
>> > +
>> > +if [ -z "$IGT_RUNNER_ATTACHMENTS_DIR" ]; then
>> > +	echo "Missing IGT_RUNNER_ATTACHMENTS_DIR env"
>> > +	exit 0
>> > +fi
>> 
>> Well, we could allow the hook user to define the output and default it
>> to "$IGT_RUNNER_ATTACHMENTS_DIR/guc_log" if not passed and
>> $IGT_RUNNER_ATTACHMENTS_DIR is defined.  Otherwise, we should error out
>> IMO (i.e. exit 1).
>
> Ok, in case of missing above enviroment exit with -1 makes sense.
>
>> 
>> As an example, someone could be calling a single test binary directly
>> and want to use this hook.
>
> If standalone test wants to execute hook script which was designed to
> use within runner it should be executed with proper envs set apriori
> its execution.
>
> I would like to minimize number of envs passed to scripts from the
> runner - IGT_RUNNER_ATTACHMENTS_DIR should be single mandatory
> which would point out the script where to write. And single optional
> mentioned below - IGT_HOOK_ALLOWLIST_DIR in which script could look
> for allowlist. I didn't set it in the runner because I don't know
> the allowlist path (this will be know after installing and as DESTDIR=
> might be used we can't say from the runner perspective where it will
> be). Assuming script and allowlist will land in same directory (as
> currently is set in meson) finding it (dirname/realpath) is possible
> within the script regardless it was installed.
>
>> 
>> We could do some simple argument parsing here or, to make it simpler,
>> establish that IGT_HOOKS_GUC_LOG_DEST would be such an input variable
>> (and document it in the doc comment at the start of the script).
>
> Multiplying environment is imo bad idea.
>

I think the hook should be a standalone tool that could be used for
either single tests or in the igt_runner environment.  I think we
should make it easier for both cases, meaning to have a simple way of
the hook user to define the necesary parameters (destination dir and
allowlist file) and make it define sensible defaults when it detects
it is running in a igt_runner environment.

If you prefer that the hook should be specific for the igt_runner
environment, then I guess it is fine to avoid the extra work I'm
suggesting.  We could revisit this in the future if we find users trying
to use it with standalone tests.

>> 
>> > +
>> > +cd "$IGT_RUNNER_ATTACHMENTS_DIR"
>> > +
>> > +# Look for allowlist in following places:
>> > +# 1. Try in IGT_HOOK_ALLOWLIST_DIR if this environment exists
>> > +# 2. Try in <scriptdir>
>> > +
>> > +if [ ! -z "$IGT_HOOK_ALLOWLIST_DIR" ]; then
>> > +	ALLOWLIST_PATH="${IGT_HOOK_ALLOWLIST_DIR}/${ALLOWLIST}"
>> > +else
>> > +	ALLOWLIST_PATH="${scriptdir}/${ALLOWLIST}"
>> > +fi
>> 
>> I would just receive the allowlist path from CLI args or, to make it
>> simpler, from an environment like IGT_HOOKS_GUC_LOG_ALLOWLIST; and I
>> think we should default to not using any allowlist if none was passed.
>
> That's good idea - if script would receive optional arg which is
> filename it may use it. I'll add this to copy guc log script.
>
>> 
>> > +
>> > +if [ ! -e "${ALLOWLIST_PATH}" ]; then
>> > +	exit 0
>> > +fi
>> > +
>> > +STNAME="${IGT_HOOK_TEST_FULLNAME}"
>> > +echo "${STNAME}" | grep -q -f "${ALLOWLIST_PATH}"
>> > +if [ $? -ne 0 ]; then
>> > +	exit 0
>> > +fi
>> 
>> This can simplified with:
>> 
>>      if ! echo "$IGT_HOOK_TEST_FULLNAME" | grep -q -f "$ALLOWLIST_PATH"; then
>>         exit 0;
>>      fi
>> 
>> > +
>> > +for log in $(find /sys/kernel/debug/dri -iname 'guc_log'); do
>> 
>> Suggestion: use $(cd /sys/kernel/debug/dri && find -name guc_log)...
>> 
>> > +	attout=$(echo ${log:23} | sed -e 's/\//_/g')
>> 
>> ...and then here we don't need this line...
>> 
>> > +	mkdir -p "${STNAME}"
>> > +	cp "$log" "${STNAME}/${attout}"
>> 
>> ...and here we can simply do: cp "$log" "$STNAME/${log////_}".
>
> Will apply above in v4.
>
>> 
>> One issue with the find command above is that we could potentially
>> collect GuC log for other device (not the one under test).  I know that
>> IGT supports selecting a device via the environment variable
>> IGT_DEVICE.  I wonder if we should make it set an environment for the
>> device under test; we could use a different variable name to avoid
>> issues with the existing one.
>
> Good point. Question is - how to pass more than one PCIID in multigpu
> scenarios? For single card it is simple, we can set IGT_HOOK_LAST_PCIID=
> for each opened device. I'll take a look to this and share my thoughts.

Ah, interesting.  I wasn't aware of this possibility; so we do have
tests that work on multiple devices?

Yeah, maybe a variable that uses some sort of separator.  I guess this
needs a separate discussion and we can keep the hook looking for
guc_log files for any available device for now?  We could add a comment
noting this behavior.

--
Gustavo Sousa

>
> Thanks for the review.
> --
> Zbigniew
>
>> 
>> --
>> Gustavo Sousa
>> 
>> > +done
>> > diff --git a/scripts/meson.build b/scripts/meson.build
>> > index 6e64065c5e..2ce961898d 100644
>> > --- a/scripts/meson.build
>> > +++ b/scripts/meson.build
>> > @@ -15,3 +15,6 @@ endif
>> >  igt_doc_script = find_program('igt_doc.py', required : build_testplan)
>> >  gen_rst_index = find_program('gen_rst_index', required : build_sphinx)
>> >  generate_iga64_codes = find_program('generate_iga64_codes')
>> > +
>> > +install_data('hooks/guc_copy_on_fail.sh', install_dir : datadir / 'hooks')
>> > +install_data('hooks/guc_copy.allowlist', install_dir : datadir / 'hooks')
>> > -- 
>> > 2.43.0