--------------------------------------------------------------------- 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...) 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). BTW: do you want me to kill private_default_ctx as well? It doesn´t look very useful... _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
