Re: [Intel-gfx] [PATCH 08/10] drm/i915/selftests: Measure set-priority duration

[Andi Shyti <andi@etezian.org>]

On Wed, Jan 20, 2021 at 12:22:03PM +0000, Chris Wilson wrote:
> As a topological sort, we expect it to run in linear graph time,
> O(V+E). In removing the recursion, it is no longer a DFS but rather a
> BFS, and performs as O(VE). Let's demonstrate how bad this is with a few
> examples, and build a few test cases to verify a potential fix.

very noble purpose, but...

[...]

> +static int sparse(struct drm_i915_private *i915,
> +		  bool (*fn)(struct i915_request *rq,
> +			     unsigned long v, unsigned long e))
> +{
> +	return chains(i915, sparse_chain, fn);
> +}

... this is quite an intricate web of functions calling each
other.

Is there any simplier way to do this? This is that kind of code
that if you go on holiday for a few days you forget what you
wrote.

I do need three drawing boards and 24 fingers for keeping track
of what's happening here. :)

> +
> +static void report(const char *what, unsigned long v, unsigned long e, u64 dt)
> +{
> +	pr_info("(%4lu, %7lu), %s:%10lluns\n", v, e, what, dt);
> +}
> +
> +static u64 __set_priority(struct i915_request *rq, int prio)
> +{
> +	u64 dt;
> +
> +	preempt_disable();
> +	dt = ktime_get_raw_fast_ns();
> +	i915_request_set_priority(rq, prio);
> +	dt = ktime_get_raw_fast_ns() - dt;
> +	preempt_enable();
> +
> +	return dt;
> +}
> +
> +static bool set_priority(struct i915_request *rq,
> +			 unsigned long v, unsigned long e)
> +{
> +	report("set-priority", v, e, __set_priority(rq, I915_PRIORITY_BARRIER));

can't we pr_info directly here and spare a function?

> +	return true;
> +}
> +
> +static int single_priority(void *arg)
> +{
> +	return single(arg, set_priority);
> +}
> +
> +static int wide_priority(void *arg)
> +{
> +	return wide(arg, set_priority);
> +}
> +
> +static int inv_priority(void *arg)
> +{
> +	return inv(arg, set_priority);
> +}
> +
> +static int sparse_priority(void *arg)
> +{
> +	return sparse(arg, set_priority);
> +}
> +
> +int i915_scheduler_perf_selftests(struct drm_i915_private *i915)
> +{
> +	static const struct i915_subtest tests[] = {
> +		SUBTEST(single_priority),
> +		SUBTEST(wide_priority),
> +		SUBTEST(inv_priority),
> +		SUBTEST(sparse_priority),
> +	};
> +	static const struct {
> +		const char *name;
> +		size_t sz;
> +	} types[] = {
> +#define T(t) { #t, sizeof(struct t) }
> +		T(i915_priolist),
> +		T(i915_sched_attr),
> +		T(i915_sched_node),
> +#undef T

is this really making the code better? Is it a big deal to
clearly use

		{ i915_priolist, sizeof(i915_priolist) },
		{ i915_sched_attr, sizeof(i915_sched_attr) },
		{ i915_sched_node, sizeof(i915_sched_node) },

> +		{}
> +	};
> +	typeof(*types) *t;
> +
> +	for (t = types; t->name; t++)
> +		pr_info("sizeof(%s): %zd\n", t->name, t->sz);
> +
> +	return i915_subtests(tests, i915);
> +}
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx