> -----Original Message----- > From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On Behalf Of > Daniel Vetter > Sent: Monday, May 19, 2014 2:53 PM > To: Mateo Lozano, Oscar > Cc: Lespiau, Damien; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 06/50] drm/i915: > s/intel_ring_buffer/intel_engine > > On Mon, May 19, 2014 at 3:41 PM, Mateo Lozano, Oscar > <oscar.mateo@xxxxxxxxx> 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/whatever > wizzbang thingy. > > > > Ok, so we create I915_NUM_RINGS contexts for the global default contexts, > plus I915_NUM_RINGS contexts for every filp and 1 render context for every > 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 cannot have > ctx->id = DEFAULT_CONTEXT_ID at the same time (I´ll have to special-case > them by using dev_priv->private_default_ctx[RING] to find them). Of course, if > you prefer, I can abstract away most of the functionality in i915_gem_context.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 need 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 patch > 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-case > 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 checking for > the creation of the default context. Nope, we don´t special case the per-filp default context search: it uses an idr_find, same as the others. Actually, I don´t really see why private_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_context.c away as well? _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
