Re: [Intel-gfx] [PATCH] drm/i915/gt: Do not add same i915_request to intel_context twice

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

On 10/12/2021 01:31, dong.yang@intel.com wrote:
> From: "Yang, Dong" <dong.yang@intel.com>
> 
> With unknow race condition, the i915_request will be added

What do you mean with unknown here?

> to intel_context list twice, and result in system panic.
> 
> If node alreay exist then do not add it again.

Note the call chains are under ce->signal_lock and protecting from double add AFAICT:

static void insert_breadcrumb(struct i915_request *rq)
{
...
	if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
		return;
...
	set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);


bool i915_request_enable_breadcrumb(struct i915_request *rq)
{
...
	spin_lock(&ce->signal_lock);
	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
		insert_breadcrumb(rq);
	spin_unlock(&ce->signal_lock);


void i915_request_cancel_breadcrumb(struct i915_request *rq)
{
...
	spin_lock(&ce->signal_lock);
	if (!test_and_clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
		spin_unlock(&ce->signal_lock);
		return;
	}

void intel_context_remove_breadcrumbs(struct intel_context *ce,
				      struct intel_breadcrumbs *b)
{
...
	spin_lock_irqsave(&ce->signal_lock, flags);

	if (list_empty(&ce->signals))
		goto unlock;

	list_for_each_entry_safe(rq, rn, &ce->signals, signal_link) {
		GEM_BUG_ON(!__i915_request_is_complete(rq));
		if (!test_and_clear_bit(I915_FENCE_FLAG_SIGNAL,
					&rq->fence.flags))
			continue;

The last one in signal_irq_work is guarded by the __i915_request_is_complete check.

So I think more context is needed on how you found this may be an issue.

Regards,

Tvrtko

> 
> Signed-off-by: Yang, Dong <dong.yang@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index 209cf265bf74..9c7bc060d2ae 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -387,6 +387,9 @@ static void insert_breadcrumb(struct i915_request *rq)
>   		}
>   	}
>   
> +	if (&rq->signal_link == pos)
> +		return;
> +
>   	i915_request_get(rq);
>   	list_add_rcu(&rq->signal_link, pos);
>   	GEM_BUG_ON(!check_signal_order(ce, rq));
>