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

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

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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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?

As a weakly related side note, there is a stale comment mentioning 
b->lock in intel_engine_enable_signalling.

Regards,

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