On Mon, May 19, 2014 at 04:12:26PM +0000, Mateo Lozano, Oscar wrote: > > > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter > > Sent: Monday, May 19, 2014 4:50 PM > > To: Mateo Lozano, Oscar > > Cc: Daniel Vetter; Lespiau, Damien; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Subject: Re: [PATCH 06/50] drm/i915: > > s/intel_ring_buffer/intel_engine > > > > 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 frobbing in the context create code. > > > > > > Ok, no problem. I´ll 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 > > > > contexts should be good enough. The lookup/create/destroy logic should > > carry over. > > > > > > 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? > > Sounds good: other than using a union, my latest patchset already looked like this :) > (and this means bringing back from life the deferred ctx creation idea...) Yeah, it took me a while to see the light here ;-) But if we split up the sw ctx tracking structure a bit from the low-level stuff I think this indeed makes sense. The lazy context allocation simply irked me while we still called the structure a _hw_ context. Which at that point it just isn't any more. > > Summarizing the entire conversation, we have agreed to three refactoring patches: > > A) s/intel_ring_buffer/intel_engine_cs/ > B) Split the ringbuffers and the rings (the ringbuffer lives inside the > ring thanks to a pointer, that will be NULL for the LRC case). > C) s/i915_hw_context/i915_context/ > > Plus an LRC-specific patch: > > D) Introduce one context backing object per engine (using a union that > leaves one backing object for the legacy case and an array of backing > objects and ringbuffers for the LRC case). Yes, ack on this prep-work refactor plan. And in case I didn't mention it yet ofc ack for shoveling all the new execlist stuff into intel_lrc.c. Btw for intel_lrc.c can you please (at the end is probably best) add a patch to add kerneldoc for non-static functions and pull them into the i915 guide? Similar to how we've done it for Brad's cmd parser. I think we need better docs, so best to get started with new features. > BTW: do you want me to kill private_default_ctx as well? It doesn´t look very useful... Yeah, might as well throw this on top. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
