From patchwork Tue Apr 19 12:16:44 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tvrtko Ursulin X-Patchwork-Id: 8879241 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 250559F39D for ; Tue, 19 Apr 2016 12:16:51 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id F3AD720272 for ; Tue, 19 Apr 2016 12:16:49 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 8D27620270 for ; Tue, 19 Apr 2016 12:16:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D24626E346; Tue, 19 Apr 2016 12:16:47 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id C53D76E346 for ; Tue, 19 Apr 2016 12:16:45 +0000 (UTC) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 19 Apr 2016 05:16:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,506,1455004800"; d="scan'208";a="961865152" Received: from tursulin-linux.isw.intel.com (HELO [10.102.226.196]) ([10.102.226.196]) by fmsmga002.fm.intel.com with ESMTP; 19 Apr 2016 05:16:44 -0700 To: Chris Wilson , intel-gfx@lists.freedesktop.org References: <1460721275-1001-1-git-send-email-tvrtko.ursulin@linux.intel.com> <1461048560-31983-1-git-send-email-chris@chris-wilson.co.uk> <1461048560-31983-10-git-send-email-chris@chris-wilson.co.uk> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: <571621AC.9030301@linux.intel.com> Date: Tue, 19 Apr 2016 13:16:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1461048560-31983-10-git-send-email-chris@chris-wilson.co.uk> Subject: Re: [Intel-gfx] [PATCH 9/9] drm/i915: Move releasing of the GEM request from free to retire/cancel X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 19/04/16 07:49, 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,v3: Rebalance execlists by moving the context unpinning. > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_gem.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_request.c | 25 ++++++++++--------------- > 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 | 4 ---- > drivers/gpu/drm/i915/intel_pm.c | 2 +- > 7 files changed, 15 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 4057c0febccd..1f5434f7a294 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2542,7 +2542,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; > > @@ -3548,7 +3548,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 8d7c415f1896..c14dca3d8b87 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.c > +++ b/drivers/gpu/drm/i915/i915_gem_request.c > @@ -250,6 +250,7 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) > 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 > @@ -261,9 +262,15 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) > */ > request->ringbuf->last_retired_head = request->postfix; > > - list_del_init(&request->list); > i915_gem_request_remove_from_client(request); > > + if (request->pinned_context) { > + if (i915.enable_execlists) > + intel_lr_context_unpin(request->pinned_context, > + request->engine); > + } > + > + i915_gem_context_unreference(request->ctx); > i915_gem_request_unreference(request); > } > > @@ -636,19 +643,7 @@ int 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 (req->pinned_context) { > - if (i915.enable_execlists) > - intel_lr_context_unpin(req->pinned_context, > - req->engine); > - } > - > - i915_gem_context_unreference(ctx); > + struct drm_i915_gem_request *req = > + container_of(req_ref, typeof(*req), ref); > kmem_cache_free(to_i915(req)->requests, req); > } > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h > index 389813cbc19a..48bff7dc4f90 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.h > +++ b/drivers/gpu/drm/i915/i915_gem_request.h > @@ -170,23 +170,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->engine->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->engine->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 79f175853548..d7d19e17318e 100644 > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c ! :) > @@ -379,7 +379,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 caff656417c2..07dfd55fff91 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11370,7 +11370,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 0e55f206e592..0eaa74fab87b 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -587,7 +587,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request) > struct drm_i915_gem_request *cursor; > int num_elements = 0; > > - intel_lr_context_pin(request->ctx, request->engine); I would prefer this step to be a separate patch from the retire/free reorg. Also, in my testing, another patch was required before this step to make it work. Basically: From 848df37ef90dfb7c7adbf59d155c39d8e4521b8e Mon Sep 17 00:00:00 2001 From: Tvrtko Ursulin Date: Fri, 15 Apr 2016 16:13:26 +0100 Subject: [PATCH 6/7] drm/i915: Store LRC hardware id in the context This way in the following patch we can disconnect requests from contexts. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/intel_lrc.c | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 79c23108cc35..d85c03425c54 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2334,6 +2334,9 @@ struct drm_i915_gem_request { /** Execlists no. of times this request has been sent to the ELSP */ int elsp_submitted; + + /** Execlists context hardware id. */ + unsigned ctx_hw_id; }; struct drm_i915_gem_request * __must_check diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 8f8da1b01458..871f399f84f6 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -475,7 +475,7 @@ execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id) if (!head_req) return 0; - if (unlikely(head_req->ctx->hw_id != request_id)) + if (unlikely(head_req->ctx_hw_id != request_id)) return 0; WARN(head_req->elsp_submitted == 0, "Never submitted head request\n"); @@ -614,6 +614,7 @@ static void execlists_context_queue(struct drm_i915_gem_request *request) } list_add_tail(&request->execlist_link, &engine->execlist_queue); + request->ctx_hw_id = request->ctx->hw_id; if (num_elements == 0) execlists_context_unqueue(engine);