[Intel-gfx] [PATCH 30/38] drm/i915: Make context pinning part of intel_context_ops
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Mar 5 17:31:23 UTC 2019
On 01/03/2019 14:03, Chris Wilson wrote:
> Push the intel_context pin callback down from intel_engine_cs onto the
> context itself by virtue of having a central caller for
> intel_context_pin() being able to lookup the intel_context itself.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_context.c | 34 ++++++
> drivers/gpu/drm/i915/intel_context.h | 7 +-
> drivers/gpu/drm/i915/intel_context_types.h | 1 +
> drivers/gpu/drm/i915/intel_engine_types.h | 2 -
> drivers/gpu/drm/i915/intel_lrc.c | 114 ++++++++-----------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 45 ++------
> drivers/gpu/drm/i915/selftests/mock_engine.c | 32 +-----
> 7 files changed, 98 insertions(+), 137 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c
> index 242b1b6ad253..7de02be0b552 100644
> --- a/drivers/gpu/drm/i915/intel_context.c
> +++ b/drivers/gpu/drm/i915/intel_context.c
> @@ -110,6 +110,40 @@ intel_context_instance(struct i915_gem_context *ctx,
> return pos;
> }
>
> +struct intel_context *
> +intel_context_pin(struct i915_gem_context *ctx,
> + struct intel_engine_cs *engine)
> +{
> + struct intel_context *ce;
> + int err;
> +
> + lockdep_assert_held(&ctx->i915->drm.struct_mutex);
> +
> + ce = intel_context_instance(ctx, engine);
> + if (IS_ERR(ce))
> + return ce;
> +
> + if (unlikely(!ce->pin_count++)) {
> + err = ce->ops->pin(ce);
> + if (err)
> + goto err_unpin;
> +
> + mutex_lock(&ctx->mutex);
> + list_add(&ce->active_link, &ctx->active_engines);
> + mutex_unlock(&ctx->mutex);
> +
> + i915_gem_context_get(ctx);
> + GEM_BUG_ON(ce->gem_context != ctx);
> + }
> + GEM_BUG_ON(!ce->pin_count); /* no overflow! */
> +
> + return ce;
> +
> +err_unpin:
> + ce->pin_count = 0;
> + return ERR_PTR(err);
> +}
> +
> static void intel_context_retire(struct i915_active_request *active,
> struct i915_request *rq)
> {
> diff --git a/drivers/gpu/drm/i915/intel_context.h b/drivers/gpu/drm/i915/intel_context.h
> index c3fffd9b8ae4..aa34311a472e 100644
> --- a/drivers/gpu/drm/i915/intel_context.h
> +++ b/drivers/gpu/drm/i915/intel_context.h
> @@ -45,11 +45,8 @@ __intel_context_insert(struct i915_gem_context *ctx,
> void
> __intel_context_remove(struct intel_context *ce);
>
> -static inline struct intel_context *
> -intel_context_pin(struct i915_gem_context *ctx, struct intel_engine_cs *engine)
> -{
> - return engine->context_pin(engine, ctx);
> -}
> +struct intel_context *
> +intel_context_pin(struct i915_gem_context *ctx, struct intel_engine_cs *engine);
>
> static inline void __intel_context_pin(struct intel_context *ce)
> {
> diff --git a/drivers/gpu/drm/i915/intel_context_types.h b/drivers/gpu/drm/i915/intel_context_types.h
> index 857f5c335324..804fa93de853 100644
> --- a/drivers/gpu/drm/i915/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/intel_context_types.h
> @@ -19,6 +19,7 @@ struct intel_context;
> struct intel_ring;
>
> struct intel_context_ops {
> + int (*pin)(struct intel_context *ce);
> void (*unpin)(struct intel_context *ce);
> void (*destroy)(struct intel_context *ce);
> };
> diff --git a/drivers/gpu/drm/i915/intel_engine_types.h b/drivers/gpu/drm/i915/intel_engine_types.h
> index 5dac6b439f95..aefc66605729 100644
> --- a/drivers/gpu/drm/i915/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/intel_engine_types.h
> @@ -357,8 +357,6 @@ struct intel_engine_cs {
> void (*set_default_submission)(struct intel_engine_cs *engine);
>
> const struct intel_context_ops *context;
> - struct intel_context *(*context_pin)(struct intel_engine_cs *engine,
> - struct i915_gem_context *ctx);
>
> int (*request_alloc)(struct i915_request *rq);
> int (*init_context)(struct i915_request *rq);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0800f8edffeb..8dddea8e1c97 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -166,9 +166,8 @@
>
> #define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT | I915_PRIORITY_NOSEMAPHORE)
>
> -static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine,
> - struct intel_context *ce);
> +static int execlists_context_deferred_alloc(struct intel_context *ce,
> + struct intel_engine_cs *engine);
> static void execlists_init_reg_state(u32 *reg_state,
> struct intel_context *ce,
> struct intel_engine_cs *engine,
> @@ -330,11 +329,10 @@ assert_priority_queue(const struct i915_request *prev,
> * engine info, SW context ID and SW counter need to form a unique number
> * (Context ID) per lrc.
> */
> -static void
> -intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine,
> - struct intel_context *ce)
> +static u64
> +lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
> {
> + struct i915_gem_context *ctx = ce->gem_context;
> u64 desc;
>
> BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (BIT(GEN8_CTX_ID_WIDTH)));
> @@ -352,7 +350,7 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
> * Consider updating oa_get_render_ctx_id in i915_perf.c when changing
> * anything below.
> */
> - if (INTEL_GEN(ctx->i915) >= 11) {
> + if (INTEL_GEN(engine->i915) >= 11) {
> GEM_BUG_ON(ctx->hw_id >= BIT(GEN11_SW_CTX_ID_WIDTH));
> desc |= (u64)ctx->hw_id << GEN11_SW_CTX_ID_SHIFT;
> /* bits 37-47 */
> @@ -369,7 +367,7 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
> desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-52 */
> }
>
> - ce->lrc_desc = desc;
> + return desc;
> }
>
> static void unwind_wa_tail(struct i915_request *rq)
> @@ -1284,7 +1282,7 @@ static void execlists_context_unpin(struct intel_context *ce)
> i915_gem_context_put(ce->gem_context);
> }
>
> -static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
> +static int __context_pin(struct i915_vma *vma)
> {
> unsigned int flags;
> int err;
> @@ -1307,11 +1305,14 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
> }
>
> static void
> -__execlists_update_reg_state(struct intel_engine_cs *engine,
> - struct intel_context *ce)
> +__execlists_update_reg_state(struct intel_context *ce,
> + struct intel_engine_cs *engine)
> {
> - u32 *regs = ce->lrc_reg_state;
> struct intel_ring *ring = ce->ring;
> + u32 *regs = ce->lrc_reg_state;
> +
> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
>
> regs[CTX_RING_BUFFER_START + 1] = i915_ggtt_offset(ring->vma);
> regs[CTX_RING_HEAD + 1] = ring->head;
> @@ -1324,25 +1325,26 @@ __execlists_update_reg_state(struct intel_engine_cs *engine,
> }
> }
>
> -static struct intel_context *
> -__execlists_context_pin(struct intel_engine_cs *engine,
> - struct i915_gem_context *ctx,
> - struct intel_context *ce)
> +static int
> +__execlists_context_pin(struct intel_context *ce,
> + struct intel_engine_cs *engine)
> {
> void *vaddr;
> int ret;
>
> - ret = execlists_context_deferred_alloc(ctx, engine, ce);
> + GEM_BUG_ON(!ce->gem_context->ppgtt);
> +
> + ret = execlists_context_deferred_alloc(ce, engine);
> if (ret)
> goto err;
> GEM_BUG_ON(!ce->state);
>
> - ret = __context_pin(ctx, ce->state);
> + ret = __context_pin(ce->state);
> if (ret)
> goto err;
>
> vaddr = i915_gem_object_pin_map(ce->state->obj,
> - i915_coherent_map_type(ctx->i915) |
> + i915_coherent_map_type(engine->i915) |
> I915_MAP_OVERRIDE);
> if (IS_ERR(vaddr)) {
> ret = PTR_ERR(vaddr);
> @@ -1353,26 +1355,16 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> if (ret)
> goto unpin_map;
>
> - ret = i915_gem_context_pin_hw_id(ctx);
> + ret = i915_gem_context_pin_hw_id(ce->gem_context);
> if (ret)
> goto unpin_ring;
>
> - intel_lr_context_descriptor_update(ctx, engine, ce);
> -
> - GEM_BUG_ON(!intel_ring_offset_valid(ce->ring, ce->ring->head));
> -
> + ce->lrc_desc = lrc_descriptor(ce, engine);
> ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
> -
> - __execlists_update_reg_state(engine, ce);
> + __execlists_update_reg_state(ce, engine);
>
> ce->state->obj->pin_global++;
> -
> - mutex_lock(&ctx->mutex);
> - list_add(&ce->active_link, &ctx->active_engines);
> - mutex_unlock(&ctx->mutex);
> -
> - i915_gem_context_get(ctx);
> - return ce;
> + return 0;
>
> unpin_ring:
> intel_ring_unpin(ce->ring);
> @@ -1381,31 +1373,16 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> unpin_vma:
> __i915_vma_unpin(ce->state);
> err:
> - ce->pin_count = 0;
> - return ERR_PTR(ret);
> + return ret;
> }
>
> -static struct intel_context *
> -execlists_context_pin(struct intel_engine_cs *engine,
> - struct i915_gem_context *ctx)
> +static int execlists_context_pin(struct intel_context *ce)
> {
> - struct intel_context *ce;
> -
> - ce = intel_context_instance(ctx, engine);
> - if (IS_ERR(ce))
> - return ce;
> -
> - lockdep_assert_held(&ctx->i915->drm.struct_mutex);
> - GEM_BUG_ON(!ctx->ppgtt);
> -
> - if (likely(ce->pin_count++))
> - return ce;
> - GEM_BUG_ON(!ce->pin_count); /* no overflow please! */
> -
> - return __execlists_context_pin(engine, ctx, ce);
> + return __execlists_context_pin(ce, ce->engine);
> }
Do you need this trivial wrapper for later?
>
> static const struct intel_context_ops execlists_context_ops = {
> + .pin = execlists_context_pin,
> .unpin = execlists_context_unpin,
> .destroy = execlists_context_destroy,
> };
> @@ -2029,7 +2006,7 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
> intel_ring_update_space(rq->ring);
>
> execlists_init_reg_state(regs, rq->hw_context, engine, rq->ring);
> - __execlists_update_reg_state(engine, rq->hw_context);
> + __execlists_update_reg_state(rq->hw_context, engine);
>
> out_unlock:
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> @@ -2354,7 +2331,6 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> engine->reset.finish = execlists_reset_finish;
>
> engine->context = &execlists_context_ops;
> - engine->context_pin = execlists_context_pin;
> engine->request_alloc = execlists_request_alloc;
>
> engine->emit_flush = gen8_emit_flush;
> @@ -2831,9 +2807,16 @@ populate_lr_context(struct intel_context *ce,
> return ret;
> }
>
> -static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine,
> - struct intel_context *ce)
> +static struct i915_timeline *get_timeline(struct i915_gem_context *ctx)
> +{
> + if (ctx->timeline)
> + return i915_timeline_get(ctx->timeline);
> + else
> + return i915_timeline_create(ctx->i915, ctx->name, NULL);
> +}
> +
> +static int execlists_context_deferred_alloc(struct intel_context *ce,
> + struct intel_engine_cs *engine)
> {
> struct drm_i915_gem_object *ctx_obj;
> struct i915_vma *vma;
> @@ -2853,26 +2836,25 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> */
> context_size += LRC_HEADER_PAGES * PAGE_SIZE;
>
> - ctx_obj = i915_gem_object_create(ctx->i915, context_size);
> + ctx_obj = i915_gem_object_create(engine->i915, context_size);
> if (IS_ERR(ctx_obj))
> return PTR_ERR(ctx_obj);
>
> - vma = i915_vma_instance(ctx_obj, &ctx->i915->ggtt.vm, NULL);
> + vma = i915_vma_instance(ctx_obj, &engine->i915->ggtt.vm, NULL);
> if (IS_ERR(vma)) {
> ret = PTR_ERR(vma);
> goto error_deref_obj;
> }
>
> - if (ctx->timeline)
> - timeline = i915_timeline_get(ctx->timeline);
> - else
> - timeline = i915_timeline_create(ctx->i915, ctx->name, NULL);
> + timeline = get_timeline(ce->gem_context);
> if (IS_ERR(timeline)) {
> ret = PTR_ERR(timeline);
> goto error_deref_obj;
> }
>
> - ring = intel_engine_create_ring(engine, timeline, ctx->ring_size);
> + ring = intel_engine_create_ring(engine,
> + timeline,
> + ce->gem_context->ring_size);
> i915_timeline_put(timeline);
> if (IS_ERR(ring)) {
> ret = PTR_ERR(ring);
> @@ -2917,7 +2899,7 @@ void intel_lr_context_resume(struct drm_i915_private *i915)
> list_for_each_entry(ce, &ctx->active_engines, active_link) {
> GEM_BUG_ON(!ce->ring);
> intel_ring_reset(ce->ring, 0);
> - __execlists_update_reg_state(ce->engine, ce);
> + __execlists_update_reg_state(ce, ce->engine);
> }
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 9002f7f6d32e..6af2d393b7ff 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1508,11 +1508,9 @@ alloc_context_vma(struct intel_engine_cs *engine)
> return ERR_PTR(err);
> }
>
> -static struct intel_context *
> -__ring_context_pin(struct intel_engine_cs *engine,
> - struct i915_gem_context *ctx,
> - struct intel_context *ce)
> +static int ring_context_pin(struct intel_context *ce)
> {
> + struct intel_engine_cs *engine = ce->engine;
> int err;
>
> /* One ringbuffer to rule them all */
> @@ -1523,55 +1521,29 @@ __ring_context_pin(struct intel_engine_cs *engine,
> struct i915_vma *vma;
>
> vma = alloc_context_vma(engine);
> - if (IS_ERR(vma)) {
> - err = PTR_ERR(vma);
> - goto err;
> - }
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
>
> ce->state = vma;
> }
>
> err = __context_pin(ce);
> if (err)
> - goto err;
> + return err;
>
> err = __context_pin_ppgtt(ce->gem_context);
> if (err)
> goto err_unpin;
>
> - mutex_lock(&ctx->mutex);
> - list_add(&ce->active_link, &ctx->active_engines);
> - mutex_unlock(&ctx->mutex);
> -
> - i915_gem_context_get(ctx);
> - return ce;
> + return 0;
>
> err_unpin:
> __context_unpin(ce);
> -err:
> - ce->pin_count = 0;
> - return ERR_PTR(err);
> -}
> -
> -static struct intel_context *
> -ring_context_pin(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
> -{
> - struct intel_context *ce;
> -
> - ce = intel_context_instance(ctx, engine);
> - if (IS_ERR(ce))
> - return ce;
> -
> - lockdep_assert_held(&ctx->i915->drm.struct_mutex);
> -
> - if (likely(ce->pin_count++))
> - return ce;
> - GEM_BUG_ON(!ce->pin_count); /* no overflow please! */
> -
> - return __ring_context_pin(engine, ctx, ce);
> + return err;
> }
>
> static const struct intel_context_ops ring_context_ops = {
> + .pin = ring_context_pin,
> .unpin = ring_context_unpin,
> .destroy = ring_context_destroy,
> };
> @@ -2282,7 +2254,6 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
> engine->reset.finish = reset_finish;
>
> engine->context = &ring_context_ops;
> - engine->context_pin = ring_context_pin;
> engine->request_alloc = ring_request_alloc;
>
> /*
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index f1a80006289d..5a5c3ba22061 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -137,41 +137,20 @@ static void mock_context_destroy(struct intel_context *ce)
> mock_ring_free(ce->ring);
> }
>
> -static struct intel_context *
> -mock_context_pin(struct intel_engine_cs *engine,
> - struct i915_gem_context *ctx)
> +static int mock_context_pin(struct intel_context *ce)
> {
> - struct intel_context *ce;
> - int err = -ENOMEM;
> -
> - ce = intel_context_instance(ctx, engine);
> - if (IS_ERR(ce))
> - return ce;
> -
> - if (ce->pin_count++)
> - return ce;
> -
> if (!ce->ring) {
> - ce->ring = mock_ring(engine);
> + ce->ring = mock_ring(ce->engine);
> if (!ce->ring)
> - goto err;
> + return -ENOMEM;
> }
>
> mock_timeline_pin(ce->ring->timeline);
> -
> - mutex_lock(&ctx->mutex);
> - list_add(&ce->active_link, &ctx->active_engines);
> - mutex_unlock(&ctx->mutex);
> -
> - i915_gem_context_get(ctx);
> - return ce;
> -
> -err:
> - ce->pin_count = 0;
> - return ERR_PTR(err);
> + return 0;
> }
>
> static const struct intel_context_ops mock_context_ops = {
> + .pin = mock_context_pin,
> .unpin = mock_context_unpin,
> .destroy = mock_context_destroy,
> };
> @@ -235,7 +214,6 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
> engine->base.status_page.addr = (void *)(engine + 1);
>
> engine->base.context = &mock_context_ops;
> - engine->base.context_pin = mock_context_pin;
> engine->base.request_alloc = mock_request_alloc;
> engine->base.emit_flush = mock_emit_flush;
> engine->base.emit_fini_breadcrumb = mock_emit_breadcrumb;
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list