Re: [PATCH v2 08/15] drm/i915: Take a reference whilst processing the signaler request

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

On 22/02/2017 11:46, Chris Wilson wrote:
> The plan in the near-future is to allow requests to be removed from the
> signaler. We can no longer then rely on holding a reference to the
> request for the duration it is in the signaling tree, and instead must
> obtain a reference to the request for the current operation using RCU.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 24 ++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>  2 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index d5bf4c0b2deb..62e6b8181200 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -496,7 +496,11 @@ static int intel_breadcrumbs_signaler(void *arg)
>  		 * need to wait for a new interrupt from the GPU or for
>  		 * a new client.
>  		 */
> -		request = READ_ONCE(b->first_signal);
> +		rcu_read_lock();
> +		request = rcu_dereference(b->first_signal);
> +		if (request)
> +			request = i915_gem_request_get_rcu(request);
> +		rcu_read_unlock();
>  		if (signal_complete(request)) {
>  			local_bh_disable();
>  			dma_fence_signal(&request->fence);
> @@ -515,24 +519,28 @@ static int intel_breadcrumbs_signaler(void *arg)
>  			 * the oldest before picking the next one.
>  			 */
>  			spin_lock_irq(&b->lock);
> -			if (request == b->first_signal) {
> +			if (request == rcu_access_pointer(b->first_signal)) {
>  				struct rb_node *rb =
>  					rb_next(&request->signaling.node);
> -				b->first_signal = rb ? to_signaler(rb) : NULL;
> +				rcu_assign_pointer(b->first_signal,
> +						   rb ? to_signaler(rb) : NULL);
>  			}
>  			rb_erase(&request->signaling.node, &b->signals);
>  			spin_unlock_irq(&b->lock);
>
>  			i915_gem_request_put(request);
>  		} else {
> -			if (kthread_should_stop())
> +			if (kthread_should_stop()) {
> +				GEM_BUG_ON(request);
>  				break;
> +			}
>
>  			schedule();
>
>  			if (kthread_should_park())
>  				kthread_parkme();
>  		}
> +		i915_gem_request_put(request);
>  	} while (1);
>  	__set_current_state(TASK_RUNNING);
>
> @@ -597,7 +605,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
>  	rb_link_node(&request->signaling.node, parent, p);
>  	rb_insert_color(&request->signaling.node, &b->signals);
>  	if (first)
> -		smp_store_mb(b->first_signal, request);
> +		rcu_assign_pointer(b->first_signal, request);
>
>  	spin_unlock(&b->lock);
>
> @@ -670,7 +678,7 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>  	/* The engines should be idle and all requests accounted for! */
>  	WARN_ON(READ_ONCE(b->first_wait));
>  	WARN_ON(!RB_EMPTY_ROOT(&b->waiters));
> -	WARN_ON(READ_ONCE(b->first_signal));
> +	WARN_ON(rcu_access_pointer(b->first_signal));
>  	WARN_ON(!RB_EMPTY_ROOT(&b->signals));
>
>  	if (!IS_ERR_OR_NULL(b->signaler))
> @@ -686,8 +694,8 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
>
>  	spin_lock_irq(&b->lock);
>
> -	if (b->first_wait) {
> -		wake_up_process(b->first_wait->tsk);
> +	if (rcu_access_pointer(b->first_signal)) {
> +		wake_up_process(b->signaler);
>  		busy |= intel_engine_flag(engine);
>  	}
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b91c2c7ef5a6..3fcb9dd19b07 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -242,7 +242,7 @@ struct intel_engine_cs {
>  		struct rb_root signals; /* sorted by retirement */
>  		struct intel_wait *first_wait; /* oldest waiter by retirement */
>  		struct task_struct *signaler; /* used for fence signalling */
> -		struct drm_i915_gem_request *first_signal;
> +		struct drm_i915_gem_request __rcu *first_signal;
>  		struct timer_list fake_irq; /* used after a missed interrupt */
>  		struct timer_list hangcheck; /* detect missed interrupts */
>
>

Looks OK.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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