[22/55] drm/i915: Update deferred context creation to do explicit request management

Submitted by John.C.Harrison@Intel.com on May 29, 2015, 4:43 p.m.

Details

Message ID 1432917856-12261-23-git-send-email-John.C.Harrison@Intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

John.C.Harrison@Intel.com May 29, 2015, 4:43 p.m.
From: John Harrison <John.C.Harrison@Intel.com>

In execlist mode, context initialisation is deferred until first use of the
given context. This is because execlist mode has per ring context state and thus
many more context storage objects than legacy mode and many are never actually
used. Previously, the initialisation commands were written to the ring and
tagged with some random request structure via the OLR. This seemed to be causing
a null pointer deference bug under certain circumstances (BZ:88865).

This patch adds explicit request creation and submission to the deferred
initialisation code path. Thus removing any reliance on or randomness caused by
the OLR.

Note that it should be possible to move the deferred context creation until even
later - when the context is actually switched to rather than when it is merely
validated. This would allow the initialisation to be done within the request of
the work that is wanting to use the context. Hence, the extra request that is
created, used and retired just for the context init could be removed completely.
However, this is left for a follow up patch.

For: VIZ-5115
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 37efa93..2730efd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1971,13 +1971,22 @@  int intel_lr_context_deferred_create(struct intel_context *ctx,
 		lrc_setup_hardware_status_page(ring, ctx_obj);
 	else if (ring->id == RCS && !ctx->rcs_initialized) {
 		if (ring->init_context) {
-			ret = ring->init_context(ring, ctx);
+			struct drm_i915_gem_request *req;
+
+			ret = i915_gem_request_alloc(ring, ctx, &req);
+			if (ret)
+				return ret;
+
+			ret = ring->init_context(req->ring, ctx);
 			if (ret) {
 				DRM_ERROR("ring init context: %d\n", ret);
+				i915_gem_request_cancel(req);
 				ctx->engine[ring->id].ringbuf = NULL;
 				ctx->engine[ring->id].state = NULL;
 				goto error;
 			}
+
+			i915_add_request_no_flush(req->ring);
 		}
 
 		ctx->rcs_initialized = true;

Comments

On 29/05/2015 17:43, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> In execlist mode, context initialisation is deferred until first use of the
> given context. This is because execlist mode has per ring context state and thus
> many more context storage objects than legacy mode and many are never actually
> used. Previously, the initialisation commands were written to the ring and
> tagged with some random request structure via the OLR. This seemed to be causing
> a null pointer deference bug under certain circumstances (BZ:88865).
>
> This patch adds explicit request creation and submission to the deferred
> initialisation code path. Thus removing any reliance on or randomness caused by
> the OLR.
>
> Note that it should be possible to move the deferred context creation until even
> later - when the context is actually switched to rather than when it is merely
> validated. This would allow the initialisation to be done within the request of
> the work that is wanting to use the context. Hence, the extra request that is
> created, used and retired just for the context init could be removed completely.
> However, this is left for a follow up patch.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c |   11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 37efa93..2730efd 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1971,13 +1971,22 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
>   		lrc_setup_hardware_status_page(ring, ctx_obj);
>   	else if (ring->id == RCS && !ctx->rcs_initialized) {
>   		if (ring->init_context) {
> -			ret = ring->init_context(ring, ctx);
> +			struct drm_i915_gem_request *req;
> +
> +			ret = i915_gem_request_alloc(ring, ctx, &req);
> +			if (ret)
> +				return ret;
> +
> +			ret = ring->init_context(req->ring, ctx);
>   			if (ret) {
>   				DRM_ERROR("ring init context: %d\n", ret);
> +				i915_gem_request_cancel(req);
>   				ctx->engine[ring->id].ringbuf = NULL;
>   				ctx->engine[ring->id].state = NULL;
>   				goto error;
>   			}
> +
> +			i915_add_request_no_flush(req->ring);
>   		}
>
>   		ctx->rcs_initialized = true;
>

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

Thanks,
Tomas