From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mika Kuoppala Subject: [RFC] drm/i915: reference count batch object on requests Date: Mon, 2 Dec 2013 17:31:53 +0200 Message-ID: <1385998313-5783-1-git-send-email-mika.kuoppala@intel.com> References: <1385995666-3541-1-git-send-email-mika.kuoppala@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 7546EFABB1 for ; Mon, 2 Dec 2013 07:32:33 -0800 (PST) In-Reply-To: <1385995666-3541-1-git-send-email-mika.kuoppala@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org Cc: miku@iki.fi List-Id: intel-gfx@lists.freedesktop.org We used to lean on active_list to handle the references to batch objects. But there are useful cases when same, albeit simple, batch can be executing on multiple rings concurrently. For this case the active_list reference count handling is just not enough as batch could be freed by ring A request retirement as it is still running on ring B. Fix this by doing proper batch_obj reference counting. Signed-off-by: Mika Kuoppala Notes: This is a patch which ameliorates the [PATCH] tests/gem_reset_stats: add close-pending-fork Chris wasn't happy about the refcounting as it might hide the true problem. But I haven't been able to find the real culprit, thus the RFC. --- drivers/gpu/drm/i915/i915_gem.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40d9dcf..858538f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2145,13 +2145,12 @@ int __i915_add_request(struct intel_ring_buffer *ring, request->head = request_start; request->tail = request_ring_position; - /* Whilst this request exists, batch_obj will be on the - * active_list, and so will hold the active reference. Only when this - * request is retired will the the batch_obj be moved onto the - * inactive_list and lose its active reference. Hence we do not need - * to explicitly hold another reference here. + /* Active list has one reference but that is not enough as same + * batch_obj can be active on multiple rings */ request->batch_obj = obj; + if (request->batch_obj) + drm_gem_object_reference(&request->batch_obj->base); /* Hold a reference to the current context so that we can inspect * it later in case a hangcheck error event fires. @@ -2340,6 +2339,9 @@ static void i915_gem_free_request(struct drm_i915_gem_request *request) if (request->ctx) i915_gem_context_unreference(request->ctx); + if (request->batch_obj) + drm_gem_object_unreference(&request->batch_obj->base); + kfree(request); } -- 1.7.9.5