| Message ID | 1461177750-20187-15-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > Instead of allocating a new request when allocating a context, use the > request that initiated the allocation to emit the context > initialisation. This serves two purposes, it makes the initialisation > atomic with first use (simplifying scheduling and our own error > handling). Secondly, it enables us to remove the explicit context > allocation required by higher levels of GEM and make that property of > execlists opaque (in the next patch). There is also a minor step > forwards towards convergence of legacy/execlist contexts. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Nitpick below. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++--------------------- > 2 files changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2bf3a8f97d52..e4b510bcee62 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -868,6 +868,7 @@ struct intel_context { > struct i915_vma *lrc_vma; > u64 lrc_desc; > uint32_t *lrc_reg_state; > + bool initialised; > } engine[I915_NUM_ENGINES]; > > struct list_head link; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 838abd4b42a3..d765267153c5 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -672,9 +672,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req, > > int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request) > { > - int ret = 0; > + struct intel_engine_cs *engine = request->engine; > + int ret; > > - request->ringbuf = request->ctx->engine[request->engine->id].ringbuf; > + request->ringbuf = request->ctx->engine[engine->id].ringbuf; > > if (i915.enable_guc_submission) { > /* > @@ -689,7 +690,20 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request > return ret; > } > > - return intel_lr_context_pin(request->ctx, request->engine); > + ret = intel_lr_context_pin(request->ctx, engine); > + if (ret) > + return ret; > + > + if (!request->ctx->engine[engine->id].initialised) { > + ret = engine->init_context(request); > + if (ret) { > + intel_lr_context_unpin(request->ctx, engine); I prefer the goto teardown path, it's easy to read and modify later on. > + return ret; > + } > + request->ctx->engine[engine->id].initialised = true; > + } > + > + return 0; > } > > static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, > @@ -2634,25 +2648,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx, > > ctx->engine[engine->id].ringbuf = ringbuf; > ctx->engine[engine->id].state = ctx_obj; > + ctx->engine[engine->id].initialised = engine->init_context == NULL; > > - if (ctx != ctx->i915->kernel_context && engine->init_context) { > - struct drm_i915_gem_request *req; > - > - req = i915_gem_request_alloc(engine, ctx); > - if (IS_ERR(req)) { > - ret = PTR_ERR(req); > - DRM_ERROR("ring create req: %d\n", ret); > - goto error_ringbuf; > - } > - > - ret = engine->init_context(req); > - i915_add_request_no_flush(req); > - if (ret) { > - DRM_ERROR("ring init context: %d\n", > - ret); > - goto error_ringbuf; > - } > - } > return 0; > > error_ringbuf:
On Thu, Apr 21, 2016 at 09:57:03AM +0300, Joonas Lahtinen wrote: > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > > + if (!request->ctx->engine[engine->id].initialised) { > > + ret = engine->init_context(request); > > + if (ret) { > > + intel_lr_context_unpin(request->ctx, engine); > > I prefer the goto teardown path, it's easy to read and modify later on. Ah, that would lead to bugs here. After we emit init_context on this request, the request must run to completion as the request itself tracks modification to global data, e.g. the ctx->initialised flag here and the golden render state object's liveness tracking. Well that deserves a comment! -Chris
On to, 2016-04-21 at 08:08 +0100, Chris Wilson wrote: > On Thu, Apr 21, 2016 at 09:57:03AM +0300, Joonas Lahtinen wrote: > > > > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > > > > > > + if (!request->ctx->engine[engine->id].initialised) { > > > + ret = engine->init_context(request); > > > + if (ret) { > > > + intel_lr_context_unpin(request->ctx, engine); > > I prefer the goto teardown path, it's easy to read and modify later on. Meant something like this which is functionally the same but less nesting; if (request->ctx->engine[engine->id].initialised) return 0; ret = engine->init_context(request); if (ret) goto out_unpin; request->ctx->engine[engine->id].initialised = true; return 0; out_unpin: intel_lr_context_unpin(request->ctx, engine); return ret; } > Ah, that would lead to bugs here. After we emit init_context on this > request, the request must run to completion as the request itself tracks > modification to global data, e.g. the ctx->initialised flag here and the > golden render state object's liveness tracking. > > Well that deserves a comment! > -Chris >
On Thu, Apr 21, 2016 at 10:47:30AM +0300, Joonas Lahtinen wrote: > On to, 2016-04-21 at 08:08 +0100, Chris Wilson wrote: > > On Thu, Apr 21, 2016 at 09:57:03AM +0300, Joonas Lahtinen wrote: > > > > > > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > > > > > > > > + if (!request->ctx->engine[engine->id].initialised) { > > > > + ret = engine->init_context(request); > > > > + if (ret) { > > > > + intel_lr_context_unpin(request->ctx, engine); > > > I prefer the goto teardown path, it's easy to read and modify later on. > > Meant something like this which is functionally the same but less > nesting; > > if (request->ctx->engine[engine->id].initialised) > return 0; > > ret = engine->init_context(request); > if (ret) > goto out_unpin; > > request->ctx->engine[engine->id].initialised = true; > > return 0; > > out_unpin: > intel_lr_context_unpin(request->ctx, engine); > > return ret; Yes, I am arguing that this leaves the next person thinking it will be safe to extend the unwind. And so by not conforming to the idiom, it should be more of a danger signal. Compromise? if (!request->ctx->engine[engine->id].initialised) { ret = engine->init_context(request); if (ret) goto err_unpin; request->ctx->engine[engine->id].initialised = true; } /* Note that after this point, we have committed to using * this request as it is being used to both track the * state of engine initialisation and liveness of the * golden renderstate above. Think twice before you try * to cancel/unwind this request now. */ return 0; err_unpin: intel_lr_context_unpin(request->ctx, engine); return ret; }
On to, 2016-04-21 at 08:56 +0100, Chris Wilson wrote: > On Thu, Apr 21, 2016 at 10:47:30AM +0300, Joonas Lahtinen wrote: > > > > On to, 2016-04-21 at 08:08 +0100, Chris Wilson wrote: > > > > > > On Thu, Apr 21, 2016 at 09:57:03AM +0300, Joonas Lahtinen wrote: > > > > > > > > > > > > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > > > > > > > > > > > > > > > + if (!request->ctx->engine[engine->id].initialised) { > > > > > + ret = engine->init_context(request); > > > > > + if (ret) { > > > > > + intel_lr_context_unpin(request->ctx, engine); > > > > I prefer the goto teardown path, it's easy to read and modify later on. > > Meant something like this which is functionally the same but less > > nesting; > > > > if (request->ctx->engine[engine->id].initialised) > > return 0; > > > > ret = engine->init_context(request); > > if (ret) > > goto out_unpin; > > > > request->ctx->engine[engine->id].initialised = true; > > > > return 0; > > > > out_unpin: > > intel_lr_context_unpin(request->ctx, engine); > > > > return ret; > Yes, I am arguing that this leaves the next person thinking it will be > safe to extend the unwind. And so by not conforming to the idiom, it > should be more of a danger signal. > > Compromise? Fair enough. > > if (!request->ctx->engine[engine->id].initialised) { > ret = engine->init_context(request); > if (ret) > goto err_unpin; > > request->ctx->engine[engine->id].initialised = true; > } > > /* Note that after this point, we have committed to using > * this request as it is being used to both track the > * state of engine initialisation and liveness of the > * golden renderstate above. Think twice before you try > * to cancel/unwind this request now. > */ > > return 0; > > err_unpin: > intel_lr_context_unpin(request->ctx, engine); > return ret; > } > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2bf3a8f97d52..e4b510bcee62 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -868,6 +868,7 @@ struct intel_context { struct i915_vma *lrc_vma; u64 lrc_desc; uint32_t *lrc_reg_state; + bool initialised; } engine[I915_NUM_ENGINES]; struct list_head link; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 838abd4b42a3..d765267153c5 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -672,9 +672,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req, int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request) { - int ret = 0; + struct intel_engine_cs *engine = request->engine; + int ret; - request->ringbuf = request->ctx->engine[request->engine->id].ringbuf; + request->ringbuf = request->ctx->engine[engine->id].ringbuf; if (i915.enable_guc_submission) { /* @@ -689,7 +690,20 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request return ret; } - return intel_lr_context_pin(request->ctx, request->engine); + ret = intel_lr_context_pin(request->ctx, engine); + if (ret) + return ret; + + if (!request->ctx->engine[engine->id].initialised) { + ret = engine->init_context(request); + if (ret) { + intel_lr_context_unpin(request->ctx, engine); + return ret; + } + request->ctx->engine[engine->id].initialised = true; + } + + return 0; } static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, @@ -2634,25 +2648,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx, ctx->engine[engine->id].ringbuf = ringbuf; ctx->engine[engine->id].state = ctx_obj; + ctx->engine[engine->id].initialised = engine->init_context == NULL; - if (ctx != ctx->i915->kernel_context && engine->init_context) { - struct drm_i915_gem_request *req; - - req = i915_gem_request_alloc(engine, ctx); - if (IS_ERR(req)) { - ret = PTR_ERR(req); - DRM_ERROR("ring create req: %d\n", ret); - goto error_ringbuf; - } - - ret = engine->init_context(req); - i915_add_request_no_flush(req); - if (ret) { - DRM_ERROR("ring init context: %d\n", - ret); - goto error_ringbuf; - } - } return 0; error_ringbuf:
Instead of allocating a new request when allocating a context, use the request that initiated the allocation to emit the context initialisation. This serves two purposes, it makes the initialisation atomic with first use (simplifying scheduling and our own error handling). Secondly, it enables us to remove the explicit context allocation required by higher levels of GEM and make that property of execlists opaque (in the next patch). There is also a minor step forwards towards convergence of legacy/execlist contexts. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++--------------------- 2 files changed, 19 insertions(+), 21 deletions(-)