Re: [PATCH 05/20] drm/i915: Cancel reset-engine if we couldn't find an active request

[Michel Thierry <michel.thierry@intel.com>]

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?

Thanks
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx