Re: [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine

[Daniel Vetter <daniel@ffwll.ch>]

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
> > <oscar.mateo@intel.com> 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?

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