[Intel-gfx] [PATCH 15/37] drm/i915: Export i915_request_skip()

[Chris Wilson]

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 at 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