| Message ID | 1461048560-31983-7-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/04/16 07:49, 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. A few words on pros and cons? > 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(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 77becfd5a09d..b65e3b16e1b0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -870,6 +870,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 79ac6f06a502..c104015813d3 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -669,9 +669,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) { > /* > @@ -686,7 +687,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, > @@ -2576,25 +2590,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: > Otherwise looks correct. Just missing the motivation explained. Regards, Tvrtko
On 19/04/16 07:49, 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. > > 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(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 77becfd5a09d..b65e3b16e1b0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -870,6 +870,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 79ac6f06a502..c104015813d3 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -669,9 +669,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) { > /* > @@ -686,7 +687,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); Actually, change here is that previously this wasn't done for the kernel context. Needs to be explained why we either want that, or why it is fine. > + 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, > @@ -2576,25 +2590,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: > Regards, Tvrtko
On Tue, Apr 19, 2016 at 11:15:31AM +0100, Tvrtko Ursulin wrote: > > On 19/04/16 07:49, 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. > > > >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(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >index 77becfd5a09d..b65e3b16e1b0 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -870,6 +870,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 79ac6f06a502..c104015813d3 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -669,9 +669,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) { > > /* > >@@ -686,7 +687,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); > > Actually, change here is that previously this wasn't done for the > kernel context. Needs to be explained why we either want that, or > why it is fine. We never execute the kernel context with execlists. It really is just a gigantic placeholder for the HWS (and golden context state for GuC). If we had, we would have noticed that we don't set the wa registers for it. Hopefully, we would have noticed! -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 77becfd5a09d..b65e3b16e1b0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -870,6 +870,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 79ac6f06a502..c104015813d3 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -669,9 +669,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) { /* @@ -686,7 +687,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, @@ -2576,25 +2590,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. 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(-)