[Intel-gfx] [PATCH 045/190] drm/i915: Move releasing of the GEM request from free to retire/cancel
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Apr 5 13:42:16 UTC 2016
On 08/03/16 13:15, Tvrtko Ursulin wrote:
>
> On 11/01/16 09:16, Chris Wilson wrote:
>> If we move the release of the GEM request (i.e. decoupling it from the
>> various lists used for client and context tracking) after it is complete
>> (either by the GPU retiring the request, or by the caller cancelling the
>> request), we can remove the requirement that the final unreference of
>> the GEM request need to be under the struct_mutex.
>>
>> v2: Execlists as always is badly asymetric and year old patches still
>> haven't landed to fix it up.
>
> Looks good and pretty standalone to me (depends on i915_gem_request code
> movement only I think). Just one question below.
>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 4 +--
>> drivers/gpu/drm/i915/i915_gem_request.c | 50
>> ++++++++++++++------------------
>> drivers/gpu/drm/i915/i915_gem_request.h | 14 ---------
>> drivers/gpu/drm/i915/intel_breadcrumbs.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>> drivers/gpu/drm/i915/intel_lrc.c | 6 ++--
>> drivers/gpu/drm/i915/intel_pm.c | 2 +-
>> 7 files changed, 30 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 68a25617ca7a..6d8d65304abf 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2502,7 +2502,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void
>> *data, struct drm_file *file)
>> ret = __i915_wait_request(req[i], true,
>> args->timeout_ns > 0 ? &args->timeout_ns :
>> NULL,
>> to_rps_client(file));
>> - i915_gem_request_unreference__unlocked(req[i]);
>> + i915_gem_request_unreference(req[i]);
>> }
>> return ret;
>>
>> @@ -3505,7 +3505,7 @@ i915_gem_ring_throttle(struct drm_device *dev,
>> struct drm_file *file)
>> return 0;
>>
>> ret = __i915_wait_request(target, true, NULL, NULL);
>> - i915_gem_request_unreference__unlocked(target);
>> + i915_gem_request_unreference(target);
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
>> b/drivers/gpu/drm/i915/i915_gem_request.c
>> index b4ede6dd7b20..1c4f4d83a3c2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_request.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
>> @@ -184,13 +184,6 @@ err:
>> return ret;
>> }
>>
>> -void i915_gem_request_cancel(struct drm_i915_gem_request *req)
>> -{
>> - intel_ring_reserved_space_cancel(req->ringbuf);
>> -
>> - i915_gem_request_unreference(req);
>> -}
>> -
>> int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
>> struct drm_file *file)
>> {
>> @@ -235,9 +228,28 @@ i915_gem_request_remove_from_client(struct
>> drm_i915_gem_request *request)
>> request->pid = NULL;
>> }
>>
>> +static void __i915_gem_request_release(struct drm_i915_gem_request
>> *request)
>> +{
>> + i915_gem_request_remove_from_client(request);
>> +
>> + i915_gem_context_unreference(request->ctx);
>> + i915_gem_request_unreference(request);
>> +}
>> +
>> +void i915_gem_request_cancel(struct drm_i915_gem_request *req)
>> +{
>> + intel_ring_reserved_space_cancel(req->ringbuf);
>> + if (i915.enable_execlists) {
>> + if (req->ctx != req->ring->default_context)
>> + intel_lr_context_unpin(req);
>> + }
>> + __i915_gem_request_release(req);
>> +}
>> +
>> static void i915_gem_request_retire(struct drm_i915_gem_request
>> *request)
>> {
>> trace_i915_gem_request_retire(request);
>> + list_del_init(&request->list);
>>
>> /* We know the GPU must have read the request to have
>> * sent us the seqno + interrupt, so use the position
>> @@ -248,11 +260,7 @@ static void i915_gem_request_retire(struct
>> drm_i915_gem_request *request)
>> * completion order.
>> */
>> request->ringbuf->last_retired_head = request->postfix;
>> -
>> - list_del_init(&request->list);
>> - i915_gem_request_remove_from_client(request);
>> -
>> - i915_gem_request_unreference(request);
>> + __i915_gem_request_release(request);
>> }
>>
>> void
>> @@ -639,21 +647,7 @@ i915_wait_request(struct drm_i915_gem_request *req)
>>
>> void i915_gem_request_free(struct kref *req_ref)
>> {
>> - struct drm_i915_gem_request *req = container_of(req_ref,
>> - typeof(*req), ref);
>> - struct intel_context *ctx = req->ctx;
>> -
>> - if (req->file_priv)
>> - i915_gem_request_remove_from_client(req);
>> -
>> - if (ctx) {
>> - if (i915.enable_execlists) {
>> - if (ctx != req->ring->default_context)
>> - intel_lr_context_unpin(req);
>> - }
>> -
>> - i915_gem_context_unreference(ctx);
>> - }
>> -
>> + struct drm_i915_gem_request *req =
>> + container_of(req_ref, typeof(*req), ref);
>> kmem_cache_free(req->i915->requests, req);
>> }
>> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
>> b/drivers/gpu/drm/i915/i915_gem_request.h
>> index d46f22f30b0a..af1b825fce50 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_request.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
>> @@ -154,23 +154,9 @@ i915_gem_request_reference(struct
>> drm_i915_gem_request *req)
>> static inline void
>> i915_gem_request_unreference(struct drm_i915_gem_request *req)
>> {
>> - WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
>> kref_put(&req->ref, i915_gem_request_free);
>> }
>>
>> -static inline void
>> -i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
>> -{
>> - struct drm_device *dev;
>> -
>> - if (!req)
>> - return;
>> -
>> - dev = req->ring->dev;
>> - if (kref_put_mutex(&req->ref, i915_gem_request_free,
>> &dev->struct_mutex))
>> - mutex_unlock(&dev->struct_mutex);
>> -}
>> -
>> static inline void i915_gem_request_assign(struct
>> drm_i915_gem_request **pdst,
>> struct drm_i915_gem_request *src)
>> {
>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> index 0ea01bd6811c..f6731aac7fcf 100644
>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> @@ -390,7 +390,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>> */
>> intel_engine_remove_wait(engine, &signal->wait);
>>
>> - i915_gem_request_unreference__unlocked(signal->request);
>> + i915_gem_request_unreference(signal->request);
>>
>> /* Find the next oldest signal. Note that as we have
>> * not been holding the lock, another client may
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 57c54c9bc82b..32885b8d5c02 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11431,7 +11431,7 @@ static void intel_mmio_flip_work_func(struct
>> work_struct *work)
>> WARN_ON(__i915_wait_request(mmio_flip->req,
>> false, NULL,
>> &mmio_flip->i915->rps.mmioflips));
>> - i915_gem_request_unreference__unlocked(mmio_flip->req);
>> + i915_gem_request_unreference(mmio_flip->req);
>> }
>>
>> /* For framebuffer backed by dmabuf, wait for fence */
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index b634e7d7a92b..7a3069a2beb2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -587,9 +587,6 @@ static int execlists_context_queue(struct
>> drm_i915_gem_request *request)
>> struct drm_i915_gem_request *cursor;
>> int num_elements = 0;
>>
>> - if (request->ctx != ring->default_context)
>> - intel_lr_context_pin(request);
>> -
>
> Since you remove LRC pin from queue, the lifetime is now either:
>
> 1. From request create to cancel.
> 2. From request create to execlist retirement.
>
> Would it be more logical to leave the LRC pin in queue, but remove it
> from request creation instead? That would make the LRC pin lifetime only
> a single possibility, from queue to execlist retire.
I felt was so close in getting rid of execlist_retired_req_queue, using
this patch as a starting point, when I realised this patch does not play
nicely with the GuC. Back to the drawing board. :(
Regards,
Tvrtko
More information about the Intel-gfx
mailing list