On May 19, 2021 12:16:15 Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
On Wed, May 19, 2021 at 5:06 PM Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote:Once we no longer rely on error propagation, I think there's a lot wecan rip out.I honestly did not find that much ... what did you uncover?
When I was digging through this earlier today, I think I convinced myself that the cmdparser is the only user of the entire fence error architecture. I may have missed something, though.
--Jason
-Daniel--JasonOn Wed, May 19, 2021 at 5:15 AM Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:From: Jason Ekstrand <jason@xxxxxxxxxxxxxx>This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7. Eversince that commit, we've been having issues where a hang in one clientcan propagate to another. In particular, a hang in an app can propagateto the X server which causes the whole desktop to lock up.Error propagation along fences sound like a good idea, but as your bugshows, surprising consequences, since propagating errors across securityboundaries is not a good thing.What we do have is track the hangs on the ctx, and report information touserspace using RESET_STATS. That's how arb_robustness works. Also, if myunderstanding is still correct, the EIO from execbuf is when your contextis banned (because not recoverable or too many hangs). And in all thesecases it's up to userspace to figure out what is all impacted and shouldbe reported to the application, that's not on the kernel to guess andautomatically propagate.What's more, we're also building more features on top of ctx errorreporting with RESET_STATS ioctl: Encrypted buffers use the same, and theuserspace fence wait also relies on that mechanism. So it is the pathgoing forward for reporting gpu hangs and resets to userspace.So all together that's why I think we should just bury this idea again asnot quite the direction we want to go to, hence why I think the revert isthe right option here.Signed-off-by: Jason Ekstrand <jason.ekstrand@xxxxxxxxx>v2: Augment commit message. Also restore Jason's sob that Iaccidentally lost.Signed-off-by: Jason Ekstrand <jason.ekstrand@xxxxxxxxx> (v1)Reported-by: Marcin Slusarz <marcin.slusarz@xxxxxxxxx>Cc: <stable@xxxxxxxxxxxxxxx> # v5.6+Cc: Jason Ekstrand <jason.ekstrand@xxxxxxxxx>Cc: Marcin Slusarz <marcin.slusarz@xxxxxxxxx>Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>---drivers/gpu/drm/i915/i915_request.c | 8 ++------1 file changed, 2 insertions(+), 6 deletions(-)diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.cindex 970d8f4986bb..b796197c0772 100644--- a/drivers/gpu/drm/i915/i915_request.c+++ b/drivers/gpu/drm/i915/i915_request.c@@ -1426,10 +1426,8 @@ i915_request_await_execution(struct i915_request *rq,do {fence = *child++;- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {- i915_sw_fence_set_error_once(&rq->submit, fence->error);+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))continue;- }if (fence->context == rq->fence.context)continue;@@ -1527,10 +1525,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)do {fence = *child++;- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {- i915_sw_fence_set_error_once(&rq->submit, fence->error);+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))continue;- }/** Requests on the same timeline are explicitly ordered, along--2.31.0--Daniel VetterSoftware Engineer, Intel Corporationhttp://blog.ffwll.ch
