Re: [PATCH i-g-t 06/18] tests/intel/xe_oa: Add for_each_oa_unit

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

On Thu, 09 Oct 2025 09:40:29 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Wed, Oct 08, 2025 at 04:31:23PM -0700, Umesh Nerlige Ramappa wrote:
> > On Wed, Oct 08, 2025 at 02:17:53PM -0700, Ashutosh Dixit wrote:
> >> Add a for_each_oa_unit iterator, which eliminates code duplication when
> >> iterating over OA units obtained from OA unit query.
> >>
> >> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >> ---
> >> tests/intel/xe_oa.c | 21 +++++++++++----------
> >> 1 file changed, 11 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> >> index 012f5929ce..bc71bfcd30 100644
> >> --- a/tests/intel/xe_oa.c
> >> +++ b/tests/intel/xe_oa.c
> >> @@ -477,18 +477,21 @@ static struct drm_xe_engine_class_instance *oa_unit_engine(struct drm_xe_oa_unit
> >>	return !oau ? NULL : oau->num_engines ? &oau->eci[random() % oau->num_engines] : NULL;
> >> }
> >>
> >> +#define for_each_oa_unit(qoa, oau, poau, i) \
> >> +	for (i = 0, qoa = xe_oa_units(drm_fd), poau = (u8 *)&qoa->oa_units[0]; \
> >> +	     oau = (struct drm_xe_oa_unit *)poau, i < qoa->num_oa_units; \
> >> +	     poau += sizeof(*oau) + oau->num_engines * sizeof(oau->eci[0]), i++)
> >> +
> >> static struct drm_xe_oa_unit *oa_unit_by_id(int fd, int n)
> >> {
> >> -	struct drm_xe_query_oa_units *qoa = xe_oa_units(fd);
> >> +	struct drm_xe_query_oa_units *qoa;
> >>	struct drm_xe_oa_unit *oau;
> >>	u8 *poau;
> >> +	int i;
> >>
> >> -	poau = (u8 *)&qoa->oa_units[0];
> >> -	for (int i = 0; i < qoa->num_oa_units; i++) {
> >> -		oau = (struct drm_xe_oa_unit *)poau;
> >> +	for_each_oa_unit(qoa, oau, poau, i) {
> >>		if (oau->oa_unit_id == n)
> >>			return oau;
> >> -		poau += sizeof(*oau) + oau->num_engines * sizeof(oau->eci[0]);
> >>	}
> >>
> >>	return NULL;
> >> @@ -496,16 +499,14 @@ static struct drm_xe_oa_unit *oa_unit_by_id(int fd, int n)
> >>
> >> static struct drm_xe_oa_unit *oa_unit_by_type(int fd, int t)
> >> {
> >> -	struct drm_xe_query_oa_units *qoa = xe_oa_units(fd);
> >> +	struct drm_xe_query_oa_units *qoa;
> >>	struct drm_xe_oa_unit *oau;
> >>	u8 *poau;
> >> +	int i;
> >>
> >> -	poau = (u8 *)&qoa->oa_units[0];
> >> -	for (int i = 0; i < qoa->num_oa_units; i++) {
> >> -		oau = (struct drm_xe_oa_unit *)poau;
> >> +	for_each_oa_unit(qoa, oau, poau, i) {
> >>		if (oau->oa_unit_type == t)
> >>			return oau;
> >> -		poau += sizeof(*oau) + oau->num_engines * sizeof(oau->eci[0]);
> >>	}
> >>
> >>	return NULL;
> >
> > If it's possible to keep the interface for iterators simple, I would go
> > for that. i, poau can be hidden from the caller. Something like this:
> >
> > // NOTE: not compiled/tested
> >
> > static u32 xe_oa_first_unit(int fd, struct drm_xe_oa_unit **oau)
> > {
> >        struct drm_xe_query_oa_units *qoa = xe_oa_units(fd);
> >        *oau = &qoa->oa_units[0];
> >
> >        return qoa->num_oa_units;
> > }
> >
> > static void xe_oa_next_unit(struct drm_xe_oa_unit **oau)
> > {
> >	struct drm_xe_oa_unit *poau = *oau;
> >
> >        return (u8 *)poau + sizeof(*poau) + poau->num_engines * sizeof(poau->eci[0]);
>
> Not return, but update *oau.
>
> *oau = (u8 *)poau + sizeof(*poau) + poau->num_engines * sizeof(poau->eci[0]);

In v2, I have implemented something not identical, but very close to
this. But thanks for this suggestion, it has resulted in some cleaned up
code.

Ashutosh



>
> > }
> >
> > #define for_each_oa_unit(fd, oau) \
> >        for (u32 i = 0, num = xe_oa_first_unit(fd, &oau); \
> >             oau && i < num; \
> >             i++, xe_oa_next_unit(&oau))
> >
> > static const struct drm_xe_oa_unit *oa_unit_by_id(int fd, int n)
> > {
> >        struct drm_xe_oa_unit *oau;
> >
> >        for_each_oa_unit(fd, oau) {
> >                if (oau->oa_unit_id == n)
> >                        return oau;
> >        }
> >
> >        return NULL;
> > }
> >
> > static const struct drm_xe_oa_unit *oa_unit_by_type(int fd, int t)
> > {
> >        struct drm_xe_oa_unit *oau;
> >
> >        for_each_oa_unit(fd, oau) {
> >                if (oau->oa_unit_type == t)
> >                        return oau;
> >        }
> >
> >        return NULL;
> > }
> >
> > Thanks,
> > Umesh
> >
> >
> >
> >> --
> >> 2.48.1
> >>