[Intel-gfx] [CI-ping 15/15] drm/i915: Late request cancellations are harmful
Daniel Vetter
daniel at ffwll.ch
Wed Apr 13 09:57:21 UTC 2016
On Tue, Apr 12, 2016 at 09:03:09PM +0100, Chris Wilson wrote:
> Conceptually, each request is a record of a hardware transaction - we
> build up a list of pending commands and then either commit them to
> hardware, or cancel them. However, whilst building up the list of
> pending commands, we may modify state outside of the request and make
> references to the pending request. If we do so and then cancel that
> request, external objects then point to the deleted request leading to
> both graphical and memory corruption.
>
> The easiest example is to consider object/VMA tracking. When we mark an
> object as active in a request, we store a pointer to this, the most
> recent request, in the object. Then we want to free that object, we wait
> for the most recent request to be idle before proceeding (otherwise the
> hardware will write to pages now owned by the system, or we will attempt
> to read from those pages before the hardware is finished writing). If
> the request was cancelled instead, that wait completes immediately. As a
> result, all requests must be committed and not cancelled if the external
> state is unknown.
>
> All that remains of i915_gem_request_cancel() users are just a couple of
> extremely unlikely allocation failures, so remove the API entirely.
>
> A consequence of committing all incomplete requests is that we generate
> excess breadcrumbs and fill the ring much more often with dummy work. We
> have completely undone the outstanding_last_seqno optimisation.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93907
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: stable at vger.kernel.org
Cc: John Harrison <John.C.Harrison at Intel.com>
I'd like John's ack on this on too, but patch itself looks sound. Fast r-b
since we've discussed this a while ago already ...
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 --
> drivers/gpu/drm/i915/i915_gem.c | 50 ++++++++++++------------------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 4 +--
> drivers/gpu/drm/i915/intel_overlay.c | 8 ++---
> 6 files changed, 30 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 061ecc43d935..de84dd7be971 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2331,7 +2331,6 @@ struct drm_i915_gem_request {
> struct drm_i915_gem_request * __must_check
> i915_gem_request_alloc(struct intel_engine_cs *engine,
> struct intel_context *ctx);
> -void i915_gem_request_cancel(struct drm_i915_gem_request *req);
> void i915_gem_request_free(struct kref *req_ref);
> int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> struct drm_file *file);
> @@ -2883,7 +2882,6 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> struct drm_i915_gem_request *req);
> -void i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
> int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> struct drm_i915_gem_execbuffer2 *args,
> struct list_head *vmas);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b6879d43dd74..c6f09e7839ea 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2785,7 +2785,8 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
> * fully prepared. Thus it can be cleaned up using the proper
> * free code.
> */
> - i915_gem_request_cancel(req);
> + intel_ring_reserved_space_cancel(req->ringbuf);
> + i915_gem_request_unreference(req);
> return ret;
> }
>
> @@ -2822,13 +2823,6 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> return err ? ERR_PTR(err) : req;
> }
>
> -void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> -{
> - intel_ring_reserved_space_cancel(req->ringbuf);
> -
> - i915_gem_request_unreference(req);
> -}
> -
> struct drm_i915_gem_request *
> i915_gem_find_active_request(struct intel_engine_cs *engine)
> {
> @@ -3438,12 +3432,9 @@ int i915_gpu_idle(struct drm_device *dev)
> return PTR_ERR(req);
>
> ret = i915_switch_context(req);
> - if (ret) {
> - i915_gem_request_cancel(req);
> - return ret;
> - }
> -
> i915_add_request_no_flush(req);
> + if (ret)
> + return ret;
> }
>
> ret = intel_engine_idle(engine);
> @@ -4943,34 +4934,33 @@ i915_gem_init_hw(struct drm_device *dev)
> req = i915_gem_request_alloc(engine, NULL);
> if (IS_ERR(req)) {
> ret = PTR_ERR(req);
> - i915_gem_cleanup_engines(dev);
> - goto out;
> + break;
> }
>
> if (engine->id == RCS) {
> - for (j = 0; j < NUM_L3_SLICES(dev); j++)
> - i915_gem_l3_remap(req, j);
> + for (j = 0; j < NUM_L3_SLICES(dev); j++) {
> + ret = i915_gem_l3_remap(req, j);
> + if (ret)
> + goto err_request;
> + }
> }
>
> ret = i915_ppgtt_init_ring(req);
> - if (ret && ret != -EIO) {
> - DRM_ERROR("PPGTT enable %s failed %d\n",
> - engine->name, ret);
> - i915_gem_request_cancel(req);
> - i915_gem_cleanup_engines(dev);
> - goto out;
> - }
> + if (ret)
> + goto err_request;
>
> ret = i915_gem_context_enable(req);
> - if (ret && ret != -EIO) {
> - DRM_ERROR("Context enable %s failed %d\n",
> + if (ret)
> + goto err_request;
> +
> +err_request:
> + i915_add_request_no_flush(req);
> + if (ret) {
> + DRM_ERROR("Failed to enable %s, error=%d\n",
> engine->name, ret);
> - i915_gem_request_cancel(req);
> i915_gem_cleanup_engines(dev);
> - goto out;
> + break;
> }
> -
> - i915_add_request_no_flush(req);
> }
>
> out:
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 6ee4f00f620c..6f4f2a6cdf93 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1137,7 +1137,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> }
> }
>
> -void
> +static void
> i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
> {
> /* Unconditionally force add_request to emit a full flush. */
> @@ -1322,7 +1322,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>
> i915_gem_execbuffer_move_to_active(vmas, params->request);
> - i915_gem_execbuffer_retire_commands(params);
>
> return 0;
> }
> @@ -1624,7 +1623,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>
> ret = i915_gem_request_add_to_client(req, file);
> if (ret)
> - goto err_batch_unpin;
> + goto err_request;
>
> /*
> * Save assorted stuff away to pass through to *_submission().
> @@ -1641,6 +1640,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> params->request = req;
>
> ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
> +err_request:
> + i915_gem_execbuffer_retire_commands(params);
>
> err_batch_unpin:
> /*
> @@ -1657,14 +1658,6 @@ err:
> i915_gem_context_unreference(ctx);
> eb_destroy(eb);
>
> - /*
> - * If the request was created but not successfully submitted then it
> - * must be freed again. If it was submitted then it is being tracked
> - * on the active request list and no clean up is required here.
> - */
> - if (ret && !IS_ERR_OR_NULL(req))
> - i915_gem_request_cancel(req);
> -
> mutex_unlock(&dev->struct_mutex);
>
> pre_mutex_err:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1b457864e17..3cae596d10a3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11664,7 +11664,7 @@ cleanup_unpin:
> intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
> cleanup_pending:
> if (!IS_ERR_OR_NULL(request))
> - i915_gem_request_cancel(request);
> + i915_add_request_no_flush(request);
> atomic_dec(&intel_crtc->unpin_work_count);
> mutex_unlock(&dev->struct_mutex);
> cleanup:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b8f6b96472a6..6fc24deaa16a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1010,7 +1010,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
> trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>
> i915_gem_execbuffer_move_to_active(vmas, params->request);
> - i915_gem_execbuffer_retire_commands(params);
>
> return 0;
> }
> @@ -2679,13 +2678,12 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> }
>
> ret = engine->init_context(req);
> + i915_add_request_no_flush(req);
> if (ret) {
> DRM_ERROR("ring init context: %d\n",
> ret);
> - i915_gem_request_cancel(req);
> goto error_ringbuf;
> }
> - i915_add_request_no_flush(req);
> }
> return 0;
>
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 6694e9230cd5..bcc3b6a016d8 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -247,7 +247,7 @@ static int intel_overlay_on(struct intel_overlay *overlay)
>
> ret = intel_ring_begin(req, 4);
> if (ret) {
> - i915_gem_request_cancel(req);
> + i915_add_request_no_flush(req);
> return ret;
> }
>
> @@ -290,7 +290,7 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
>
> ret = intel_ring_begin(req, 2);
> if (ret) {
> - i915_gem_request_cancel(req);
> + i915_add_request_no_flush(req);
> return ret;
> }
>
> @@ -356,7 +356,7 @@ static int intel_overlay_off(struct intel_overlay *overlay)
>
> ret = intel_ring_begin(req, 6);
> if (ret) {
> - i915_gem_request_cancel(req);
> + i915_add_request_no_flush(req);
> return ret;
> }
>
> @@ -431,7 +431,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
>
> ret = intel_ring_begin(req, 2);
> if (ret) {
> - i915_gem_request_cancel(req);
> + i915_add_request_no_flush(req);
> return ret;
> }
>
> --
> 2.8.0.rc3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list