[Intel-gfx] [PATCH 1/2] drm/i915: Rename GGTT init functions

[Chris Wilson]

On Thu, Mar 24, 2016 at 04:01:53PM +0000, Dave Gordon wrote:
> On 24/03/16 07:40, Joonas Lahtinen wrote:
> >On ke, 2016-03-23 at 18:02 +0200, Ville Syrjälä wrote:
> >>On Wed, Mar 23, 2016 at 05:54:09PM +0200, Mika Kuoppala wrote:
> >>>
> >>>Ville Syrjälä <ville.syrjala at linux.intel.com> writes:
> >>>
> >>>>
> >>>>[ text/plain ]
> >>>>On Wed, Mar 23, 2016 at 03:00:22PM +0200, Joonas Lahtinen wrote:
> >>>>>
> >>>>>Rename and document the GGTT init functions to give a better
> >>>>>idea of the context where they are called from.
> >>>>>
> >>>>>i915_gem_gtt_init => i915_init_ggtt_hw
> >>>>Seems to me i915_ggtt_init_hw would match existing practices better.
> >>>>
> >>>There is also some gravity towards putting the verb first. In gem
> >>>side atleast.
> >>At least in this case ggtt_init_hw would match ppgtt_init_hw, which
> >>seems like a nice thing.
> >>
> >
> >Right, I have changed the order quite a few times already. If it's
> >i915_init_* (like i915_init_userptr), will be easier to grep.
> >
> >Adding Chris here as we discussed this yesterday. His idea is that
> >logic should be action_feature and object_verb, init_some_thingamagic,
> >vs object_destroy.

I said object_verb[_phase]. On reflection I mean object_verb_adverb

> Reasonable enough, as long as we can tell what's a feature and
> what's an object. A totally RPN scheme would be even clearer, since
> we would then treat features as objects (and actions are verbs),
> yielding:
> 
> i915_userptr_init()
> i915_engine_setup()
> i915_object_destroy()

If those objects exist, yes.  e.g userptr doesn't, but intel_engine does.
Hence i915_gem_init_userptr() and it should have been
intel_render_engine_init (oh well).
 
> and the like. That would require:
> 
> i915_gem_init_global_gtt => i915_gem_ggtt_init
> i915_gem_gtt_init        => i915_ggtt_hw_init
> i915_global_gtt_cleanup  => i915_ggtt_hw_cleanup

Not quite. init_hw is the verb (or rather verb_adverb).

Into that if an object doesn't exist such as stolen or userptr, then that
pattern doesn't hold and we use the parent as the actor (subject) and move
stolen after the verb (so it describes the verb, because to call it the
object would be confusing!).

Along those lines it is why I like i915_init_hw, i915_init_mmio etc over
i915_hw_init, i915_mmio_init as then it clear that the verb is init and
we are describing what phase of init we are in (and that we are
operating at the driver level).

> and
> 
> i915_pggtt_init()
> i915_pggtt_hw_init()
> 
> and perhaps
> 
> i915_context_allocate()
> i915_hw_ctx_init()
> i915_hw_ctx_pin_and_map()
> i915_context_free()
> 
> What do people think counts as "features" in Chris' scheme?

Not features, objects, which is quite easy to define as anything that
has a class^W struct.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre