Re: [PATCH 19/27] drm/i915/gem: Use the proto-context to handle create parameters

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 17 May 2021, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, May 17, 2021 at 7:05 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>>
>> On Mon, May 17, 2021 at 8:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>> >
>> > On Fri, May 14, 2021 at 02:13:57PM -0500, Jason Ekstrand wrote:
>> > > I can add those.  I just don't know where to put it.  We don't have an
>> > > i915_gem_vm.h.  Suggestions?
>> >
>> > gt/intel_gtt.h seems to be the header for i915_address_space stuff. Also
>> > contains the i915_vma_ops but not i915_vma.
>> >
>> > It's a pretty good mess, but probably the best place for now for these :-/
>>
>> The one for contexts is in i915_drv.h so I put the VM one there too.
>> Feel free to tell me to move it.  I don't care where it goes.
>
> i915_drv.h is the og dumping ground and needs to die. Everything in
> there needs to be moved out/split/whatever for better code
> organization. If we have a place already that fits better (even if
> maybe misnamed) it's better to put it there.

I haven't really codified this anywhere, but this is what I've been
trying to drive:

* All functions in a .c file are declared in the corresponding .h
  file. 1:1 relationship.

* Have _types.h headers separately for defining types that lead to deep
  include chains. (We have this in part because we have absolutely
  everything in struct drm_i915_private, and everything needs everything
  else to look inside i915.)

* Minimize includes from headers. Prefer forward declarations where
  possible. Prefer specific includes over generic includes.

* Each header is self-contained (this is build-tested with
  CONFIG_DRM_I915_WERROR=y).

* Avoid static inlines unless you have a performance need.

* Don't have any externs. Interfaces over data; data is not an
  interface.

* Prefix functions in a file according to the filename. intel_foo.[ch]
  would have functions intel_foo_*(). Ditto i915_bar.[ch] and
  i915_bar_*(). (Avoid non-static platform specific functions, but if
  you have them, you'd name them e.g. skl_foo_*().)

Basically the rationale is to have more order in the chaos that we've
had for a long time. It's not so much about being pedantic about the
naming, but rather the secondary effect of making people think about
where they put stuff and how it's all grouped together.

IMO it's also easier to add file.[ch] and nuke it later than add stuff
to some of our huge files and then clean it up later.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center