[Intel-gfx] [PATCH 03/55] drm/i915: i915_add_request must not fail
John Harrison
John.C.Harrison at Intel.com
Thu Jun 4 07:07:55 PDT 2015
On 02/06/2015 19:16, Tomas Elf wrote:
> On 29/05/2015 17:43, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at 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 at 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?
>
>>
>> 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))
>>
>
More information about the Intel-gfx
mailing list