[Intel-gfx] [PATCH 16/29] drm/i915: Export intel_context_instance()
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Apr 11 12:57:50 UTC 2019
On 10/04/2019 20:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-04-10 13:06:04)
>>
>> On 08/04/2019 10:17, Chris Wilson wrote:
>>> We want to pass in a intel_context into intel_context_pin() and that
>>> requires us to first be able to lookup the intel_context!
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_context.c | 37 +++++++++++-----------
>>> drivers/gpu/drm/i915/gt/intel_context.h | 19 +++++++----
>>> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 8 ++++-
>>> drivers/gpu/drm/i915/gt/mock_engine.c | 8 ++++-
>>> drivers/gpu/drm/i915/gvt/scheduler.c | 7 +++-
>>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++--
>>> drivers/gpu/drm/i915/i915_perf.c | 21 ++++++++----
>>> drivers/gpu/drm/i915/i915_request.c | 11 ++++++-
>>> 8 files changed, 83 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>>> index 6ae6a3f58364..a1267739e369 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>>> @@ -104,7 +104,7 @@ void __intel_context_remove(struct intel_context *ce)
>>> spin_unlock(&ctx->hw_contexts_lock);
>>> }
>>>
>>> -static struct intel_context *
>>> +struct intel_context *
>>> intel_context_instance(struct i915_gem_context *ctx,
>>> struct intel_engine_cs *engine)
>>> {
>>> @@ -112,7 +112,7 @@ intel_context_instance(struct i915_gem_context *ctx,
>>>
>>> ce = intel_context_lookup(ctx, engine);
>>> if (likely(ce))
>>> - return ce;
>>> + return intel_context_get(ce);
>>>
>>> ce = intel_context_alloc();
>>> if (!ce)
>>> @@ -125,7 +125,7 @@ intel_context_instance(struct i915_gem_context *ctx,
>>> intel_context_free(ce);
>>>
>>> GEM_BUG_ON(intel_context_lookup(ctx, engine) != pos);
>>> - return pos;
>>> + return intel_context_get(pos);
>>> }
>>>
>>> struct intel_context *
>>> @@ -139,30 +139,30 @@ intel_context_pin_lock(struct i915_gem_context *ctx,
>>> if (IS_ERR(ce))
>>> return ce;
>>>
>>> - if (mutex_lock_interruptible(&ce->pin_mutex))
>>> + if (mutex_lock_interruptible(&ce->pin_mutex)) {
>>> + intel_context_put(ce);
>>> return ERR_PTR(-EINTR);
>>> + }
>>>
>>> return ce;
>>> }
>>>
>>> -struct intel_context *
>>> -intel_context_pin(struct i915_gem_context *ctx,
>>> - struct intel_engine_cs *engine)
>>> +void intel_context_pin_unlock(struct intel_context *ce)
>>> + __releases(ce->pin_mutex)
>>> {
>>> - struct intel_context *ce;
>>> - int err;
>>> -
>>> - ce = intel_context_instance(ctx, engine);
>>> - if (IS_ERR(ce))
>>> - return ce;
>>> + mutex_unlock(&ce->pin_mutex);
>>> + intel_context_put(ce);
>>> +}
>>>
>>> - if (likely(atomic_inc_not_zero(&ce->pin_count)))
>>> - return ce;
>>> +int __intel_context_do_pin(struct intel_context *ce)
>>> +{
>>> + int err;
>>>
>>> if (mutex_lock_interruptible(&ce->pin_mutex))
>>> - return ERR_PTR(-EINTR);
>>> + return -EINTR;
>>>
>>> if (likely(!atomic_read(&ce->pin_count))) {
>>> + struct i915_gem_context *ctx = ce->gem_context;
>>> intel_wakeref_t wakeref;
>>>
>>> err = 0;
>>> @@ -172,7 +172,6 @@ intel_context_pin(struct i915_gem_context *ctx,
>>> goto err;
>>>
>>> i915_gem_context_get(ctx);
>>> - GEM_BUG_ON(ce->gem_context != ctx);
>>>
>>> mutex_lock(&ctx->mutex);
>>> list_add(&ce->active_link, &ctx->active_engines);
>>> @@ -186,11 +185,11 @@ intel_context_pin(struct i915_gem_context *ctx,
>>> GEM_BUG_ON(!intel_context_is_pinned(ce)); /* no overflow! */
>>>
>>> mutex_unlock(&ce->pin_mutex);
>>> - return ce;
>>> + return 0;
>>>
>>> err:
>>> mutex_unlock(&ce->pin_mutex);
>>> - return ERR_PTR(err);
>>> + return err;
>>> }
>>>
>>> void intel_context_unpin(struct intel_context *ce)
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
>>> index 9aeef88176b9..da342e9a8c2e 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
>>> @@ -49,11 +49,7 @@ intel_context_is_pinned(struct intel_context *ce)
>>> return atomic_read(&ce->pin_count);
>>> }
>>>
>>> -static inline void intel_context_pin_unlock(struct intel_context *ce)
>>> -__releases(ce->pin_mutex)
>>> -{
>>> - mutex_unlock(&ce->pin_mutex);
>>> -}
>>
>> Could leave this as static inline since the only addition is kref_put so
>> compiler could decide what to do? Don't mind either way.
>
> In the next (or two) patch.
>
>>> +void intel_context_pin_unlock(struct intel_context *ce);
>>>
>>> struct intel_context *
>>> __intel_context_insert(struct i915_gem_context *ctx,
>>> @@ -63,7 +59,18 @@ void
>>> __intel_context_remove(struct intel_context *ce);
>>>
>>> struct intel_context *
>>> -intel_context_pin(struct i915_gem_context *ctx, struct intel_engine_cs *engine);
>>> +intel_context_instance(struct i915_gem_context *ctx,
>>> + struct intel_engine_cs *engine);
>>> +
>>> +int __intel_context_do_pin(struct intel_context *ce);
>>> +
>>> +static inline int intel_context_pin(struct intel_context *ce)
>>> +{
>>> + if (likely(atomic_inc_not_zero(&ce->pin_count)))
>>> + return 0;
>>> +
>>> + return __intel_context_do_pin(ce);
>>> +}
>>>
>>> static inline void __intel_context_pin(struct intel_context *ce)
>>> {
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> index d378b276e476..f6828c0276eb 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> @@ -697,11 +697,17 @@ static int pin_context(struct i915_gem_context *ctx,
>>> struct intel_context **out)
>>> {
>>> struct intel_context *ce;
>>> + int err;
>>>
>>> - ce = intel_context_pin(ctx, engine);
>>> + ce = intel_context_instance(ctx, engine);
>>> if (IS_ERR(ce))
>>> return PTR_ERR(ce);
>>>
>>> + err = intel_context_pin(ce);
>>> + intel_context_put(ce);
>>> + if (err)
>>> + return err;
>>> +
>>> *out = ce;
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
>>> index d9f68c89dff4..a79d9909d171 100644
>>> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
>>> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
>>> @@ -239,6 +239,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>>> int id)
>>> {
>>> struct mock_engine *engine;
>>> + int err;
>>>
>>> GEM_BUG_ON(id >= I915_NUM_ENGINES);
>>>
>>> @@ -278,10 +279,15 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>>> INIT_LIST_HEAD(&engine->hw_queue);
>>>
>>> engine->base.kernel_context =
>>> - intel_context_pin(i915->kernel_context, &engine->base);
>>> + intel_context_instance(i915->kernel_context, &engine->base);
>>> if (IS_ERR(engine->base.kernel_context))
>>> goto err_breadcrumbs;
>>>
>>> + err = intel_context_pin(engine->base.kernel_context);
>>> + intel_context_put(engine->base.kernel_context);
>>> + if (err)
>>> + goto err_breadcrumbs;
>>> +
>>> return &engine->base;
>>>
>>> err_breadcrumbs:
>>> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
>>> index 40d9f549a0cd..606fc2713240 100644
>>> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
>>> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
>>> @@ -1188,12 +1188,17 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
>>> INIT_LIST_HEAD(&s->workload_q_head[i]);
>>> s->shadow[i] = ERR_PTR(-EINVAL);
>>>
>>> - ce = intel_context_pin(ctx, engine);
>>> + ce = intel_context_instance(ctx, engine);
>>> if (IS_ERR(ce)) {
>>> ret = PTR_ERR(ce);
>>> goto out_shadow_ctx;
>>> }
>>>
>>> + ret = intel_context_pin(ce);
>>> + intel_context_put(ce);
>>> + if (ret)
>>> + goto out_shadow_ctx;
>>> +
>>> s->shadow[i] = ce;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index 35732c2ae17f..c700cbc2f594 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -2101,14 +2101,19 @@ static int eb_pin_context(struct i915_execbuffer *eb,
>>> if (err)
>>> return err;
>>>
>>> + ce = intel_context_instance(eb->gem_context, engine);
>>> + if (IS_ERR(ce))
>>> + return PTR_ERR(ce);
>>> +
>>> /*
>>> * Pinning the contexts may generate requests in order to acquire
>>> * GGTT space, so do this first before we reserve a seqno for
>>> * ourselves.
>>> */
>>> - ce = intel_context_pin(eb->gem_context, engine);
>>> - if (IS_ERR(ce))
>>> - return PTR_ERR(ce);
>>> + err = intel_context_pin(ce);
>>> + intel_context_put(ce);
>>> + if (err)
>>> + return err;
>>>
>>> eb->engine = engine;
>>> eb->context = ce;
>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>> index 328a740e72cb..afaeabe5e531 100644
>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>> @@ -1205,11 +1205,17 @@ static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
>>> {
>>> struct intel_engine_cs *engine = i915->engine[RCS0];
>>> struct intel_context *ce;
>>> - int ret;
>>> + int err;
>>>
>>> - ret = i915_mutex_lock_interruptible(&i915->drm);
>>> - if (ret)
>>> - return ERR_PTR(ret);
>>> + ce = intel_context_instance(ctx, engine);
>>> + if (IS_ERR(ce))
>>> + return ce;
>>> +
>>> + err = i915_mutex_lock_interruptible(&i915->drm);
>>> + if (err) {
>>> + intel_context_put(ce);
>>> + return ERR_PTR(err);
>>> + }
>>>
>>> /*
>>> * As the ID is the gtt offset of the context's vma we
>>> @@ -1217,10 +1223,11 @@ static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
>>> *
>>> * NB: implied RCS engine...
>>> */
>>> - ce = intel_context_pin(ctx, engine);
>>> + err = intel_context_pin(ce);
>>> mutex_unlock(&i915->drm.struct_mutex);
>>> - if (IS_ERR(ce))
>>> - return ce;
>>> + intel_context_put(ce);
>>> + if (err)
>>> + return ERR_PTR(err);
>>>
>>> i915->perf.oa.pinned_ctx = ce;
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index 7932d1209247..8abd891d9287 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -753,6 +753,7 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>>> struct drm_i915_private *i915 = engine->i915;
>>> struct intel_context *ce;
>>> struct i915_request *rq;
>>> + int err;
>>>
>>> /*
>>> * Preempt contexts are reserved for exclusive use to inject a
>>> @@ -766,13 +767,21 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>>> * GGTT space, so do this first before we reserve a seqno for
>>> * ourselves.
>>> */
>>> - ce = intel_context_pin(ctx, engine);
>>> + ce = intel_context_instance(ctx, engine);
>>> if (IS_ERR(ce))
>>> return ERR_CAST(ce);
>>>
>>> + err = intel_context_pin(ce);
>>> + if (err) {
>>> + rq = ERR_PTR(err);
>>> + goto err_put;
>>> + }
>>> +
>>> rq = i915_request_create(ce);
>>> intel_context_unpin(ce);
>>>
>>> +err_put:
>>> + intel_context_put(ce);
>>> return rq;
>>> }
>>>
>>>
>>
>> The pattern of instance-pin-put is repeated so many times it begs to be
>> promoted to something like intel_context_pin_instance.
>
> It's just a transition patch. We'll get rid of intel_context_instance()
> momentarily.
And haven't I spotted this later myself!
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list