From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine Date: Mon, 19 May 2014 17:49:38 +0200 Message-ID: <20140519154938.GT8790@phenom.ffwll.local> References: <20140514132620.GA6515@strange.amr.corp.intel.com> <92648605EABDA246B775AAB04C95A7A3012EEA38@IRSMSX103.ger.corp.intel.com> <20140515205218.GX8790@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A3012EF79A@IRSMSX103.ger.corp.intel.com> <20140519122037.GK8790@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A3012EF898@IRSMSX103.ger.corp.intel.com> <92648605EABDA246B775AAB04C95A7A3012EF990@IRSMSX103.ger.corp.intel.com> <20140519151155.GQ8790@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A3012EFA46@IRSMSX103.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id 286986E0BC for ; Mon, 19 May 2014 08:49:43 -0700 (PDT) Received: by mail-ee0-f44.google.com with SMTP id c41so3716565eek.17 for ; Mon, 19 May 2014 08:49:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <92648605EABDA246B775AAB04C95A7A3012EFA46@IRSMSX103.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Mateo Lozano, Oscar" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Mon, May 19, 2014 at 03:26:09PM +0000, Mateo Lozano, Oscar wrote: > > I think special-casing the i915_gem_context_get function for the default > > context and using private_default_ctx a bit more sounds good. We need to > > adjust the idr allocator a bit though to reserve 0, and a bit of frobbi= ng in the > > context create code. > = > Ok, no problem. I=B4ll send an early patch that uses private_default_ctx > to do the special casing (no functionality change) together with the > other refactoring patches. > = > > Wrt ctx abstraction I think separate functions for execlist/legacy cont= exts > > should be good enough. The lookup/create/destroy logic should carry ove= r. > = > Including the creation of I915_NUM_RINGS contexts per-filp? do you want > that to happen for the legacy case as well? This implies a number of > other changes, like struct intel_engine *last_ring not making sense > anymore and frobbing i915_gem_create_context so that we can create more > than one context with the same ppgtt... Damn, I didn't think this through. ctx->hang_stats already tracks random stuff for logical contexts and we only have one of those for the implicit per-filp default context. Still, allocating backing storage for the big execlist contexts looks really wasteful since a lot of userspace will just use 1 ring (e.g. mesa or opencl). Also we've decided to at least for now support vcs2 implicitly, so I915_NUM_RINGS is kinda already the wrong thing to use (presuming we can indeed switch hw contexts between vcs and vcs2). Otoh adding fake contexts for the legacy case also feels ugly. So maybe we should rename i915_hw_context to i915_context and figure out a sane design for the differences later on. Same option would be to embedded struct i915_context into a i915_legacy_rcs_context and i915_execlist_context structures, but meh. So now I think we should: a) s/i915_hw_context/i915_context/ since its long past a 1:1 relationship with hw b) create a union in i915_context for legacy and execlists contexts. Legacy contexts would just have the single gem bo backing storage needed for rcs, execlists contexts would have an array of rings plus the backing storage (since the ring is just embedded in there). How does this sound? If we decide later on that the union is too ugly and we want a cleaner split, we can do the usual subclassing later on. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch