Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare
Date: Wed, 17 Jul 2019 18:29:31 +0100	[thread overview]
Message-ID: <e3ce3844-e38e-2bc9-6dfa-c61e24b2741c@linux.intel.com> (raw)
In-Reply-To: <156337178063.4375.5570535715333917830@skylake-alporthouse-com>

On 17/07/2019 14:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:42:15)
>>
>> On 17/07/2019 14:30, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:21:50)
>>>>
>>>> On 17/07/2019 14:08, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
>>>>>>
>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>> -             if (retry)
>>>>>>> -                     stop_engines(gt, engine_mask);
>>>>>>> -
>>>>>>
>>>>>> Only other functional change I see is that we stop retrying to stop the
>>>>>> engines before reset attempts. I don't know if that is a concern or not.
>>>>>
>>>>> Ah, but we do stop the engine before resets in *reset_prepare. The other
>>>>> path to arrive is in sanitize where we don't know enough state to safely
>>>>> tweak the engines. For those, I claim it shouldn't matter as the engines
>>>>> should be idle and we only need the reset to clear stale context state.
>>>>
>>>> Yes I know that we do call stop in prepare, just not on the reset retry
>>>> path. It's the above loop, if reset was failing and needed retries
>>>> before we would re-retried stopping engines and now we would not.
>>>
>>> The engines are still stopped. The functional change is to remove the
>>> dangerous clearing of RING_HEAD/CTL.
>>
>> Okay for execlists, but for ringbuffer I was simply asking if _one_ of
>> the reasons for failed reset could be failure to stop cs. In which case
>> removal of stop_engines from the retry loop might be detrimental for
>> ringbuffer.
> 
> For ringbuffer, we do the full shebang in prepare_reset, with a nice
> splat if we fail to clear the head. iirc, that has never been an issue,
> although one should always reserve judgment for g4x to randomly fail
> with head updates. If it helps, we can remove the loop here as I don't
> think it accomplishes anything -- the examples I have where it times out
> are followed by a hard machine hang.

No it's fine if you say it was never the issue. I just wanted some 
reassurances on the particular point.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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