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

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, June 19, 2014 11:08 AM
> 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 Thu, Jun 19, 2014 at 11:23 AM, Mateo Lozano, Oscar
> <oscar.mateo at intel.com> wrote:
> >> > -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?
> 
> Yeah I generally promote the rule-of-thumb to only do vfuncs once we have
> the 3rd case (and I don't think we should count fake contexts really). Until
> then if-ladders and hacks are good enough. Actually better since usually you
> need a few completely different platforms to know what's required of your
> vfunc intefaces to cover it all.
> 
> I really only latched onto this because of your FIXME comment, no other
> reason at all. So if we decide that some reorg helps the code a lot we can do
> that as a follow-up, but really not required upfront imo.

So green light to the enum idea? It´ll look better than the current dev_priv->hw_context_size + dev_priv->lrc_enabled and I can send it early as prep-work...