Re: [PATCH 05/71] drm/i915/execlists: Disable submission tasklets when rescheduling

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

On 03/05/2018 07:36, Chris Wilson wrote:
> As we reschedule the requests, we do not want the submission tasklet
> running until we finish updating the priority chains. (We start
> rewriting priorities from the oldest, but the dequeue looks at the most
> recent in-flight, so there is a small race condition where dequeue may
> decide that preemption is falsely required.) Combine the tasklet kicking
> from adding a new request with the set-wedge protection so that we only
> have to adjust the preempt-counter once to achieve both goals.
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     | 4 ++--
>   drivers/gpu/drm/i915/i915_request.c | 5 +----
>   2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5ece6ae4bdff..03cd30001b5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -578,10 +578,10 @@ static void __fence_set_priority(struct dma_fence *fence,
>   	rq = to_request(fence);
>   	engine = rq->engine;
>   
> -	rcu_read_lock();
> +	local_bh_disable(); /* RCU serialisation for set-wedged protection */
>   	if (engine->schedule)
>   		engine->schedule(rq, attr);
> -	rcu_read_unlock();
> +	local_bh_enable(); /* kick the tasklets if queues were reprioritised */
>   }
>   
>   static void fence_set_priority(struct dma_fence *fence,
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 75061f9e48eb..0756fafa7f81 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1109,12 +1109,9 @@ void __i915_request_add(struct i915_request *request, bool flush_caches)
>   	 * decide whether to preempt the entire chain so that it is ready to
>   	 * run at the earliest possible convenience.
>   	 */
> -	rcu_read_lock();
> +	local_bh_disable();
>   	if (engine->schedule)
>   		engine->schedule(request, &request->ctx->sched);
> -	rcu_read_unlock();
> -
> -	local_bh_disable();
>   	i915_sw_fence_commit(&request->submit);
>   	local_bh_enable(); /* Kick the execlists tasklet if just scheduled */
>   
> 

AFAICS this doesn't disable tasklets running on other CPUs in parallel, 
on different engines, so they still may see the non-atomic (wrt 
schedule) snapshot of the submission queues. So I am not sure what it 
means. It prevents a tasklet from interrupt the schedule of this request 
- but as I said, I am not sure of the benefit.

Regards,

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