On Sat, Oct 23, 2021 at 07:46:48PM +0200, Thomas Hellström wrote: > > On 10/22/21 20:09, John Harrison wrote: > > And to be clear, the engine reset is not supposed to fail. Whether > > issued by GuC or i915, the GDRST register is supposed to self clear > > according to the bspec. If we are being sent the G2H notification for an > > engine reset failure then the assumption is that the hardware is broken. > > This is not a situation that is ever intended to occur in a production > > system. Therefore, it is not something we should spend huge amounts of > > effort on making a perfect selftest for. > > I don't agree. Selftests are there to verify that assumptions made and > contracts in the code hold and that hardware behaves as intended / assumed. > No selftest should ideally trigger in a production driver / system. That > doesn't mean we can remove all selftests or ignore updating them for altered > assumptions / contracts. I think it's important here to acknowledge the fact > that this and the perf selftest have found two problems that need > consideration for fixing for a production system. > I'm confused - we are going down the rabbit hole here. Back to this patch. This test was written for very specific execlists behavior. It was updated to also support the GuC. In that update we missed fixing the failure path, well because it always passed. Now it has failed, we see that it doesn't fail gracefully, and takes down the machine. This patch fixes that. It also openned my eyes to the horror show reset locking that needs to be fixed long term. > > > > The current theory is that the timeout in GuC is not quite long enough > > for DG1. Given that the bspec does not specify any kind of timeout, it > > is only a best guess anyway! Once that has been tuned correctly, we > > should never hit this case again. Not ever, Not in a selftest, not in an > > end user use case, just not ever. > > ..until we introduce new hardware for which the tuning doesn't hold anymore > or somebody in a two years wants to lower the timeout wondering why it was > set so long? If an engine reset fails in the GuC, the GuC signals the i915 via a G2H that the engine reset has failed and i915 initiates a full GT reset. After this patch (which removes hacky behavior to block foreign, relative to the test, resets) we can see the i915 behaving correctly and the GPU recovering. This path in the code is working as designed. Do you have test for that behavior, no. Can we? No at the moment as we would need a way for the GuC to intentionally fail a engine reset. Right now all we have is either flaky HW or GuC isn't waiting long enough. As far as why the engine reset fails - I am currently working with the GuC team to get a firmware with a configurable timeout period so we can root cause the engine failure. Figures crossed we are just not waiting long enough rather than a HW issue. Regardless of everything above, this patch is correct to unblock CI in the short term and is correct in the long term too as this test shouldn't bring down CI when it fails. Matt > > /Thomas > >
