[Intel-gfx] [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Monday, May 19, 2014 4:12 PM
> To: Mateo Lozano, Oscar
> Cc: Daniel Vetter; Lespiau, Damien; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915:
> s/intel_ring_buffer/intel_engine
> 
> On Mon, May 19, 2014 at 02:43:05PM +0000, Mateo Lozano, Oscar wrote:
> > > -----Original Message-----
> > > From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On
> > > Behalf Of Daniel Vetter
> > > Sent: Monday, May 19, 2014 2:53 PM
> > > To: Mateo Lozano, Oscar
> > > Cc: Lespiau, Damien; intel-gfx at 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 at 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
> > > ctx->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.

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...