[Intel-gfx] [PATCH 09/53] drm/i915/bdw: Initialization for Logical Ring Contexts

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 18, 2014 9:25 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 09/53] drm/i915/bdw: Initialization for
> Logical Ring Contexts
> 
> On Fri, Jun 13, 2014 at 04:37:27PM +0100, oscar.mateo at intel.com wrote:
> > From: Oscar Mateo <oscar.mateo at intel.com>
> >
> > Early in the series we had our own gen8_gem_context_init/fini
> > functions, but the truth is they now look almost the same as the
> > legacy hw context init/fini functions. We can always split them later
> > if this ceases to be the case.
> >
> > Also, we do not fall back to legacy ringbuffers when logical ring
> > context initialization fails (not very likely to happen and, even if
> > it does, hw contexts would probably fail as well).
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 801b891..3f3fb36 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -416,7 +416,13 @@ int i915_gem_context_init(struct drm_device
> *dev)
> >  	if (WARN_ON(dev_priv->ring[RCS].default_context))
> >  		return 0;
> >
> > -	if (HAS_HW_CONTEXTS(dev)) {
> > +	dev_priv->lrc_enabled = intel_enable_execlists(dev);
> > +
> > +	if (dev_priv->lrc_enabled) {
> > +		/* NB: intentionally left blank. We will allocate our own
> > +		 * backing objects as we need them, thank you very much */
> > +		dev_priv->hw_context_size = 0;
> > +	} else if (HAS_HW_CONTEXTS(dev)) {
> >  		dev_priv->hw_context_size =
> round_up(get_context_size(dev), 4096);
> >  		if (dev_priv->hw_context_size > (1<<20)) {
> >  			DRM_DEBUG_DRIVER("Disabling HW Contexts;
> invalid size %d\n", @@
> > -436,7 +442,9 @@ int i915_gem_context_init(struct drm_device *dev)
> >  	for (i = 0; i < I915_NUM_RINGS; i++)
> >  		dev_priv->ring[i].default_context = ctx;
> >
> > -	DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv-
> >hw_context_size ? "HW" : "fake");
> > +	DRM_DEBUG_DRIVER("%s context support initialized\n",
> > +			dev_priv->lrc_enabled ? "LR" :
> > +			dev_priv->hw_context_size ? "HW" : "fake");
> >  	return 0;
> >  }
> >
> > @@ -765,9 +773,12 @@ int i915_switch_context(struct intel_engine_cs
> *ring,
> >  	return do_switch(ring, to);
> >  }
> >
> > -static bool hw_context_enabled(struct drm_device *dev)
> > +static bool contexts_enabled(struct drm_device *dev)
> >  {
> > -	return to_i915(dev)->hw_context_size;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > +	/* FIXME: this would be cleaner with a "context type" enum */
> > +	return dev_priv->lrc_enabled || dev_priv->hw_context_size;
> 
> Since you have a bunch of if ladders the usual approach isn't an enum but a
> vfunc table to abstract behaviour. Think object types instead of switch
> statements. Style bikeshed though (presume code later on doesn't have
> excesses here).
> -Daniel

Hmmmm... I offered to do this with vfuncs early on, but you mentioned special-casing should be enough. And I agreed: at the end, the LR contexts are not that different from traditional HW contexts. This is what I had in mind:

CTX_TYPE_FAKE: no backing objects.
CTX_TYPE_HW: one render backing object at creation time.
CTX_TYPE_LR: n backing objects, with deferred creation. A few functions are moot (e.g. switch, reset).

The current system (looking at dev_priv->hw_context_size to distinguish fake from hw contexts) is imo a bit obfuscated.
And we can always abstract this away with vfuncs if it becomes too complex in the future...

What do you think? can special casing made do for the time being?