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

[Tvrtko Ursulin]

On 10/12/2021 01:31, dong.y...@intel.com wrote:

From: "Yang, Dong" <dong.y...@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.y...@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));