Re: [PATCH 19/29] drm/i915: Pass intel_context to intel_context_pin_lock()

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 08/04/2019 10:17, Chris Wilson wrote:
> Move the intel_context_instance() to the caller so that we can decouple
> ourselves from one context instance per engine.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_context.c       | 26 ------
>   drivers/gpu/drm/i915/gt/intel_context.h       | 14 ++-
>   drivers/gpu/drm/i915/i915_gem_context.c       | 88 +++++++++++--------
>   .../gpu/drm/i915/selftests/i915_gem_context.c |  2 +-
>   4 files changed, 64 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index a1267739e369..46bf8d8fb7e4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -128,32 +128,6 @@ intel_context_instance(struct i915_gem_context *ctx,
>   	return intel_context_get(pos);
>   }
>   
> -struct intel_context *
> -intel_context_pin_lock(struct i915_gem_context *ctx,
> -		       struct intel_engine_cs *engine)
> -	__acquires(ce->pin_mutex)
> -{
> -	struct intel_context *ce;
> -
> -	ce = intel_context_instance(ctx, engine);
> -	if (IS_ERR(ce))
> -		return ce;
> -
> -	if (mutex_lock_interruptible(&ce->pin_mutex)) {
> -		intel_context_put(ce);
> -		return ERR_PTR(-EINTR);
> -	}
> -
> -	return ce;
> -}
> -
> -void intel_context_pin_unlock(struct intel_context *ce)
> -	__releases(ce->pin_mutex)
> -{
> -	mutex_unlock(&ce->pin_mutex);
> -	intel_context_put(ce);
> -}
> -
>   int __intel_context_do_pin(struct intel_context *ce)
>   {
>   	int err;
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index da342e9a8c2e..4f8ef45e6344 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -39,9 +39,17 @@ intel_context_lookup(struct i915_gem_context *ctx,
>    * can neither be bound to the GPU or unbound whilst the lock is held, i.e.
>    * intel_context_is_pinned() remains stable.
>    */
> -struct intel_context *
> -intel_context_pin_lock(struct i915_gem_context *ctx,
> -		       struct intel_engine_cs *engine);
> +static inline int intel_context_pin_lock(struct intel_context *ce)
> +	__acquires(ce->pin_mutex)
> +{
> +	return mutex_lock_interruptible(&ce->pin_mutex);
> +}

So if this is not getting the kref any more why are the other callers 
okay? In previous patch(es) some certainly looked like they assume pin 
implies a reference.

> +
> +static inline void intel_context_pin_unlock(struct intel_context *ce)
> +	__releases(ce->pin_mutex)
> +{
> +	mutex_unlock(&ce->pin_mutex);
> +}
>   
>   static inline bool
>   intel_context_is_pinned(struct intel_context *ce)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 51c427ac0c21..1e84fe0a4c55 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -141,6 +141,18 @@ static void lut_close(struct i915_gem_context *ctx)
>   	rcu_read_unlock();
>   }
>   
> +static struct intel_context *
> +lookup_user_engine(struct i915_gem_context *ctx, u16 class, u16 instance)
> +{
> +	struct intel_engine_cs *engine;
> +
> +	engine = intel_engine_lookup_user(ctx->i915, class, instance);
> +	if (!engine)
> +		return ERR_PTR(-EINVAL);
> +
> +	return intel_context_instance(ctx, engine);
> +}
> +
>   static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
>   {
>   	unsigned int max;
> @@ -1142,19 +1154,17 @@ gen8_modify_rpcs(struct intel_context *ce, struct intel_sseu sseu)
>   }
>   
>   static int
> -__i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
> -				    struct intel_engine_cs *engine,
> -				    struct intel_sseu sseu)
> +__intel_context_reconfigure_sseu(struct intel_context *ce,
> +				 struct intel_sseu sseu)
>   {
> -	struct intel_context *ce;
> -	int ret = 0;
> +	int ret;
>   
> -	GEM_BUG_ON(INTEL_GEN(ctx->i915) < 8);
> -	GEM_BUG_ON(engine->id != RCS0);
> +	GEM_BUG_ON(INTEL_GEN(ce->gem_context->i915) < 8);
> +	GEM_BUG_ON(ce->engine->id != RCS0);
>   
> -	ce = intel_context_pin_lock(ctx, engine);
> -	if (IS_ERR(ce))
> -		return PTR_ERR(ce);
> +	ret = intel_context_pin_lock(ce);
> +	if (ret)
> +		return ret;
>   
>   	/* Nothing to do if unmodified. */
>   	if (!memcmp(&ce->sseu, &sseu, sizeof(sseu)))
> @@ -1170,19 +1180,18 @@ __i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
>   }
>   
>   static int
> -i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
> -				  struct intel_engine_cs *engine,
> -				  struct intel_sseu sseu)
> +intel_context_reconfigure_sseu(struct intel_context *ce, struct intel_sseu sseu)
>   {
> +	struct drm_i915_private *i915 = ce->gem_context->i915;
>   	int ret;
>   
> -	ret = mutex_lock_interruptible(&ctx->i915->drm.struct_mutex);
> +	ret = mutex_lock_interruptible(&i915->drm.struct_mutex);
>   	if (ret)
>   		return ret;
>   
> -	ret = __i915_gem_context_reconfigure_sseu(ctx, engine, sseu);
> +	ret = __intel_context_reconfigure_sseu(ce, sseu);
>   
> -	mutex_unlock(&ctx->i915->drm.struct_mutex);
> +	mutex_unlock(&i915->drm.struct_mutex);
>   
>   	return ret;
>   }
> @@ -1290,7 +1299,7 @@ static int set_sseu(struct i915_gem_context *ctx,
>   {
>   	struct drm_i915_private *i915 = ctx->i915;
>   	struct drm_i915_gem_context_param_sseu user_sseu;
> -	struct intel_engine_cs *engine;
> +	struct intel_context *ce;
>   	struct intel_sseu sseu;
>   	int ret;
>   
> @@ -1307,27 +1316,31 @@ static int set_sseu(struct i915_gem_context *ctx,
>   	if (user_sseu.flags || user_sseu.rsvd)
>   		return -EINVAL;
>   
> -	engine = intel_engine_lookup_user(i915,
> -					  user_sseu.engine_class,
> -					  user_sseu.engine_instance);
> -	if (!engine)
> -		return -EINVAL;
> +	ce = lookup_user_engine(ctx,
> +				user_sseu.engine_class,
> +				user_sseu.engine_instance);
> +	if (IS_ERR(ce))
> +		return PTR_ERR(ce);
>   
>   	/* Only render engine supports RPCS configuration. */
> -	if (engine->class != RENDER_CLASS)
> -		return -ENODEV;
> +	if (ce->engine->class != RENDER_CLASS) {
> +		ret = -ENODEV;
> +		goto out_ce;
> +	}
>   
>   	ret = user_to_context_sseu(i915, &user_sseu, &sseu);
>   	if (ret)
> -		return ret;
> +		goto out_ce;
>   
> -	ret = i915_gem_context_reconfigure_sseu(ctx, engine, sseu);
> +	ret = intel_context_reconfigure_sseu(ce, sseu);
>   	if (ret)
> -		return ret;
> +		goto out_ce;
>   
>   	args->size = sizeof(user_sseu);
>   
> -	return 0;
> +out_ce:
> +	intel_context_put(ce);
> +	return ret;
>   }
>   
>   static int ctx_setparam(struct drm_i915_file_private *fpriv,
> @@ -1532,8 +1545,8 @@ static int get_sseu(struct i915_gem_context *ctx,
>   		    struct drm_i915_gem_context_param *args)
>   {
>   	struct drm_i915_gem_context_param_sseu user_sseu;
> -	struct intel_engine_cs *engine;
>   	struct intel_context *ce;
> +	int err;
>   
>   	if (args->size == 0)
>   		goto out;
> @@ -1547,22 +1560,25 @@ static int get_sseu(struct i915_gem_context *ctx,
>   	if (user_sseu.flags || user_sseu.rsvd)
>   		return -EINVAL;
>   
> -	engine = intel_engine_lookup_user(ctx->i915,
> -					  user_sseu.engine_class,
> -					  user_sseu.engine_instance);
> -	if (!engine)
> -		return -EINVAL;
> -
> -	ce = intel_context_pin_lock(ctx, engine); /* serialises with set_sseu */
> +	ce = lookup_user_engine(ctx,
> +				user_sseu.engine_class,
> +				user_sseu.engine_instance);
>   	if (IS_ERR(ce))
>   		return PTR_ERR(ce);
>   
> +	err = intel_context_pin_lock(ce); /* serialises with set_sseu */
> +	if (err) {
> +		intel_context_put(ce);
> +		return err;
> +	}
> +
>   	user_sseu.slice_mask = ce->sseu.slice_mask;
>   	user_sseu.subslice_mask = ce->sseu.subslice_mask;
>   	user_sseu.min_eus_per_subslice = ce->sseu.min_eus_per_subslice;
>   	user_sseu.max_eus_per_subslice = ce->sseu.max_eus_per_subslice;
>   
>   	intel_context_pin_unlock(ce);
> +	intel_context_put(ce);
>   
>   	if (copy_to_user(u64_to_user_ptr(args->value), &user_sseu,
>   			 sizeof(user_sseu)))
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 8e2a94333559..214d1fd2f4dc 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -1017,7 +1017,7 @@ __sseu_test(struct drm_i915_private *i915,
>   	if (ret)
>   		return ret;
>   
> -	ret = __i915_gem_context_reconfigure_sseu(ce->gem_context, ce->engine, sseu);
> +	ret = __intel_context_reconfigure_sseu(ce, sseu);
>   	if (ret)
>   		goto out_spin;
>   

Rest looks good.

Regards,

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