Re: [PATCH 01/13] drm/i915: Assert breadcrumbs are correctly ordered in the signal handler

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 01/13] drm/i915: Assert breadcrumbs are correctly ordered in the signal handler
Date: Fri, 3 May 2019 14:49:33 +0100	[thread overview]
Message-ID: <7d7da028-4672-8004-1ce8-d6dd8e76572a@linux.intel.com> (raw)
In-Reply-To: <155689063353.3139.2461740627641851596@skylake-alporthouse-com>

On 03/05/2019 14:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-03 14:32:59)
>>
>> On 03/05/2019 12:52, Chris Wilson wrote:
>>> Inside the signal handler, we expect the requests to be ordered by their
>>> breadcrumb such that no later request may be complete if we find an
>>> earlier incomplete. Add an assert to check that the next breadcrumb
>>> should not be logically before the current.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> index 3cbffd400b1b..a6ffb25f72a2 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> @@ -99,6 +99,12 @@ void intel_engine_breadcrumbs_irq(struct intel_engine_cs *engine)
>>>                        struct i915_request *rq =
>>>                                list_entry(pos, typeof(*rq), signal_link);
>>>    
>>> +                     GEM_BUG_ON(next != &ce->signals &&
>>> +                                i915_seqno_passed(rq->fence.seqno,
>>> +                                                  list_entry(next,
>>> +                                                             typeof(*rq),
>>> +                                                             signal_link)->fence.seqno));
>>
>> I know its only GEM_BUG_ON, but why check for this in the irq handler?
>> You don't trust the insertion, or group deletion? Or just becuase it is
>> the smallest amount of code to piggy-back on existing iteration?
> 
> At this point, it's documenting the required sorting of ce->signals. The
> vulnerable part is list insertion. Would you like something like
> 
> check_signal_order(ce, rq)
> 
> and use it after insertion as well?
> 
> We can do prev/next checking, just to be sure.

I don't feel strongly either way. Was just curious why you decided to 
put it where it was.

Helper I suppose is better since it is more self-documenting and it's 
easy to call it from all strategic places.

Regards,

Tvrtko

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