Re: [igt-dev] [PATCH v19 6/6] test: perf_pmu: use the gem_engine_topology library

From: Andi Shyti <andi.shyti@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: IGT dev <igt-dev@lists.freedesktop.org>, Andi Shyti <andi@etezian.org>
Subject: Re: [igt-dev] [PATCH v19 6/6] test: perf_pmu: use the gem_engine_topology library
Date: Mon, 8 Apr 2019 19:56:51 +0300	[thread overview]
Message-ID: <20190408165651.GC2556@intel.intel> (raw)
In-Reply-To: <a3dafb83-05d0-e089-a508-20c1b6708f77@linux.intel.com>

Hi Tvrtko,

> > Replace the legacy for_each_engine* defines with the ones
> > implemented in the gem_engine_topology library.
> > 
> > Use whenever possible gem_engine_can_store_dword() that checks
> > class instead of flags.
> > 
> > Now the __for_each_engine_class_instance and
> > for_each_engine_class_instance are unused, remove them.
> 
> In the future please start adding per patch changelogs and in the cover
> letter just list the main changes.

I personally never liked versioning in the commit log because
it's a reference external to the current commit history.

But if this is the igt culture, I'll comply. Next time I will add
the versioning in the commit log.

> > -init(int gem_fd, const struct intel_execution_engine2 *e, uint8_t sample)
> > +init(int gem_fd, struct intel_execution_engine2 *e, uint8_t sample)
> 
> Why is const a problem here? The function only reads from the object?

It's not a problem, but I think it's not necessary. init() is not
called anymore with a 'const' variable type.

> >   	unsigned long slept;
> >   	uint64_t val, val2, ts[2];
> > @@ -347,6 +357,7 @@ busy_double_start(int gem_fd, const struct intel_execution_engine2 *e)
> >   	int fd;
> >   	ctx = gem_context_create(gem_fd);
> > +	intel_init_engine_list(gem_fd, ctx);
> 
> You didn't find this a bit ugly? I was thinking that we should maybe have a
> helper to configure the context which doesn't return the engine data struct.
> Doesn't matter I guess.

well yes, it's not very nice :)

"do it once, do it right!" I'll add a helper that just binds the
engines to the given context (without creating all the stuff we
have in init_engine_list()).

> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks a lot!
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev