Re: [PATCH 06/22] drm/i915: Hold a reference to the active HW context

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 18/03/2019 09:51, Chris Wilson wrote:
> For virtual engines, we need to keep the HW context alive while it
> remains in use. For regular HW contexts, they are created and kept alive
> until the end of the GEM context. For simplicity, generalise the
> requirements and keep an active reference to each HW context.

Is there a functional effect from this patch? Later with veng added?

Regards,

Tvrtko


> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c      |  2 +-
>   drivers/gpu/drm/i915/intel_context.c         |  6 ++++++
>   drivers/gpu/drm/i915/intel_context.h         | 11 +++++++++++
>   drivers/gpu/drm/i915/intel_context_types.h   |  6 +++++-
>   drivers/gpu/drm/i915/intel_lrc.c             |  4 +++-
>   drivers/gpu/drm/i915/intel_ringbuffer.c      |  4 +++-
>   drivers/gpu/drm/i915/selftests/mock_engine.c |  7 ++++++-
>   7 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 21208a865380..d776d43707e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -232,7 +232,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
>   	i915_ppgtt_put(ctx->ppgtt);
>   
>   	rbtree_postorder_for_each_entry_safe(it, n, &ctx->hw_contexts, node)
> -		it->ops->destroy(it);
> +		intel_context_put(it);
>   
>   	kfree(ctx->name);
>   	put_pid(ctx->pid);
> diff --git a/drivers/gpu/drm/i915/intel_context.c b/drivers/gpu/drm/i915/intel_context.c
> index 0ab894a058f6..8931e0fee873 100644
> --- a/drivers/gpu/drm/i915/intel_context.c
> +++ b/drivers/gpu/drm/i915/intel_context.c
> @@ -172,6 +172,7 @@ intel_context_pin(struct i915_gem_context *ctx,
>   		list_add(&ce->active_link, &ctx->active_engines);
>   		mutex_unlock(&ctx->mutex);
>   
> +		intel_context_get(ce);
>   		smp_mb__before_atomic(); /* flush pin before it is visible */
>   	}
>   
> @@ -192,6 +193,7 @@ void intel_context_unpin(struct intel_context *ce)
>   		return;
>   
>   	/* We may be called from inside intel_context_pin() to evict another */
> +	intel_context_get(ce);
>   	mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING);
>   
>   	if (likely(atomic_dec_and_test(&ce->pin_count))) {
> @@ -202,9 +204,11 @@ void intel_context_unpin(struct intel_context *ce)
>   		mutex_unlock(&ce->gem_context->mutex);
>   
>   		i915_gem_context_put(ce->gem_context);
> +		intel_context_put(ce);
>   	}
>   
>   	mutex_unlock(&ce->pin_mutex);
> +	intel_context_put(ce);
>   }
>   
>   static void intel_context_retire(struct i915_active_request *active,
> @@ -221,6 +225,8 @@ intel_context_init(struct intel_context *ce,
>   		   struct i915_gem_context *ctx,
>   		   struct intel_engine_cs *engine)
>   {
> +	kref_init(&ce->ref);
> +
>   	ce->gem_context = ctx;
>   	ce->engine = engine;
>   	ce->ops = engine->cops;
> diff --git a/drivers/gpu/drm/i915/intel_context.h b/drivers/gpu/drm/i915/intel_context.h
> index 9546d932406a..ebc861b1a49e 100644
> --- a/drivers/gpu/drm/i915/intel_context.h
> +++ b/drivers/gpu/drm/i915/intel_context.h
> @@ -73,4 +73,15 @@ static inline void __intel_context_pin(struct intel_context *ce)
>   
>   void intel_context_unpin(struct intel_context *ce);
>   
> +static inline struct intel_context *intel_context_get(struct intel_context *ce)
> +{
> +	kref_get(&ce->ref);
> +	return ce;
> +}
> +
> +static inline void intel_context_put(struct intel_context *ce)
> +{
> +	kref_put(&ce->ref, ce->ops->destroy);
> +}
> +
>   #endif /* __INTEL_CONTEXT_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_context_types.h b/drivers/gpu/drm/i915/intel_context_types.h
> index 6dc9b4b9067b..624729a35875 100644
> --- a/drivers/gpu/drm/i915/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/intel_context_types.h
> @@ -7,6 +7,7 @@
>   #ifndef __INTEL_CONTEXT_TYPES__
>   #define __INTEL_CONTEXT_TYPES__
>   
> +#include <linux/kref.h>
>   #include <linux/list.h>
>   #include <linux/mutex.h>
>   #include <linux/rbtree.h>
> @@ -22,7 +23,8 @@ 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);
> +
> +	void (*destroy)(struct kref *kref);
>   };
>   
>   /*
> @@ -36,6 +38,8 @@ struct intel_sseu {
>   };
>   
>   struct intel_context {
> +	struct kref ref;
> +
>   	struct i915_gem_context *gem_context;
>   	struct intel_engine_cs *engine;
>   	struct intel_engine_cs *active;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 13f5545fc1d2..fbf67105f040 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1242,8 +1242,10 @@ static void __execlists_context_fini(struct intel_context *ce)
>   	i915_gem_object_put(ce->state->obj);
>   }
>   
> -static void execlists_context_destroy(struct intel_context *ce)
> +static void execlists_context_destroy(struct kref *kref)
>   {
> +	struct intel_context *ce = container_of(kref, typeof(*ce), ref);
> +
>   	GEM_BUG_ON(intel_context_is_pinned(ce));
>   
>   	if (ce->state)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6d60bc258feb..35fdebd67e5f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1345,8 +1345,10 @@ static void __ring_context_fini(struct intel_context *ce)
>   	i915_gem_object_put(ce->state->obj);
>   }
>   
> -static void ring_context_destroy(struct intel_context *ce)
> +static void ring_context_destroy(struct kref *ref)
>   {
> +	struct intel_context *ce = container_of(ref, typeof(*ce), ref);
> +
>   	GEM_BUG_ON(intel_context_is_pinned(ce));
>   
>   	if (ce->state)
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index 7641b74ada98..639d36eb904a 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -128,12 +128,16 @@ static void mock_context_unpin(struct intel_context *ce)
>   	mock_timeline_unpin(ce->ring->timeline);
>   }
>   
> -static void mock_context_destroy(struct intel_context *ce)
> +static void mock_context_destroy(struct kref *ref)
>   {
> +	struct intel_context *ce = container_of(ref, typeof(*ce), ref);
> +
>   	GEM_BUG_ON(intel_context_is_pinned(ce));
>   
>   	if (ce->ring)
>   		mock_ring_free(ce->ring);
> +
> +	intel_context_free(ce);
>   }
>   
>   static int mock_context_pin(struct intel_context *ce)
> @@ -151,6 +155,7 @@ static int mock_context_pin(struct intel_context *ce)
>   static const struct intel_context_ops mock_context_ops = {
>   	.pin = mock_context_pin,
>   	.unpin = mock_context_unpin,
> +
>   	.destroy = mock_context_destroy,
>   };
>   
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx