Re: [PATCH 03/55] drm/i915: i915_add_request must not fail

[Tomas Elf <tomas.elf@intel.com>]

On 04/06/2015 15:07, John Harrison wrote:
> On 02/06/2015 19:16, Tomas Elf wrote:
>> On 29/05/2015 17:43, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> The i915_add_request() function is called to keep track of work that
>>> has been
>>> written to the ring buffer. It adds epilogue commands to track
>>> progress (seqno
>>> updates and such), moves the request structure onto the right list
>>> and other
>>> such house keeping tasks. However, the work itself has already been
>>> written to
>>> the ring and will get executed whether or not the add request call
>>> succeeds. So
>>> no matter what goes wrong, there isn't a whole lot of point in
>>> failing the call.
>>>
>>> At the moment, this is fine(ish). If the add request does bail early
>>> on and not
>>> do the housekeeping, the request will still float around in the
>>> ring->outstanding_lazy_request field and be picked up next time. It
>>> means
>>> multiple pieces of work will be tagged as the same request and driver
>>> can't
>>> actually wait for the first piece of work until something else has been
>>> submitted. But it all sort of hangs together.
>>>
>>> This patch series is all about removing the OLR and guaranteeing that
>>> each piece
>>> of work gets its own personal request. That means that there is no more
>>> 'hoovering up of forgotten requests'. If the request does not get
>>> tracked then
>>> it will be leaked. Thus the add request call _must_ not fail. The
>>> previous patch
>>> should have already ensured that it _will_ not fail by removing the
>>> potential
>>> for running out of ring space. This patch enforces the rule by
>>> actually removing
>>> the early exit paths and the return code.
>>>
>>> Note that if something does manage to fail and the epilogue commands
>>> don't get
>>> written to the ring, the driver will still hang together. The request
>>> will be
>>> added to the tracking lists. And as in the old case, any subsequent
>>> work will
>>> generate a new seqno which will suffice for marking the old one as
>>> complete.
>>>
>>> v2: Improved WARNings (Tomas Elf review request).
>>>
>>> For: VIZ-5115
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h              |    6 ++--
>>>   drivers/gpu/drm/i915/i915_gem.c              |   43
>>> ++++++++++++--------------
>>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c   |    2 +-
>>>   drivers/gpu/drm/i915/i915_gem_render_state.c |    2 +-
>>>   drivers/gpu/drm/i915/intel_lrc.c             |    2 +-
>>>   drivers/gpu/drm/i915/intel_overlay.c         |    8 ++---
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c      |    8 ++---
>>>   7 files changed, 31 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index eba1857..1be4a52 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2860,9 +2860,9 @@ void i915_gem_init_swizzling(struct drm_device
>>> *dev);
>>>   void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
>>>   int __must_check i915_gpu_idle(struct drm_device *dev);
>>>   int __must_check i915_gem_suspend(struct drm_device *dev);
>>> -int __i915_add_request(struct intel_engine_cs *ring,
>>> -               struct drm_file *file,
>>> -               struct drm_i915_gem_object *batch_obj);
>>> +void __i915_add_request(struct intel_engine_cs *ring,
>>> +            struct drm_file *file,
>>> +            struct drm_i915_gem_object *batch_obj);
>>>   #define i915_add_request(ring) \
>>>       __i915_add_request(ring, NULL, NULL)
>>>   int __i915_wait_request(struct drm_i915_gem_request *req,
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index 6f51416..dd39aa5 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -1155,15 +1155,12 @@ i915_gem_check_wedge(struct i915_gpu_error
>>> *error,
>>>   int
>>>   i915_gem_check_olr(struct drm_i915_gem_request *req)
>>>   {
>>> -    int ret;
>>> -
>>> WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
>>>
>>> -    ret = 0;
>>>       if (req == req->ring->outstanding_lazy_request)
>>> -        ret = i915_add_request(req->ring);
>>> +        i915_add_request(req->ring);
>>>
>>> -    return ret;
>>> +    return 0;
>>>   }
>>
>> i915_gem_check_olr never returns anything but 0. How about making it
>> void?
> Seems like redundant/unnecessary changes given that the entire function
> is removed later in the series. Changing it from a '__must_check' to a
> 'void' now would be a lot of extra changes.
>
>> Thanks,
>> Tomas
>>
>>>
>>>   static void fake_irq(unsigned long data)
>>> @@ -2466,9 +2463,14 @@ i915_gem_get_seqno(struct drm_device *dev, u32
>>> *seqno)
>>>       return 0;
>>>   }
>>>
>>> -int __i915_add_request(struct intel_engine_cs *ring,
>>> -               struct drm_file *file,
>>> -               struct drm_i915_gem_object *obj)
>>> +/*
>>> + * 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
>>> + * going to happen on the hardware. This would be a Bad Thing(tm).
>>> + */
>>> +void __i915_add_request(struct intel_engine_cs *ring,
>>> +            struct drm_file *file,
>>> +            struct drm_i915_gem_object *obj)
>>>   {
>>>       struct drm_i915_private *dev_priv = ring->dev->dev_private;
>>>       struct drm_i915_gem_request *request;
>>> @@ -2478,7 +2480,7 @@ int __i915_add_request(struct intel_engine_cs
>>> *ring,
>>>
>>>       request = ring->outstanding_lazy_request;
>>>       if (WARN_ON(request == NULL))
>>> -        return -ENOMEM;
>>> +        return;
>>
>> You have a WARN for the other points of failure in this function, why
>> not here?
> Erm, you mean like the one in the 'if' itself?
>

Exactly! :)

Reviewed-by: Tomas Elf <tomas.elf@intel.com>

Thanks,
Tomas

>>
>>>
>>>       if (i915.enable_execlists) {
>>>           ringbuf = request->ctx->engine[ring->id].ringbuf;
>>> @@ -2500,15 +2502,12 @@ int __i915_add_request(struct intel_engine_cs
>>> *ring,
>>>        * is that the flush _must_ happen before the next request, no
>>> matter
>>>        * what.
>>>        */
>>> -    if (i915.enable_execlists) {
>>> +    if (i915.enable_execlists)
>>>           ret = logical_ring_flush_all_caches(ringbuf, request->ctx);
>>> -        if (ret)
>>> -            return ret;
>>> -    } else {
>>> +    else
>>>           ret = intel_ring_flush_all_caches(ring);
>>> -        if (ret)
>>> -            return ret;
>>> -    }
>>> +    /* Not allowed to fail! */
>>> +    WARN(ret, "*_ring_flush_all_caches failed: %d!\n", ret);
>>>
>>>       /* Record the position of the start of the request so that
>>>        * should we detect the updated seqno part-way through the
>>> @@ -2517,17 +2516,15 @@ int __i915_add_request(struct intel_engine_cs
>>> *ring,
>>>        */
>>>       request->postfix = intel_ring_get_tail(ringbuf);
>>>
>>> -    if (i915.enable_execlists) {
>>> +    if (i915.enable_execlists)
>>>           ret = ring->emit_request(ringbuf, request);
>>> -        if (ret)
>>> -            return ret;
>>> -    } else {
>>> +    else {
>>>           ret = ring->add_request(ring);
>>> -        if (ret)
>>> -            return ret;
>>>
>>>           request->tail = intel_ring_get_tail(ringbuf);
>>>       }
>>> +    /* Not allowed to fail! */
>>> +    WARN(ret, "emit|add_request failed: %d!\n", ret);
>>>
>>>       request->head = request_start;
>>>
>>> @@ -2576,8 +2573,6 @@ int __i915_add_request(struct intel_engine_cs
>>> *ring,
>>>
>>>       /* Sanity check that the reserved size was large enough. */
>>>       intel_ring_reserved_space_end(ringbuf);
>>> -
>>> -    return 0;
>>>   }
>>>
>>>   static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index bd0e4bd..2b48a31 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -1061,7 +1061,7 @@ i915_gem_execbuffer_retire_commands(struct
>>> drm_device *dev,
>>>       ring->gpu_caches_dirty = true;
>>>
>>>       /* Add a breadcrumb for the completion of the batch buffer */
>>> -    (void)__i915_add_request(ring, file, obj);
>>> +    __i915_add_request(ring, file, obj);
>>>   }
>>>
>>>   static int
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c
>>> b/drivers/gpu/drm/i915/i915_gem_render_state.c
>>> index 521548a..ce4788f 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
>>> @@ -173,7 +173,7 @@ int i915_gem_render_state_init(struct
>>> intel_engine_cs *ring)
>>>
>>>       i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), ring);
>>>
>>> -    ret = __i915_add_request(ring, NULL, so.obj);
>>> +    __i915_add_request(ring, NULL, so.obj);
>>>       /* __i915_add_request moves object to inactive if it fails */
>>>   out:
>>>       i915_gem_render_state_fini(&so);
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>> index e62d396..7a75fc8 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1373,7 +1373,7 @@ static int
>>> intel_lr_context_render_state_init(struct intel_engine_cs *ring,
>>>
>>>       i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), ring);
>>>
>>> -    ret = __i915_add_request(ring, file, so.obj);
>>> +    __i915_add_request(ring, file, so.obj);
>>>       /* intel_logical_ring_add_request moves object to inactive if it
>>>        * fails */
>>>   out:
>>> diff --git a/drivers/gpu/drm/i915/intel_overlay.c
>>> b/drivers/gpu/drm/i915/intel_overlay.c
>>> index 25c8ec6..e7534b9 100644
>>> --- a/drivers/gpu/drm/i915/intel_overlay.c
>>> +++ b/drivers/gpu/drm/i915/intel_overlay.c
>>> @@ -220,9 +220,7 @@ static int intel_overlay_do_wait_request(struct
>>> intel_overlay *overlay,
>>>       WARN_ON(overlay->last_flip_req);
>>>       i915_gem_request_assign(&overlay->last_flip_req,
>>>                            ring->outstanding_lazy_request);
>>> -    ret = i915_add_request(ring);
>>> -    if (ret)
>>> -        return ret;
>>> +    i915_add_request(ring);
>>>
>>>       overlay->flip_tail = tail;
>>>       ret = i915_wait_request(overlay->last_flip_req);
>>> @@ -291,7 +289,9 @@ static int intel_overlay_continue(struct
>>> intel_overlay *overlay,
>>>       WARN_ON(overlay->last_flip_req);
>>>       i915_gem_request_assign(&overlay->last_flip_req,
>>>                            ring->outstanding_lazy_request);
>>> -    return i915_add_request(ring);
>>> +    i915_add_request(ring);
>>> +
>>> +    return 0;
>>>   }
>>>
>>>   static void intel_overlay_release_old_vid_tail(struct intel_overlay
>>> *overlay)
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> index 74c2222..7061b07 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> @@ -2156,14 +2156,10 @@ static int intel_wrap_ring_buffer(struct
>>> intel_engine_cs *ring)
>>>   int intel_ring_idle(struct intel_engine_cs *ring)
>>>   {
>>>       struct drm_i915_gem_request *req;
>>> -    int ret;
>>>
>>>       /* We need to add any requests required to flush the objects
>>> and ring */
>>> -    if (ring->outstanding_lazy_request) {
>>> -        ret = i915_add_request(ring);
>>> -        if (ret)
>>> -            return ret;
>>> -    }
>>> +    if (ring->outstanding_lazy_request)
>>> +        i915_add_request(ring);
>>>
>>>       /* Wait upon the last request to be completed */
>>>       if (list_empty(&ring->request_list))
>>>
>>
>

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