Quoting Tvrtko Ursulin (2018-02-05 13:45:16)
>
> On 05/02/2018 13:36, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-02-05 13:23:54)
> >> How would you view taking the request->lock over this block in the
> >> signaler and then being able to call simply
> >> intel_engine_cancel_signaling - ending up with very similar code as in
> >> i915_gem_request_retire?
> >
> > I am not keen on the conflation here (maybe it's just a hatred of bool
> > parameters). But at first glance it's just the commonality of
> > remove_signal, which is already a common routine?
> >
> >> Only difference would be the tasklet kicking, but maybe still it would
> >> be possible to consolidate the two in one helper.
> >>
> >> __i915_gem_request_retire_signaling(req, bool kick_tasklets)
> >> {
> >> if (!DMA_FENCE_FLAG_SIGNALED_BIT) {
> >> dma_fence_signal_locked(...);
> >> if (kick_tasklets) {
> >> local_bh_disable();
> >> local_bh_enable();
> >> }
> >
> > We can't kick the tasklet inside a spinlock. Especially not a lockclass
> > as nested as request.lock :(
>
> Yep bool is ugly. So maybe make the helper return status, so the caller
> can kick if fence was signaled? Or you would worry about losing a little
> bit of latency? That also is not ideal I agree.
>
> Too bad, I would kind of like to consolidate if nothing to be
> symmetrical wrt req->lock usage.
>
> Otherwise patch looks safe to me. At least I failed to find any problems.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Took the r-b and pushed because I wanted to see the back of this oom
fix. The next steps in this series are to try and lighten the
irq/signaler threads, so suggestions are most appreciated.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
