Re: [PATCH v2 25/28] drm/i915: Interrupt driven request completion

[John Harrison <John.C.Harrison@Intel.com>]

On 18/11/2014 09:40, Daniel, Thomas wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of John.C.Harrison@Intel.com
>> Sent: Friday, November 14, 2014 12:19 PM
>> To: Intel-GFX@Lists.FreeDesktop.Org
>> Subject: [Intel-gfx] [PATCH v2 25/28] drm/i915: Interrupt driven request
>> completion
>>
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Added a hook to the ring noftification code to process request completion.
>> This means that there is no longer a need to explicitly process request
>> completions every time a request object is tested. Instead, the test code
>> simply becomes 'return req->completed'. Obviously, this only works if ring
>> interrupts are enabled, however, this is already the case for the duration of
>> __wait_request() which is the point where the driver really needs to know.
>>
>> To prevent stale requests floating around indefinitely, the retire work
>> handler also now performs a completion check periodically.
>>
>> For: VIZ-4377
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h |    5 -----
>>   drivers/gpu/drm/i915/i915_gem.c |   10 ++++++++++
>>   drivers/gpu/drm/i915/i915_irq.c |    2 ++
>>   3 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index 8531e0f..66219b5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2072,11 +2072,6 @@ static inline void i915_gem_request_assign(struct
>> drm_i915_gem_request **pdst,  static inline bool
>> i915_gem_request_completed(struct drm_i915_gem_request *req,
>>   					      bool lazy_coherency)
>>   {
>> -	if (req->complete)
>> -		return true;
>> -
>> -	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
>> -
>>   	return req->complete;
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c index edf712b..039dbb8 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1264,6 +1264,11 @@ int __i915_wait_request(struct
>> drm_i915_gem_request *req,
>>   	if (!irq_test_in_progress && WARN_ON(!ring->irq_get(ring)))
>>   		return -ENODEV;
>>
>> +	/* Completion status should be interrupt driven but it is possible
>> +	 * the request popped out before the interrupt was enabled. So do
>> an
>> +	 * explicit check now... */
>> +	i915_gem_complete_requests_ring(req->ring, false);
>> +
>>   	/* Record current time in case interrupted by signal, or wedged */
>>   	trace_i915_gem_request_wait_begin(req);
>>   	before = ktime_get_raw_ns();
>> @@ -2487,6 +2492,10 @@ int __i915_add_request(struct intel_engine_cs
>> *ring,
>>   	list_add_tail(&request->list, &ring->request_list);
>>   	request->file_priv = NULL;
>>
>> +	/* Avoid race condition where the request completes before it has
>> +	 * been added to the list. */
>> +	ring->last_read_seqno = 0;
>> +
>>   	if (file) {
>>   		struct drm_i915_file_private *file_priv = file->driver_priv;
>>
>> @@ -2858,6 +2867,7 @@ i915_gem_retire_requests(struct drm_device
>> *dev)
>>   	int i;
>>
>>   	for_each_ring(ring, dev_priv, i) {
>> +		i915_gem_complete_requests_ring(ring, false);
>>   		i915_gem_retire_requests_ring(ring);
>>   		idle &= list_empty(&ring->request_list);
>>   	}
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c index 198bbc6..4f63966 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -987,6 +987,8 @@ static void notify_ring(struct drm_device *dev,
>>
>>   	trace_i915_gem_request_complete(ring);
>>
>> +	i915_gem_complete_requests_ring(ring, false);
> I915_gem_complete_requests_ring takes the request list lock and iterates over the entire list.  Are you sure this is safe to do during the interrupt handler?
>
> Thomas.
The list is generally fairly small (max tens of items when running a 
desktop with a bunch of OGL apps/benchmarks running concurrently, 
usually single digit counts). Also, the processing is pretty minimal - 
do an integer comparison and set a flag if true. The overhead really is 
not great.

>> +
>>   	wake_up_all(&ring->irq_queue);
>>   	i915_queue_hangcheck(dev);
>>   }
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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