On Thu, 06 Jan 2022, Matt Roper <matthew.d.roper@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > > 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
