From patchwork Thu Jan 7 12:34:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Gordon X-Patchwork-Id: 7976671 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 39E80BEEE5 for ; Thu, 7 Jan 2016 12:34:45 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 54BB6200FF for ; Thu, 7 Jan 2016 12:34:44 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 5C2C4200ED for ; Thu, 7 Jan 2016 12:34:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 97C988A3A2; Thu, 7 Jan 2016 04:34:42 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTP id 009B28A3A2 for ; Thu, 7 Jan 2016 04:34:40 -0800 (PST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP; 07 Jan 2016 04:34:40 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,532,1444719600"; d="scan'208";a="876370477" Received: from dsgordon-linux2.isw.intel.com (HELO [10.102.226.88]) ([10.102.226.88]) by fmsmga001.fm.intel.com with ESMTP; 07 Jan 2016 04:34:39 -0800 To: Chris Wilson , intel-gfx@lists.freedesktop.org, Jesse Barnes References: <1452162052-22573-1-git-send-email-david.s.gordon@intel.com> <1452162052-22573-2-git-send-email-david.s.gordon@intel.com> <20160107115846.GI652@nuc-i3427.alporthouse.com> From: Dave Gordon Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Message-ID: <568E5B5F.8020109@intel.com> Date: Thu, 7 Jan 2016 12:34:39 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20160107115846.GI652@nuc-i3427.alporthouse.com> Subject: Re: [Intel-gfx] [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests 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=-4.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 07/01/16 11:58, Chris Wilson wrote: > On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote: >> There are a number of places where the driver needs a request, but isn't >> working on behalf of any specific user or in a specific context. At >> present, we associate them with the per-engine default context. A future >> patch will abolish those per-engine context pointers; but we can already >> eliminate a lot of the references to them, just by making the allocator >> allow NULL as a shorthand for "an appropriate context for this ring", >> which will mean that the callers don't need to know anything about how >> the "appropriate context" is found (e.g. per-ring vs per-device, etc). >> >> So this patch renames the existing i915_gem_request_alloc(), and makes >> it local (static inline), and replaces it with a wrapper that provides >> a default if the context is NULL, and also has a nicer calling >> convention (doesn't require a pointer to an output parameter). Then we >> change all callers to use the new convention: >> OLD: >> err = i915_gem_request_alloc(ring, user_ctx, &req); >> if (err) ... >> NEW: >> req = i915_gem_request_alloc(ring, user_ctx); >> if (IS_ERR(req)) ... >> OLD: >> err = i915_gem_request_alloc(ring, ring->default_context, &req); >> if (err) ... >> NEW: >> req = i915_gem_request_alloc(ring, NULL); >> if (IS_ERR(req)) ... > > Nak. You haven't fixed i915_gem_request_alloc() at all. > > http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f > is the patch I have been carrying ever since. > -Chris I think you'll find that the version of i915_gem_request_alloc() I've implemented is equivalent to yours, with the *additional* (and better) semantic of not requiring the caller to specify (ring->default_param) as the context parameter (which is the main point, as far as I'm concerned; making the calling convention nicer was just incidental). *MINE*: ... You seem to have done EXACTLY THE SAME transformation of the function signature (except I remembered to add __must_check, and wrote some kerneldoc for it), so what's not to like? I haven't done anything to {__}i915_gem_object_sync(), but that's a separate issue that you can fix on top of this. .Dave. diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c6dd4db..c2b000a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2260,9 +2260,9 @@ struct drm_i915_gem_request { }; -int i915_gem_request_alloc(struct intel_engine_cs *ring, - struct intel_context *ctx, - struct drm_i915_gem_request **req_out); +struct drm_i915_gem_request * __must_check +i915_gem_request_alloc(struct intel_engine_cs *engine, + struct intel_context *ctx); void i915_gem_request_cancel(struct drm_i915_gem_request *req); void i915_gem_request_free(struct kref *req_ref); int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, ... *YOURS*: diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h index 0869505..2da9e0b 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.h +++ b/drivers/gpu/drm/i915/i915_gem_request.h @@ -118,9 +118,9 @@ struct drm_i915_gem_request { int elsp_submitted; }; -int i915_gem_request_alloc(struct intel_engine_cs *ring, - struct intel_context *ctx, - struct drm_i915_gem_request **req_out); +struct drm_i915_gem_request * +i915_gem_request_alloc(struct intel_engine_cs *ring, + struct intel_context *ctx); void i915_gem_request_cancel(struct drm_i915_gem_request *req); int i915_gem_request_add_to_client(struct drm_i915_gem_request *req, struct drm_file *file);