Hi Krzysztof,
[...]
> Can a request that's not already on the GPU be passed into _unsubmit()
> again? Otherwise setting running_since to 0 is not needed, because it's
> gonna get overwritten in _submit() anyway.
Nice catch.
>
> > if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
> > i915_request_cancel_breadcrumb(request);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.h
> > b/drivers/gpu/drm/i915/i915_request.h
> > index b09135301f39..48b619ca6bf4 100644
> > --- a/drivers/gpu/drm/i915/i915_request.h
> > +++ b/drivers/gpu/drm/i915/i915_request.h
> > @@ -325,6 +325,9 @@ struct i915_request {
> > struct i915_request_watchdog {
> > struct llist_node link;
> > struct hrtimer timer;
> > + ktime_t running_since;
> > + ktime_t preempted_at;
> > + ktime_t total_run_time;
> > } watchdog;
> >
>
> The preempted_at variable is currently not needed, as it's only ever
> written to and never read from in this patch.
>
> The running_since = 0 trick in _unsubmit() used to filter out the
> requests that have been passed to it but haven't actually advanced seems
> a bit hacky at first glance, assuming it's needed. It does make sense
> if you know that running_since > 0 if the request is active and = 0
> otherwise after reading the code a bit, but it's not readily apparent
> to me.
>
> Something like the following would be clearer to me, however please do
> judge yourself and check with others whether it is actually more readable.
>
> You could keep track of the most recent time of being given GPU time
> (running_since) and the most recent preemption (preempted_at). If
> running_since > preempted_at, that means the request has actually
> advanced, since it has been _submit()ed after the most recent _unsubmit().
> Conversely, if preempted_at > running_since, then _unsubmit() has been
> called after the most recent _submit(), so the request couldn't have been
> active.
>
> So something along these lines:
>
> in _submit() (I'm omitting the struct stuff for brevity)
>
> running_since = ktime_get();
>
> in _unsubmit()
>
> /* "if the most recent submit was after the most recent preemption" */
> if (running_since > preempted_at) { /* possibly >= instead? */
> preempted_at = ktime_get();
>
> total_run_time = ktime_add(total_run_time,
> ktime_sub(preempted_at,
> running_since));
> /* the block of time between
> * activation and preemption
> */
> }
>
> This along with renaming the variables to something like (running_at,
> preempted_at) / (running_since, preempted_since) for symmetry.
Huh, I quite like this approach. More stuff appears implicit,
but after giving it more thought, I think it looks cleaner and
equally readable.
I'll experiment with this and see if the results acquired
empirically are the same.
However, this is a major change to the logic of handling
preemption, so I'll wait for some more voices from the
community before making this a fully fledged patch.
Thanks for looking at this!
--
Best Regards,
Krzysztof
