Re: [PATCH 18/43] drm/i915: Store the intel_context_ops in the intel_engine_cs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/03/2019 14:24, Chris Wilson wrote:
> If we place a pointer to the engine specific intel_context_ops in the
> engine itself, we can assign the ops pointer on initialising the
> context, and then rely on it being set. This simplifies the code in
> later patches.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c      |  1 +
>   drivers/gpu/drm/i915/intel_engine_types.h    |  1 +
>   drivers/gpu/drm/i915/intel_lrc.c             | 13 ++++++------
>   drivers/gpu/drm/i915/intel_ringbuffer.c      | 22 +++++++++-----------
>   drivers/gpu/drm/i915/selftests/mock_engine.c | 13 ++++++------
>   5 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 8037d0efbb4a..bc89d01d338d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -379,6 +379,7 @@ intel_context_init(struct intel_context *ce,
>   {
>   	ce->gem_context = ctx;
>   	ce->engine = engine;
> +	ce->ops = engine->cops;
>   
>   	INIT_LIST_HEAD(&ce->signal_link);
>   	INIT_LIST_HEAD(&ce->signals);
> diff --git a/drivers/gpu/drm/i915/intel_engine_types.h b/drivers/gpu/drm/i915/intel_engine_types.h
> index f7ceda334169..4d2c0ba58e6e 100644
> --- a/drivers/gpu/drm/i915/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/intel_engine_types.h
> @@ -351,6 +351,7 @@ struct intel_engine_cs {
>   
>   	void		(*set_default_submission)(struct intel_engine_cs *engine);
>   
> +	const struct intel_context_ops *cops;
>   	struct intel_context *(*context_pin)(struct intel_engine_cs *engine,
>   					     struct i915_gem_context *ctx);
>   
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0bd837b64ade..31cc4a82b928 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1386,11 +1386,6 @@ __execlists_context_pin(struct intel_engine_cs *engine,
>   	return ERR_PTR(ret);
>   }
>   
> -static const struct intel_context_ops execlists_context_ops = {
> -	.unpin = execlists_context_unpin,
> -	.destroy = execlists_context_destroy,
> -};
> -
>   static struct intel_context *
>   execlists_context_pin(struct intel_engine_cs *engine,
>   		      struct i915_gem_context *ctx)
> @@ -1404,11 +1399,14 @@ execlists_context_pin(struct intel_engine_cs *engine,
>   		return ce;
>   	GEM_BUG_ON(!ce->pin_count); /* no overflow please! */
>   
> -	ce->ops = &execlists_context_ops;
> -
>   	return __execlists_context_pin(engine, ctx, ce);
>   }
>   
> +static const struct intel_context_ops execlists_context_ops = {
> +	.unpin = execlists_context_unpin,
> +	.destroy = execlists_context_destroy,
> +};
> +
>   static int gen8_emit_init_breadcrumb(struct i915_request *rq)
>   {
>   	u32 *cs;
> @@ -2352,6 +2350,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>   	engine->reset.reset = execlists_reset;
>   	engine->reset.finish = execlists_reset_finish;
>   
> +	engine->cops = &execlists_context_ops;
>   	engine->context_pin = execlists_context_pin;
>   	engine->request_alloc = execlists_request_alloc;
>   
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 82f33ff94dc8..78e3c03aab4e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1349,7 +1349,7 @@ intel_ring_free(struct intel_ring *ring)
>   	kfree(ring);
>   }
>   
> -static void intel_ring_context_destroy(struct intel_context *ce)
> +static void ring_context_destroy(struct intel_context *ce)
>   {
>   	GEM_BUG_ON(ce->pin_count);
>   
> @@ -1426,7 +1426,7 @@ static void __context_unpin(struct intel_context *ce)
>   	i915_vma_unpin(vma);
>   }
>   
> -static void intel_ring_context_unpin(struct intel_context *ce)
> +static void ring_context_unpin(struct intel_context *ce)
>   {
>   	__context_unpin_ppgtt(ce->gem_context);
>   	__context_unpin(ce);
> @@ -1549,14 +1549,8 @@ __ring_context_pin(struct intel_engine_cs *engine,
>   	return ERR_PTR(err);
>   }
>   
> -static const struct intel_context_ops ring_context_ops = {
> -	.unpin = intel_ring_context_unpin,
> -	.destroy = intel_ring_context_destroy,
> -};
> -
>   static struct intel_context *
> -intel_ring_context_pin(struct intel_engine_cs *engine,
> -		       struct i915_gem_context *ctx)
> +ring_context_pin(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>   {
>   	struct intel_context *ce = to_intel_context(ctx, engine);
>   
> @@ -1566,11 +1560,14 @@ intel_ring_context_pin(struct intel_engine_cs *engine,
>   		return ce;
>   	GEM_BUG_ON(!ce->pin_count); /* no overflow please! */
>   
> -	ce->ops = &ring_context_ops;
> -
>   	return __ring_context_pin(engine, ctx, ce);
>   }
>   
> +static const struct intel_context_ops ring_context_ops = {
> +	.unpin = ring_context_unpin,
> +	.destroy = ring_context_destroy,
> +};
> +
>   static int intel_init_ring_buffer(struct intel_engine_cs *engine)
>   {
>   	struct i915_timeline *timeline;
> @@ -2276,7 +2273,8 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
>   	engine->reset.reset = reset_ring;
>   	engine->reset.finish = reset_finish;
>   
> -	engine->context_pin = intel_ring_context_pin;
> +	engine->cops = &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 8032a8a9542f..856ec2706f3e 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -137,11 +137,6 @@ static void mock_context_destroy(struct intel_context *ce)
>   		mock_ring_free(ce->ring);
>   }
>   
> -static const struct intel_context_ops mock_context_ops = {
> -	.unpin = mock_context_unpin,
> -	.destroy = mock_context_destroy,
> -};
> -
>   static struct intel_context *
>   mock_context_pin(struct intel_engine_cs *engine,
>   		 struct i915_gem_context *ctx)
> @@ -160,8 +155,6 @@ mock_context_pin(struct intel_engine_cs *engine,
>   
>   	mock_timeline_pin(ce->ring->timeline);
>   
> -	ce->ops = &mock_context_ops;
> -
>   	mutex_lock(&ctx->mutex);
>   	list_add(&ce->active_link, &ctx->active_engines);
>   	mutex_unlock(&ctx->mutex);
> @@ -174,6 +167,11 @@ mock_context_pin(struct intel_engine_cs *engine,
>   	return ERR_PTR(err);
>   }
>   
> +static const struct intel_context_ops mock_context_ops = {
> +	.unpin = mock_context_unpin,
> +	.destroy = mock_context_destroy,
> +};
> +
>   static int mock_request_alloc(struct i915_request *request)
>   {
>   	INIT_LIST_HEAD(&request->mock.link);
> @@ -232,6 +230,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>   	engine->base.mask = BIT(id);
>   	engine->base.status_page.addr = (void *)(engine + 1);
>   
> +	engine->base.cops = &mock_context_ops;
>   	engine->base.context_pin = mock_context_pin;
>   	engine->base.request_alloc = mock_request_alloc;
>   	engine->base.emit_flush = mock_emit_flush;
> 

I thought cops was a joke.. :)))

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx