Re: [PATCH i-g-t v2 07/10] tests/intel/xe_drm_fdinfo: Add an iterator for virtual engines

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jul 11, 2024 at 05:30:14PM -0500, Lucas De Marchi wrote:
> On Thu, Jul 11, 2024 at 11:31:37AM GMT, Umesh Nerlige Ramappa wrote:
> > On Thu, Jul 11, 2024 at 08:34:03AM -0500, Lucas De Marchi wrote:
> > > On Tue, Jul 02, 2024 at 05:25:29PM GMT, Umesh Nerlige Ramappa wrote:
> > > > Add a helper iterator for virtual engines.
> > > > 
> > > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > > > ---
> > > > tests/intel/xe_drm_fdinfo.c | 29 +++++++++++++++++++++++++++++
> > > > 1 file changed, 29 insertions(+)
> > > > 
> > > > diff --git a/tests/intel/xe_drm_fdinfo.c b/tests/intel/xe_drm_fdinfo.c
> > > > index f2051c422..9d3d1b285 100644
> > > > --- a/tests/intel/xe_drm_fdinfo.c
> > > > +++ b/tests/intel/xe_drm_fdinfo.c
> > > > @@ -90,6 +90,34 @@ static const uint64_t batch_addr[] = {
> > > > 	0x1d0000,
> > > > 	0x1e0000,
> > > > };
> > > > +
> > > > +#define MAX_GTS 2
> > > > +#define MAX_INSTANCE 9
> > > > +struct virtual_hwe {
> > > > +	struct drm_xe_engine_class_instance eci[MAX_INSTANCE];
> > > > +	int count;
> > > > +} vhwe[MAX_GTS][DRM_XE_ENGINE_CLASS_COMPUTE + 1] = {};
> > > 
> > > there's no concept of "virtual engine". I think this is something
> > > brought over from i915? Is it related to the width when submitting?
> > > 
> > > Please add a comment for the confused reader and it's probably worth
> > > this abstraction to be outside this test. +Matt Brost
> > 
> > width is for parallel submission and num_placements is for virtual.
> 
> ok... let's also s/virtual engines/virtual exec_queues/  where
> appropriate. Like we have in xe_exec_balancer.c
> 
> can we also use the helper in xe_exec_balancer?

I'm with Lucas that this looks overly complicated.

Perhaps this code from xe_exec_balancer.c can be moved to helper
somewhere?

194         xe_for_each_engine(fd, hwe) {
195                 if (hwe->engine_class != class || hwe->gt_id != gt)
196                         continue;
197
198                 eci[num_placements++] = *hwe;
199         }
200         if (num_placements < 2)
201                 return;

This code collects the hwe instances for a class / gt + calculates the
number of placements. Once you have that calling __xe_exec_queue_create
is pretty simple.

So something like this:

int
xe_exec_queue_virtaul_setup(int fd, struct drm_xe_engine_class_instance *eci,
			    int class, int gt)
{
	struct drm_xe_engine_class_instance *hwe;
	int num_placements = 0;

        xe_for_each_engine(fd, hwe) {
                 if (hwe->engine_class != class || hwe->gt_id != gt)
                         continue;

                 eci[num_placements++] = *hwe;
        }

	return num_placements;
}

Then your test function is something like:

int foo_test(int fd, int class, int gt)
{
	struct drm_xe_engine_class_instance eci[MAX_INSTANCE];
	int num_placement;

	num_placement = xe_exec_queue_virtaul_setup(fd, eci, class, gt);
	if (num_placement < 2)
		return;

	/* Do test */
}

Then your main IGT loop is:

	igt_subtest("foo_test")
		xe_for_each_gt(fd, gt)
			xe_for_each_engine_class(class)
				foo_test(fd, class, gt);

This is the pattern in xe_exec_balancer.c. I don't think we should
reinvent this (it is helpful if IGTs all look fairly similar so if you
unstand one, you probably understand most).

Matt

> 
> 
> Lucas De Marchi
> 
> > 
> > > 
> > > > +
> > > > +static void list_virtual_engines(int fd)
> > > 
> > > list? shouldn't it be virtual_engines_init()?
> > 
> > Right, I will change that.
> > 
> > Thanks,
> > Umesh
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > > +{
> > > > +	struct drm_xe_engine_class_instance *hwe;
> > > > +
> > > > +	xe_for_each_engine(fd, hwe) {
> > > > +		struct virtual_hwe *v;
> > > > +
> > > > +		igt_assert(hwe->gt_id < MAX_GTS);
> > > > +		igt_assert(hwe->engine_class < DRM_XE_ENGINE_CLASS_COMPUTE + 1);
> > > > +		v = &vhwe[hwe->gt_id][hwe->engine_class];
> > > > +
> > > > +		igt_assert(v->count < MAX_INSTANCE);
> > > > +		v->eci[v->count++] = *hwe;
> > > > +	}
> > > > +}
> > > > +#define xe_for_each_multi_engine(__fd, __hwe, __count) \
> > > > +	for (int igt_unique(gt) = 0; igt_unique(gt) < MAX_GTS; igt_unique(gt)++) \
> > > > +		for (int igt_unique(c) = 0; igt_unique(c) < DRM_XE_ENGINE_CLASS_COMPUTE + 1; igt_unique(c)++) \
> > > > +			for_if((__hwe = &vhwe[igt_unique(gt)][igt_unique(c)].eci[0]) && ((__count = vhwe[igt_unique(gt)][igt_unique(c)].count) > 1))
> > > > +
> > > > static void read_engine_cycles(int xe, struct pceu_cycles *pceu)
> > > > {
> > > > 	struct drm_client_fdinfo info = { };
> > > > @@ -678,6 +706,7 @@ igt_main
> > > > 		xe = drm_open_driver(DRIVER_XE);
> > > > 		igt_require_xe(xe);
> > > > 		igt_require(igt_parse_drm_fdinfo(xe, &info, NULL, 0, NULL, 0));
> > > > +		list_virtual_engines(xe);
> > > > 	}
> > > > 
> > > > 	igt_describe("Check if basic fdinfo content is present for memory");
> > > > -- 
> > > > 2.38.1
> > > >