Re: [PATCH i-g-t v2 2/2] CONTRIBUTING: Add guide about igt libraries

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 31 Mar 2025 06:34:24 -0700, Kamil Konieczny wrote:
>
> Add some general guide about adding new library function and
> a few guides for their usage outside of tests.
>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Acked-by: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
> ---
>  CONTRIBUTING.md | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> index 8370451d6..d4efa61f5 100644
> --- a/CONTRIBUTING.md
> +++ b/CONTRIBUTING.md
> @@ -38,6 +38,34 @@ The Code
>
>  [igt-describe]: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe
>
> +IGT libraries
> +-------------
> +- Tests and benchmarks are the main usage of IGT libraries, so they
> +  could use test specific macros/functions, for example igt_assert,
> +  igt_require, igt_skip, igt_info or igt_debug.
> +
> +- New library function could be written when it will have at least two
> +  different users, for example if it could be used by two or more tests.
> +  In some cases single user can be accepted, when it is very likely it
> +  will be used in future.
> +
> +- In a new library function():
> +  if it uses some of the macros igt_assert/igt_require/igt_skip then
> +  consider to write also __function() with the same functionality but
> +  without them.
> +
> +- Libraries and igt_runner
> +  Runner should not use lib functions. It is crucial for CI runs so using
> +  libraries puts a risk of bringing changes meant for tests which in turn
> +  could break runner.
> +  Note: You will find places where igt_runner uses lib functions - this will
> +  be on ToDo list to be fixed.
> +
> +- Libraries and tools/
> +  Tools should try to not use lib functions. Any abnormal condition should
> +  be simply reported by printf or fprintf to stdout/stderr and then tool
> +  should exit gracefully. Do not use igt_abort nor igt_assert, igt_print,
> +  igt_debug nor other testing/printing macros from igt lib/

Not sure about runner, but for tools, I'd rather have shared functions from
lib/ rather than duplicating code. So we should be more specific here about
which functions tools should not use. Maybe tools should not use 'igt_abort
nor igt_assert, igt_print, igt_debug' etc. or even using these might be ok,
as long as the tool works as expected?

Or maybe just say here that 'give some thought if you are planning to use
IGT lib code in tools, some IGT lib functions might be be appropriate in
tools'.

For example previously a lot of extra code was written in i915 perf
library, just to avoid using IGT lib/. I changed this approach in xe perf
lib and decided to use IGT lib/.



>
>  Sending Patches
>  ---------------
> --
> 2.49.0
>