Hi Chris,
> > > +static int s_many(void *arg)
> > > +{
> > > + struct perf_series *ps = arg;
> >
> > why do we need to go through void... all functions are taking a
> > perf_series structure.
>
> The kthread API defines the function pointer as int (*fn)(void *arg);
In the parallel tes, not in the serial test. right? But, OK,
it's not a big deal.
> > > + p->time = ktime_sub(ktime_get(), p->time);
> > > + if (p->busy) {
> > > + p->busy = ktime_sub(intel_engine_get_busy_time(p->engine),
> > > + p->busy - 1);
> > > + intel_disable_engine_stats(p->engine);
> > > + }
> > > +
> > > + err = switch_to_kernel_sync(ce, err);
> >
> > how about err?
>
> It's a case of where we need to flush all the engines regardless of the
> error, so we wait until the end.
so you put it there to remove also the warning :)
> > > + p->runtime += intel_context_get_total_runtime_ns(ce);
> >
> > assigning a negative value to an unsigned so that you can add it
> > later? looks nice but odd :)
> >
> > It's easier to understand if we do
> >
> > p->runtime = intel_context_get_total_runtime_ns(ce) - p->runtime;
> >
> > if you like it, no need to change, though.
>
> I've done both. I'm fond of the unsigned addition of a negative number.
> It's a few instructions less. :-p
Yes, nevertheless, I like it, even though it's somehow an abuse
of the concept of unsigned.
> > > + count = 0;
> > > + p->time = ktime_get();
> > > + do {
> > > + struct i915_request *rq;
> > > +
> > > + rq = i915_request_create(ce);
> > > + if (IS_ERR(rq)) {
> > > + err = PTR_ERR(rq);
> > > + break;
> > > + }
> > > +
> > > + i915_request_add(rq);
> >
> > do we need a request_put as well?
>
> No. Nasty API, add consumes the reference. It's on the list to update
> now that we have a ~majority of cases that want to keep a reference to
> their request.
arrghh!
> > > + for_each_uabi_engine(engine, i915) {
> > > + int status;
> > > +
> > > + if (IS_ERR(engines[idx].tsk))
> > > + break;
> >
> > if we break here aren't we missing engine_pm_put and put_task?
>
> No. We broke in the earlier loop as well, so the ERR_PTR is the
> sentinel, indicating the end of the assignments.
Oh yes, true!
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxx>
Now finally, after a month it has been lingering around, you can
finally push it.
Thanks,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
