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:11:55 +0200 Message-ID: <20140519151155.GQ8790@phenom.ffwll.local> References: <1399637360-4277-7-git-send-email-oscar.mateo@intel.com> <20140513132827.GF3908@phenom.ffwll.local> <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> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id B0C1F6E779 for ; Mon, 19 May 2014 08:12:00 -0700 (PDT) Received: by mail-ee0-f49.google.com with SMTP id e53so3706066eek.8 for ; Mon, 19 May 2014 08:11:59 -0700 (PDT) Content-Disposition: inline In-Reply-To: <92648605EABDA246B775AAB04C95A7A3012EF990@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 02:43:05PM +0000, Mateo Lozano, Oscar wrote: > > -----Original Message----- > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf = Of > > Daniel Vetter > > Sent: Monday, May 19, 2014 2:53 PM > > To: Mateo Lozano, Oscar > > Cc: Lespiau, Damien; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915: > > s/intel_ring_buffer/intel_engine > > = > > On Mon, May 19, 2014 at 3:41 PM, Mateo Lozano, Oscar > > wrote: > > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > > >> b/drivers/gpu/drm/i915/i915_drv.h index 108e1ec2fa4b..e34db43dead3 > > >> 100644 > > >> --- a/drivers/gpu/drm/i915/i915_drv.h > > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > > >> @@ -1825,7 +1825,9 @@ struct drm_i915_file_private { > > >> } mm; > > >> struct idr context_idr; > > >> > > >> - struct i915_hw_context *private_default_ctx; > > >> + /* default context for each ring, NULL if hw doesn't support hw > > >> contexts > > >> + * (or fancy new lrcs) on that ring. */ > > >> + struct i915_hw_context *private_default_ctx[I915_NUM_RINGS]; > > >> atomic_t rps_wait_boost; > > >> }; > > >> > > >> Of course we need to add an i915_hw_context->engine_cs pointer and we > > >> need to check that at execbuf to make sure we don't run contexts on > > >> the wrong engine. > > >> If we later on decide that we want to expose multiple hw contexts for > > >> !RCS to userspace we can easily add a bunch of ring flags to the > > >> context create ioctl. So this doesn't restrict us at all in the > > >> features we can support without jumping through hoops. > > >> > > >> Also if we'd shovel all per-ring lrcs into the same i915_hw_context > > >> structure then we'd need to rename that and drop the _hw part - it's > > >> no longer a 1:1 correspondence to an actual hw ring/context/lrc/what= ever > > wizzbang thingy. > > > > > > Ok, so we create I915_NUM_RINGS contexts for the global default conte= xts, > > plus I915_NUM_RINGS contexts for every filp and 1 render context for ev= ery > > create ioctl. > > > But the magic stuff is going to pop out in many more places: I cannot > > idr_alloc/idr_find for the per-filp contexts, because all of them canno= t have > > ctx->id =3D DEFAULT_CONTEXT_ID at the same time (I=B4ll have to special= -case > > them by using dev_priv->private_default_ctx[RING] to find them). Of cou= rse, if > > you prefer, I can abstract away most of the functionality in i915_gem_c= ontext.c > > and make sure this kind magic is only done for the LRC path (similar to= what > > you propose to do with intel_ringbuffer.c). > > = > > Argh, forgotten about the pageflips again. But for those we already nee= d some > > other context pointer, and thus far we've only supported ring-switching= on one > > ring (well, almost everywhere at least). Since the mmio base pageflip p= atch > > seems mostly ready I think we could just merge that one first and then = forget > > about ring-based pageflips for execlists. Way too much pain to be worth= it > > really ;-) > = > Sound like a plan :) > = > > For the default context special-casing I've somehow though we special-c= ase > > that in the lookup code. But the code in there is a bit convoluted, so = a bit of > > tidying up (and shoveling more of the checking and lookup logic into > > i915_gem_context.c) can't hurt really. Also we seem to lack error check= ing for > > the creation of the default context. > = > Nope, we don=B4t special case the per-filp default context search: it use= s an idr_find, same as the others. Actually, I don=B4t really see why priva= te_default_ctx is needed at all in the current code? > = > So, for the per-filp default contexts: > = > + struct i915_hw_context *private_default_ctx[I915_NUM_RINGS]; > = > and we special-case the hell out of them? > for legacy and execlists code, or do you want to abstract i915_gem_contex= t.c away as well? 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 frobbing in the context create code. Wrt ctx abstraction I think separate functions for execlist/legacy contexts should be good enough. The lookup/create/destroy logic should carry over. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch