[Intel-gfx] [PATCH 0/1] Start cleaning up register definitions

[Jani Nikula]

On Thu, 06 Jan 2022, Matt Roper <matthew.d.roper at intel.com> wrote:
> Our i915_reg.h file has become a huge unwieldy mess over the years.  A
> lot of definitions don't follow any logical ordering, there's
> unintentional duplication of registers under different names, the coding
> style is very inconsistent, and there's tons of unused definitions
> (nearly a quarter of i915_reg.h is dead code!).  This makes it a pain to
> work with and also needlessly slows down development since any trivial
> change to i915_reg.h forces us to unnecessarily rebuild the entire i915
> driver instead of just the affected are of the code.

Agreed.

> I'd like to start cleaning up the register definitions in a few steps:
>  (1) Eliminate unnused register and bit definitions.  Some of these may
>      get re-added in the future if/when code starts using them but
>      that's fine; we'd prefer to carefully (re)review the register
>      definitions at that time anyway.

I don't agree with this one, though. I'll reply to the actual patch on
details.

I think step 1 should be splitting out the generic register type and
helper macros to a separate file. It's needed by a number of other
headers for i915_reg_t while most of them don't need any actual register
macros, and the ones that do should be refactored to not require them.

Another early step should be splitting out the non-MMIO macros, for
example the IOSF and PUNIT sideband stuff. They don't necessarily fall
to the below categories.

>  (2) Move registers exclusive to the command parser to their own header
>  (3) Move OA registers to their own header
>  (4) Move GT registers to their own header
>  (5) Move display registers to their own header

Agreed.

>  (6) While moving things to new files, take the opportunity to also
>      update to update to a consistent coding style:  consistent
>      indentation, consistent case for hex values, use of
>      REG_BIT/REG_GENMASK, etc.

I think this should be split to separate move and cleanup patches. More
than anything, this is because of review mechanics. It's trivial to
review code movement by comparing diffs where the only difference in
content is +/- at the beginning of the lines. Submit, review, merge,
forget. Switching to REG_BIT/REG_GENMASK etc. needs though, and is
easier done when the diff is limited to those changes. If there are
issues found in review, you don't have to double check the code movement
anymore.

> This will make it easier to find an appropriate place to add new
> registers and should also improve quality of life for developers since
> driver builds will be faster in cases where a register is added/updated
> and only a specific part of the driver needs to be rebuilt.

Agreed. Basically only .c files should include the headers that have
register macros, and only the ones that are actually needed.

> This only includes step (1) above; the other steps will come as
> follow-up patches if there's no concern with the general goal.

While I appreciate and agree with the overall effort and goal, I don't
think this should be the first patch, nor do I think this should be an
indiscriminate automated mass removal of macros. More on that later.


BR,
Jani.

>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>
> Matt Roper (1):
>   drm/i915: Drop unused register definitions
>
>  drivers/gpu/drm/i915/i915_reg.h | 3107 +------------------------------
>  1 file changed, 9 insertions(+), 3098 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center