| Message ID | 20180629075348.27358-15-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/06/2018 08:53, Chris Wilson wrote: > In the next patch, we will want to start skipping requests on failing to > complete their payloads. So export the utility function current used to > make requests inoperable following a failed gpu reset. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 25 +++---------------------- > drivers/gpu/drm/i915/i915_request.c | 21 +++++++++++++++++++++ > drivers/gpu/drm/i915/i915_request.h | 2 ++ > 3 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c13d5b78a02e..8ae293c75ae9 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3084,25 +3084,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv) > return err; > } > > -static void skip_request(struct i915_request *request) > -{ > - void *vaddr = request->ring->vaddr; > - u32 head; > - > - /* As this request likely depends on state from the lost > - * context, clear out all the user operations leaving the > - * breadcrumb at the end (so we get the fence notifications). > - */ > - head = request->head; > - if (request->postfix < head) { > - memset(vaddr + head, 0, request->ring->size - head); > - head = 0; > - } > - memset(vaddr + head, 0, request->postfix - head); > - > - dma_fence_set_error(&request->fence, -EIO); > -} > - > static void engine_skip_context(struct i915_request *request) > { > struct intel_engine_cs *engine = request->engine; > @@ -3117,10 +3098,10 @@ static void engine_skip_context(struct i915_request *request) > > list_for_each_entry_continue(request, &engine->timeline.requests, link) > if (request->gem_context == hung_ctx) > - skip_request(request); > + i915_request_skip(request, -EIO); > > list_for_each_entry(request, &timeline->requests, link) > - skip_request(request); > + i915_request_skip(request, -EIO); > > spin_unlock(&timeline->lock); > spin_unlock_irqrestore(&engine->timeline.lock, flags); > @@ -3163,7 +3144,7 @@ i915_gem_reset_request(struct intel_engine_cs *engine, > > if (stalled) { > i915_gem_context_mark_guilty(request->gem_context); > - skip_request(request); > + i915_request_skip(request, -EIO); > > /* If this context is now banned, skip all pending requests. */ > if (i915_gem_context_is_banned(request->gem_context)) > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index d618e7127e88..51f28786b0e4 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -962,6 +962,27 @@ i915_request_await_object(struct i915_request *to, > return ret; > } > > +void i915_request_skip(struct i915_request *rq, int error) > +{ > + void *vaddr = rq->ring->vaddr; > + u32 head; > + > + GEM_BUG_ON(!IS_ERR_VALUE((long)error)); Why not simply error >= 0 ? Error is an integer in this function, and in dma_fence_set_error. In fact, looking at dma_fence_set_error it is already checking for valid errno. > + dma_fence_set_error(&rq->fence, error); Was at the end before code movement. > + > + /* > + * As this request likely depends on state from the lost > + * context, clear out all the user operations leaving the > + * breadcrumb at the end (so we get the fence notifications). > + */ > + head = rq->infix; Original assignment is head = rq->head. Looks to be the same but why change it in code movement patch? > + if (rq->postfix < head) { > + memset(vaddr + head, 0, rq->ring->size - head); > + head = 0; > + } > + memset(vaddr + head, 0, rq->postfix - head); > +} > + > /* > * NB: This function is not allowed to fail. Doing so would mean the the > * request is not being tracked for completion but the work itself is > diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h > index 7ee220ded9c9..a355a081485f 100644 > --- a/drivers/gpu/drm/i915/i915_request.h > +++ b/drivers/gpu/drm/i915/i915_request.h > @@ -258,6 +258,8 @@ void i915_request_add(struct i915_request *rq); > void __i915_request_submit(struct i915_request *request); > void i915_request_submit(struct i915_request *request); > > +void i915_request_skip(struct i915_request *request, int error); > + > void __i915_request_unsubmit(struct i915_request *request); > void i915_request_unsubmit(struct i915_request *request); > > Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-06-29 13:10:32) > > On 29/06/2018 08:53, Chris Wilson wrote: > > In the next patch, we will want to start skipping requests on failing to > > complete their payloads. So export the utility function current used to > > make requests inoperable following a failed gpu reset. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 25 +++---------------------- > > drivers/gpu/drm/i915/i915_request.c | 21 +++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_request.h | 2 ++ > > 3 files changed, 26 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index c13d5b78a02e..8ae293c75ae9 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3084,25 +3084,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv) > > return err; > > } > > > > -static void skip_request(struct i915_request *request) > > -{ > > - void *vaddr = request->ring->vaddr; > > - u32 head; > > - > > - /* As this request likely depends on state from the lost > > - * context, clear out all the user operations leaving the > > - * breadcrumb at the end (so we get the fence notifications). > > - */ > > - head = request->head; > > - if (request->postfix < head) { > > - memset(vaddr + head, 0, request->ring->size - head); > > - head = 0; > > - } > > - memset(vaddr + head, 0, request->postfix - head); > > - > > - dma_fence_set_error(&request->fence, -EIO); > > -} > > - > > static void engine_skip_context(struct i915_request *request) > > { > > struct intel_engine_cs *engine = request->engine; > > @@ -3117,10 +3098,10 @@ static void engine_skip_context(struct i915_request *request) > > > > list_for_each_entry_continue(request, &engine->timeline.requests, link) > > if (request->gem_context == hung_ctx) > > - skip_request(request); > > + i915_request_skip(request, -EIO); > > > > list_for_each_entry(request, &timeline->requests, link) > > - skip_request(request); > > + i915_request_skip(request, -EIO); > > > > spin_unlock(&timeline->lock); > > spin_unlock_irqrestore(&engine->timeline.lock, flags); > > @@ -3163,7 +3144,7 @@ i915_gem_reset_request(struct intel_engine_cs *engine, > > > > if (stalled) { > > i915_gem_context_mark_guilty(request->gem_context); > > - skip_request(request); > > + i915_request_skip(request, -EIO); > > > > /* If this context is now banned, skip all pending requests. */ > > if (i915_gem_context_is_banned(request->gem_context)) > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > > index d618e7127e88..51f28786b0e4 100644 > > --- a/drivers/gpu/drm/i915/i915_request.c > > +++ b/drivers/gpu/drm/i915/i915_request.c > > @@ -962,6 +962,27 @@ i915_request_await_object(struct i915_request *to, > > return ret; > > } > > > > +void i915_request_skip(struct i915_request *rq, int error) > > +{ > > + void *vaddr = rq->ring->vaddr; > > + u32 head; > > + > > + GEM_BUG_ON(!IS_ERR_VALUE((long)error)); > > Why not simply error >= 0 ? Because < -ERR_MAX would also be invalid as it gets reported to userspace. > Error is an integer in this function, and in > dma_fence_set_error. In fact, looking at dma_fence_set_error it is > already checking for valid errno. > > > + dma_fence_set_error(&rq->fence, error); > > Was at the end before code movement. Pulled it up to document the range of valid error. > > + > > + /* > > + * As this request likely depends on state from the lost > > + * context, clear out all the user operations leaving the > > + * breadcrumb at the end (so we get the fence notifications). > > + */ > > + head = rq->infix; > > Original assignment is head = rq->head. Looks to be the same but why > change it in code movement patch? For correctness in the callers of i915_request_skip. The point was to export a function that could be used rather than have to duplicate all but a line. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c13d5b78a02e..8ae293c75ae9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3084,25 +3084,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv) return err; } -static void skip_request(struct i915_request *request) -{ - void *vaddr = request->ring->vaddr; - u32 head; - - /* As this request likely depends on state from the lost - * context, clear out all the user operations leaving the - * breadcrumb at the end (so we get the fence notifications). - */ - head = request->head; - if (request->postfix < head) { - memset(vaddr + head, 0, request->ring->size - head); - head = 0; - } - memset(vaddr + head, 0, request->postfix - head); - - dma_fence_set_error(&request->fence, -EIO); -} - static void engine_skip_context(struct i915_request *request) { struct intel_engine_cs *engine = request->engine; @@ -3117,10 +3098,10 @@ static void engine_skip_context(struct i915_request *request) list_for_each_entry_continue(request, &engine->timeline.requests, link) if (request->gem_context == hung_ctx) - skip_request(request); + i915_request_skip(request, -EIO); list_for_each_entry(request, &timeline->requests, link) - skip_request(request); + i915_request_skip(request, -EIO); spin_unlock(&timeline->lock); spin_unlock_irqrestore(&engine->timeline.lock, flags); @@ -3163,7 +3144,7 @@ i915_gem_reset_request(struct intel_engine_cs *engine, if (stalled) { i915_gem_context_mark_guilty(request->gem_context); - skip_request(request); + i915_request_skip(request, -EIO); /* If this context is now banned, skip all pending requests. */ if (i915_gem_context_is_banned(request->gem_context)) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index d618e7127e88..51f28786b0e4 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -962,6 +962,27 @@ i915_request_await_object(struct i915_request *to, return ret; } +void i915_request_skip(struct i915_request *rq, int error) +{ + void *vaddr = rq->ring->vaddr; + u32 head; + + GEM_BUG_ON(!IS_ERR_VALUE((long)error)); + dma_fence_set_error(&rq->fence, error); + + /* + * As this request likely depends on state from the lost + * context, clear out all the user operations leaving the + * breadcrumb at the end (so we get the fence notifications). + */ + head = rq->infix; + if (rq->postfix < head) { + memset(vaddr + head, 0, rq->ring->size - head); + head = 0; + } + memset(vaddr + head, 0, rq->postfix - head); +} + /* * NB: This function is not allowed to fail. Doing so would mean the the * request is not being tracked for completion but the work itself is diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 7ee220ded9c9..a355a081485f 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -258,6 +258,8 @@ void i915_request_add(struct i915_request *rq); void __i915_request_submit(struct i915_request *request); void i915_request_submit(struct i915_request *request); +void i915_request_skip(struct i915_request *request, int error); + void __i915_request_unsubmit(struct i915_request *request); void i915_request_unsubmit(struct i915_request *request);
In the next patch, we will want to start skipping requests on failing to complete their payloads. So export the utility function current used to make requests inoperable following a failed gpu reset. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 25 +++---------------------- drivers/gpu/drm/i915/i915_request.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/i915_request.h | 2 ++ 3 files changed, 26 insertions(+), 22 deletions(-)