Re: [PATCH 3/6] drm/i915/breadcrumbs: Update bottom-half before marking as complete

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

On 15/03/2017 14:01, Chris Wilson wrote:
> When adding a new request to the breadcrumb rbtree, we mark all those
> requests inside the rbtree that are already completed as complete. This
> wakes those waiters up and allows them to skip the spinlock before
> returning to userspace. If one of those is the current bottom-half, it
> may then overwrite intel_wait as the interrupt handler dereferences it.

Last sentence sounds suspicious. The interrupts are disabled when this 
runs and locking is in place. And since the fix is to move the 
"completed" block after the "first", I wonder what can get overwritten 
by who?

Oh.. __intel_breadcrumbs_finish. But how does re-ordering help? 
Shouldn't the fix be to skip the bottom-half assignment if the 
"complete" loop has processed the waiter getting added?

Regards,

Tvrtko

> Fixes: 56299fb7d904 ("drm/i915: Signal first fence from irq handler if complete")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 34 ++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 35529b35a276..31e7c25013a4 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -381,24 +381,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	rb_link_node(&wait->node, parent, p);
>  	rb_insert_color(&wait->node, &b->waiters);
>
> -	if (completed) {
> -		struct rb_node *next = rb_next(completed);
> -
> -		GEM_BUG_ON(!next && !first);
> -		if (next && next != &wait->node) {
> -			GEM_BUG_ON(first);
> -			__intel_breadcrumbs_next(engine, next);
> -		}
> -
> -		do {
> -			struct intel_wait *crumb = to_wait(completed);
> -			completed = rb_prev(completed);
> -			__intel_breadcrumbs_finish(b, crumb);
> -		} while (completed);
> -	}
> -
>  	if (first) {
> -		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
>  		GEM_BUG_ON(!irqs_disabled());
>
>  		spin_lock(&b->irq_lock);
> @@ -414,6 +397,23 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  		__intel_breadcrumbs_enable_irq(b);
>  		spin_unlock(&b->irq_lock);
>  	}
> +
> +	if (completed) {
> +		struct rb_node *next = rb_next(completed);
> +
> +		GEM_BUG_ON(!next && !first);
> +		if (next && next != &wait->node) {
> +			GEM_BUG_ON(first);
> +			__intel_breadcrumbs_next(engine, next);
> +		}
> +
> +		do {
> +			struct intel_wait *crumb = to_wait(completed);
> +			completed = rb_prev(completed);
> +			__intel_breadcrumbs_finish(b, crumb);
> +		} while (completed);
> +	}
> +
>  	GEM_BUG_ON(!b->irq_wait);
>  	GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx