On Tue, May 16, 2017 at 05:13:58PM -0700, Michel Thierry wrote:
> On 16/05/17 00:54, Chris Wilson wrote:
> >On Mon, May 15, 2017 at 03:25:27PM -0700, Michel Thierry wrote:
> >>On 5/15/2017 2:47 PM, Chris Wilson wrote:
> >>>On Mon, May 15, 2017 at 10:31:58PM +0100, Chris Wilson wrote:
> >>>>On Mon, May 15, 2017 at 02:20:01PM -0700, Michel Thierry wrote:
> >>>>>@@ -2827,21 +2830,34 @@ int i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
> >>>>>
> >>>>> if (engine_stalled(engine)) {
> >>>>> request = i915_gem_find_active_request(engine);
> >>>>>- if (request && request->fence.error == -EIO)
> >>>>>- err = -EIO; /* Previous reset failed! */
> >>>>>+
> >>>>>+ if (request) {
> >>>>>+ if (request->fence.error == -EIO)
> >>>>>+ return ERR_PTR(-EIO); /* Previous reset failed! */
> >>>>>+
> >>>>>+ if (__i915_gem_request_completed(request,
> >>>>>+ engine->hangcheck.seqno))
> >>>>
> >>>>This is not the seqno for the request, so this is incorrect. It will
> >>>>judge that the request was preempted (as hangcheck.seqno must be less
> >>>>thn request->global_seqno) and so conclude that the request was never
> >>>>completed.
> >>>>
> >>>>You just want if (i915_gem_request_completed(request))
> >>
> >>Thanks, I'll change the function.
> >>
> >>>
> >>>Also not here. This pardon check should be deferred to the caller just
> >>>before commiting to thre reset. In the case of global reset, we want to
> >>>gather up all the engines' active requests first, complete our
> >>>preparations and then double check the engine was hung.
> >>
> >>i915_reset_engine calls this directly, but 'full reset' [from
> >>i915_gem_reset_prepare()] would not be affected and it won't pardon
> >>anything... i915_gem_reset_engine is doing the double check you
> >>mention.
> >
> >Aye, but in the long run I was thinking of capturing this request in
> >engine->hangcheck.active_request and then we reuse that info in the later
> >phases.
>
> Capture hangcheck.active_request during hangcheck_declare_hang? Or
> still here in reset_prepare?
Not in the hangcheck worker itself since we want that to be as lockless
as we can make it and since putting it inside the reset works just as
well, we should.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
