Quoting Andi Shyti (2021-01-26 18:05:38)
> 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.
This was to remove duplication, and there's more tests to come in this
group that use the same framework and only differ in the final step.
> 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?
It will be reused later.
> > + 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) },
Duplication and more typing, you even left out the struct in
sizeof(struct T) :)
Did this save me more time to add stuff than it took to write #define T?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
