Hi Andi,
Thanks for review.
On Monday, 16 December 2024 14:26:58 CET Andi Shyti wrote:
> Hi Janusz,
>
> ...
>
> > for_each_gt(gt, i915, i) {
> > + struct intel_engine_cs *engine;
> > + unsigned long timeout_ms = 0;
> > + unsigned int id;
> > +
> > if (intel_gt_is_wedged(gt))
> > ret = -EIO;
> >
> > + for_each_engine(engine, gt, id) {
> > + if (engine->props.preempt_timeout_ms >
timeout_ms)
> > + timeout_ms = engine-
>props.preempt_timeout_ms;
> > + }
>
>
> the brackets are not really required here.
OK, I was not sure if for_each_if used inside for_each_engine is supposed to
resolve potential issues with potentially confusing if nesting, but from your
comment I conclude it does. I'll fix it.
>
> > +
> > cond_resched();
> >
> > - if (intel_gt_wait_for_idle(gt, HZ * 3) == -ETIME) {
> > + if (intel_gt_wait_for_idle(gt, HZ * timeout_ms / 500) == -
ETIME) {
>
> where is this 500 coming from?
/ 1000 would convert it to seconds as needed, and / 500 used instead was
supposed to to mean that we are willing to wait for preempt_timeout_ms * 2.
Sorry for that shortcut. Would you like me to provide a clarifying comment,
or maybe better use explicit 2 * preempt_timeout / 1000 ?
Thanks,
Janusz
>
> Thanks,
> Andi
>
> > pr_err("%pS timed out, cancelling all further
testing.\n",
> > __builtin_return_address(0));
> >
>
