> -----Original Message----- > From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On Behalf Of > Daniel Vetter > Sent: Thursday, June 19, 2014 11:08 AM > To: Mateo Lozano, Oscar > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxx> 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... _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
