[Intel-gfx] [PATCH v2] drm/i915/gem: Don't leak non-persistent requests on changing engines

[Tvrtko Ursulin]

On 05/02/2020 12:13, Chris Wilson wrote:
> If we have a set of active engines marked as being non-persistent, we
> lose track of those if the user replaces those engines with
> I915_CONTEXT_PARAM_ENGINES. As part of our uABI contract is that
> non-persistent requests are terminated if they are no longer being
> tracked by the user's context (in order to prevent a lost request
> causing an untracked and so unstoppable GPU hang), we need to apply the
> same context cancellation upon changing engines.
> 
> Fixes: a0e047156cde ("drm/i915/gem: Make context persistence optional")
> Testcase: XXX
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 52a749691a8d..20f1d3e0221f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1624,11 +1624,18 @@ set_engines(struct i915_gem_context *ctx,
>   
>   replace:
>   	mutex_lock(&ctx->engines_mutex);
> +
> +	/* Flush stale requests off the old engines if required */
> +	if (!i915_gem_context_is_persistent(ctx) ||
> +	    !i915_modparams.enable_hangcheck)
> +		kill_context(ctx);

Is the negative effect of this is legit contexts can't keep submitting 
and changing the map? Only if PREEMPT_TIMEOUT is disabled I think but 
still. Might break legitimate userspace. Not that I offer solutions.. :( 
Banning changing engines once context went non-persistent? That too can 
break someone.

Regards,

Tvrtko

> +
>   	if (args->size)
>   		i915_gem_context_set_user_engines(ctx);
>   	else
>   		i915_gem_context_clear_user_engines(ctx);
>   	set.engines = rcu_replace_pointer(ctx->engines, set.engines, 1);
> +
>   	mutex_unlock(&ctx->engines_mutex);
>   
>   	call_rcu(&set.engines->rcu, free_engines_rcu);
>