[01/13] drm/i915: Assert breadcrumbs are correctly ordered in the signal handler
diff mbox series

Message ID 20190503115225.30831-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/13] drm/i915: Assert breadcrumbs are correctly ordered in the signal handler
Related show

Commit Message

Chris Wilson May 3, 2019, 11:52 a.m. UTC
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(+)

Comments

Tvrtko Ursulin May 3, 2019, 1:32 p.m. UTC | #1
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?

Regards,

Tvrtko

> +
>   			if (!__request_completed(rq))
>   				break;
>   
>
Chris Wilson May 3, 2019, 1:37 p.m. UTC | #2
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.
-Chris
Tvrtko Ursulin May 3, 2019, 1:49 p.m. UTC | #3
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

Patch
diff mbox series

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));
+
 			if (!__request_completed(rq))
 				break;