From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Volkin, Bradley D" Subject: Re: [PATCH 26/53] drm/i915/bdw: New logical ring submission mechanism Date: Tue, 24 Jun 2014 07:41:00 -0700 Message-ID: <20140624144100.GA23831@bdvolkin-ubuntu-desktop> References: <20140620210035.GC32083@bdvolkin-ubuntu-desktop> <92648605EABDA246B775AAB04C95A7A3137BEC32@IRSMSX103.ger.corp.intel.com> <20140623131355.GA14360@nuc-i3427.alporthouse.com> <92648605EABDA246B775AAB04C95A7A3137BEC64@IRSMSX103.ger.corp.intel.com> <20140623132719.GB14360@nuc-i3427.alporthouse.com> <92648605EABDA246B775AAB04C95A7A3137BEDF8@IRSMSX103.ger.corp.intel.com> <20140623134140.GC14360@nuc-i3427.alporthouse.com> <92648605EABDA246B775AAB04C95A7A3137BEE76@IRSMSX103.ger.corp.intel.com> <20140624002347.GA31605@bwidawsk.net> <92648605EABDA246B775AAB04C95A7A3137C331B@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 mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id CAE496E2CB for ; Tue, 24 Jun 2014 07:41:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <92648605EABDA246B775AAB04C95A7A3137C331B@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: Ben Widawsky , "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Tue, Jun 24, 2014 at 04:45:05AM -0700, Mateo Lozano, Oscar wrote: > Ok, let=B4s try to extract something positive out of all this. > = > OPTION A (Ben=B4s proposal): > = > > I think the only solution for what Chris is asking for is to implement = this as 1 > > context per engine, as opposed to 1 context with a context object per > > engine. As you correctly stated, I think we all agreed the latter was f= ine when > > we met. Functionally, I see no difference, but it does allow you to alw= ays use > > a context as the sole mechanism for making any decisions and performing > > any operations. Now without writing all the code, I can't promise it ac= tually > > will look better, but I think it's likely going to be a lot cleaner. Be= fore you do > > any changes though... > = > We agreed on this early on (v1), yes, but then the idea was frowned upon = by Brad and then by Daniel. I cannot recall exactly why anymore, but one bi= g reason was that the idr mechanism makes it difficult to track several con= texts with the same id (and userspace only has one context handle) and some= thing about ctx->hang_stats. From v2 on, we agreed to multiplex different e= ngines inside one intel_context (and that=B4s why we renamed i915_hw_contex= t to intel_context). Yeah, at least for me, the reason was that the multiple structs per context= id code felt awkward given that most/all of the fields in a struct intel_conte= xt are logically per-context rather than per-engine (vm, hang_stats, etc). It didn= 't seem like the right approach to me at the time. Brad > = > OPTION B (Brad=B4s proposal): > = > > So I suggested that we: > > = > > - Add a back pointer from struct intel_rinbuffer to intel_context (woul= d only > > be valid for lrc mode) > > - Move the intel_ringbuffer_get(engine, context) calls up to the callers > > - Pass (engine, ringbuf) instead of (engine, context) to intel_ring_* f= unctions > > - Have the vfunc implemenations get the context from the ringbuffer whe= re > > needed and ignore it where not > > = > > Looking again, we could probably add a back pointer to the intel_engine= _cs > > as well and then just pass around the ringbuf. > = > Sounds fine by me: intel_ringbuffer is only related to exactly one intel_= engine_cs and one intel_context, so having pointers to those two makes sens= e. > As before, this could be easily done within the existing code (passing in= tel_rinbgbuffer instead of intel_engine_cs), but Daniel wants a code split,= so I can only do it for the logical ring functions.