[Intel-gfx] [PATCH 2/6] drm/i915/breadcrumbs: Assert that irqs are disabled as we update the bottom-half

[Tvrtko Ursulin]

On 15/03/2017 18:32, Chris Wilson wrote:
> On Wed, Mar 15, 2017 at 06:20:16PM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/03/2017 14:01, Chris Wilson wrote:
>>> Check that we have disabled irqs before we take the spin_lock around
>>> reassigned the breadcrumbs.irq_wait.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 3f222dee4c25..35529b35a276 100644
>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> @@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
>>> {
>>> 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>>>
>>> +	GEM_BUG_ON(!irqs_disabled());
>>> +
>>> 	spin_lock(&b->irq_lock);
>>> 	GEM_BUG_ON(!b->irq_armed);
>>> +	GEM_BUG_ON(!b->irq_wait);
>>> 	b->irq_wait = to_wait(next);
>>> 	spin_unlock(&b->irq_lock);
>>>
>>> @@ -395,8 +398,10 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>>> 	}
>>>
>>> 	if (first) {
>>> -		spin_lock(&b->irq_lock);
>>> 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
>>> +		GEM_BUG_ON(!irqs_disabled());
>>> +
>>> +		spin_lock(&b->irq_lock);
>>> 		b->irq_wait = wait;
>>> 		/* After assigning ourselves as the new bottom-half, we must
>>> 		 * perform a cursory check to prevent a missed interrupt.
>>>
>>
>> A single GEM_BUG_ON(!irqs_disabled()) at the top of
>> __intel_engine_add_wait might be more logical?
>
> I wanted to associate it with b->irq_lock, that was my thinking.
> b->rb_lock also sadly has to be irqsafe.

That makes sense yes.

> __intel_breadcrumbs_next() also serves remove_wait, did you mean to
> remove the assert there as well?

Yes, I was thinking only one would do it.

> We can safely ignore this patch, it should be catered by lockdep fairly
> well, I was just being paranoid and going through the possible causes
> and documenting my progress.

This also makes sense. I think it's the best option.

Regards,

Tvrtko