[PATCH i-g-t 1/2] tests/intel/xe_drm_fdinfo: Group utilization tests and skip when no utilization data

[Kamil Konieczny]

Hi Bernatowicz,,
On 2025-03-05 at 17:31:43 +0100, Bernatowicz, Marcin wrote:
> 
> 
> On 3/5/2025 1:15 PM, Kamil Konieczny wrote:
> > Hi Marcin,
> > On 2025-03-04 at 19:56:39 +0100, Marcin Bernatowicz wrote:
> > > Wrap all utilization-related subtests in an igt_subtest_group and add
> > > a fixture that ensures utilization data is available before running
> > > them.
> > > 
> > > Link: https://lore.kernel.org/r/20250205191644.2550879-1-marcin.bernatowicz@linux.intel.com
> > > 
> > > Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz at linux.intel.com>
> > > Cc: Jakub Kolakowski <jakub1.kolakowski at intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Cc: Lukasz Laguna <lukasz.laguna at intel.com>
> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > > ---
> > >   tests/intel/xe_drm_fdinfo.c | 122 ++++++++++++++++++++----------------
> > >   1 file changed, 67 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/tests/intel/xe_drm_fdinfo.c b/tests/intel/xe_drm_fdinfo.c
> > > index 7330b4330..d18fe10d8 100644
> > > --- a/tests/intel/xe_drm_fdinfo.c
> > > +++ b/tests/intel/xe_drm_fdinfo.c
> > > @@ -735,10 +735,6 @@ igt_main
> > >   	igt_subtest("basic-mem")
> > >   		basic_memory(xe);
> > > -	igt_describe("Check if basic fdinfo content is present for engine utilization");
> > > -	igt_subtest("basic-utilization")
> > > -		basic_engine_utilization(xe);
> > > -
> > >   	igt_describe("Create and compare total and resident memory consumption by client");
> > >   	igt_subtest("mem-total-resident")
> > >   		mem_total_resident(xe);
> > > @@ -751,57 +747,73 @@ igt_main
> > [...]
> > 
> > > +	igt_subtest_group {
> > > +		igt_fixture {
> > > +			struct drm_client_fdinfo info = { };
> > > +
> > > +			igt_require(igt_parse_drm_fdinfo(xe, &info, engine_map,
> > > +							 ARRAY_SIZE(engine_map),
> > > +							 NULL, 0));
> > 
> > Problem with require placed here is that it will trigger also
> > for all other test outside of group. That will clobber output.
> 
> Does it mean the igt_subtest_group is deprecated and the description
> no more valid ?
> 
> "
> /**
>  * igt_subtest_group:
> 
> ...
> 
> * This macro allows to group together a set of #igt_fixture and #igt_subtest
>  * clauses. If any common setup in a fixture fails, only the subtests in
> this
>  * group will fail or skip. Subtest groups can be arbitrarily nested.
> "
> 

Well, you could group it to show logical part, use fixtures in them
for opening/closing files, just do not use igt_require as it should
be used only in first global fixture in igt_main, not in following
ones.

> > 
> > > +			igt_require(info.num_engines);
> > 
> > Both these should be placed in each subtest wrapped in function
> > (see below) but imho this looks like a global requires at begin
> > of a test? Or am I missing something?
> > 
> > > +		}
> > > +
> > 
> > Create function like:
> > 
> > void require_utilization(int xe, ... engine_map)
> > {
> > 	struct drm_client_fdinfo info = { };
> > 
> > 	igt_require(igt_parse_drm_fdinfo(xe, &info, engine_map,
> > 					 ARRAY_SIZE(engine_map),
> > 					 NULL, 0));
> > 	igt_require(info.num_engines);
> > }
> > 
> > and use it in each utulization subtest like:
> > 
> > 	igt_describe("Check if basic fdinfo content is present for engine utilization");
> > 	igt_subtest("basic-utilization") {
> > 		require_utilization(xe);
> > 		basic_engine_utilization(xe);
> > 	}
> 
> That is the repetition I wanted to avoid ;)

I agree it looks like repetition but it is better to have it.
With require in fixture you could ancounter failure logs with
unrelated message like 'require X fails'.

Other solution would be to create a speparate test for these
tests.

Regards,
Kamil

> 
> ---
> marcin
> 
> > 
> > Regards,
> > Kamil
> > 
> > > +		igt_describe("Check if basic fdinfo content is present for engine utilization");
> > > +		igt_subtest("basic-utilization")
> > > +			basic_engine_utilization(xe);
> > > +
> > 
> > [...]
> > 
>