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

[Chris Wilson <chris@chris-wilson.co.uk>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx