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 Wed, Apr 15, 2026 at 09:51:19AM -0300, Gustavo Sousa wrote:
>> 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.
>
> I already tried this in v1. But Kamil noticed this would apply same
> allowlist to all hooks what likely is not we want. For example if we
> would have two hook scripts like a.sh and b.sh and a.sh would like
> to execute only for igt@test1@basic, and b.sh to execute on
> igt@test2@enhanced we would have problem how to distinguish which
> line is for which test. It would need to extend allowlist to sth
> like:
>
> a.sh:
> igt@test1@basic
>
> b.sh:
> igt@test2@enhanced
>
> and do more complicated filtering in script than simple grep -q -f.

That's not what I meant; I didn't mean a single allowlist to apply
globally.  I meant something along the lines of either:

  igt_runner --hook "post-dyn-subtest:scripts/hooks/copy_guc_log_on_fail.sh --allow-list path/to/allowlist" ...

or

  IGT_HOOKS_GUC_LOG_ALLOWLIST=path/to/allowlist igt_runner --hook "post-dyn-subtest:scripts/hooks/copy_guc_log_on_fail.sh"

>
> At the moment I don't see really good solution, each of them have
> pros and cons. Current copy_guc_log_on_fail.sh script tries to look
> for allowlist first in IGT_HOOK_ALLOWLIST_DIR. CI may not set this
> variable, so script will exit due to lack of allowlist (I've migrated
> copy_guc_log.allowlist to tests/intel-ci, so it doesn't exist in script
> directory anymore and it just quit. So when CI will decide to enable
> hooks integration they just need to configure IGT_HOOK_ALLOWLIST_DIR
> - they know when tests/intel-ci exists so they can set this env.
>
> Kamil noticed as well instead of using multiple hook scripts we could
> use single hook-wrapper.sh, which in turn would contain all hooks
> we want to execute. Like:
>
> hook-wrapper.sh:
> #!/bin/bash
>
> scripts/hooks/copy_guc_log_on_fail.sh
> scripts/hooks/do_another_hook.sh
> scripts/hooks/and_another_one.sh
> ...
>
> Then CI folks would have less work if we would like to enable
> another hook. Just single:
> --hook 'post-subtest:scripts/hooks/hook-wrapper.sh' \
> --hook 'post-dyn-subtest:scripts/hooks/hook-wrapper.sh'
>
> I like his suggestion, we may do more without burdening CI folks
> - they just integrate hooks once. Of course we need to be careful
> with scripting to not break anything but this applies to enabling
> hooks with or without hook-wrapper.sh
>
> What's your opinion about this?

I think a hook wrapper would be fine if that's what is best for CI.  I
just think it should live in some CI-specific folder
(e.g. tests/intel-ci) instead of a general widely available scripts/
dir.  The decision of what hooks to execute is mainly on the user (CI in
this case).

In that case, the path to the allowlist could even be defined in the
hook wrapper, like in the example below.

tests/intel-ci/hook-wrapper.sh:
#!/bin/bash

SCRIPT_DIR=$(dirname "$(realpath "$0")")
IGT_DIR="$SCRIPT_DIR/../.."

guc_log_allowlist="$SCRIPT_DIR/guc_log_allowlist.txt"

$IGT_DIR/scripts/hooks/copy_guc_log_on_fail.sh --allowlist "$guc_log_allowlist"
... # Other hooks

--
Gustavo Sousa
>
> --
> Zbigniew
>
>> 
>> >
>> >> 
>> >> > @@ -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