On Fri, Jan 07, 2022 at 12:46:48PM +0200, Jani Nikula wrote:
On Thu, 06 Jan 2022, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
A large percentage (nearly 25%) of the register and bit definitions in
i915_reg.h are not used by the i915 driver and are effectively dead
code. There are a few origins for these unused definitions:
* Registers only accessed by workarounds that were either retracted by
the hardware team or that only applied to pre-production steppings
and have been removed from the driver.
* General refactoring of the codebase (e.g., separate definitions of
REG1, REG2, REG3 being replaced by a parameterized REG(n) macro
without the original definitions being dropped.
* In the early days of i915, all registers in an area of the hardware,
or all bits in a given register got documented in i915_reg.h "just in
case" they needed to be used in the future. With the exploding
complexity of the hardware and huge number of platforms we now
support, mass-documenting everything as #define's doesn't scale.
We'd also generally prefer *not* to have definitions already in the
driver before they're used since we'd rather review them explicitly
alongside new code that is starting to use them.
I still tell people to 1) split out register definitions to separate
patches, and 2) add macros for the entire feature and full contents for
each register, even if they remain unused.
One of the main reasons is review economics. It's pretty easy to look at
a patch adding register contents, review it against the bspec and be
done with it. And while you have the right bspec page open, it only
takes a very short time to add and review the entire register, instead
of having to return to it later. A patch adding just the registers could
get reviewed and merged at v1. It's one less patch the developer has to
keep carrying and rebasing, and it's one less portion the reviewer has
to worry about.
this failed multiple times though, so I'm on the other side of the fence
and think this actually makes things worse. Main reasons is because we
have several registers doing things like:
#define BLA(idx) REG_BIT(idx * 2 + 1)
And it's easy to get this wrong when it was not tested. It may be for
example that one phy or port doesn't follow the same logic.
When I review code I prefer reviewing code people actually tested.
It's less of an issue when it's a 1:1 map from bspec, but for a lot of
registers we need just 1 or 2 bits, e.g. for workarounds. Being able to
filter out workarounds we don't need because we don't even define the
register/bit is also another con to defining the complete register in a
separate patch.
So from my POV, the cons outweigtht the pros.
Overall it's about getting the easy stuff done and behind you first. And
that's a huge part of my whole approach to kernel development, and what
I try to tell others to follow.
I also think the documentation aspect is still valid, and especially so
for older hardware. It may be fine to remove some of the accumulated
cruft, *after* the register macros have been split up to smaller files
by functionality. But I don't think it should be an indiscriminate mass
removal of macros. For example, I don't think I want any of the sideband
or VGA or PCI register macros removed.
Bottom line, I don't mind adding or having unused register macros at
all. Heck, I'd be on board for switching to auto-generated register
macros with absolutely everything.
if it can be generated.... Then we'd have some additional headers for
the accessor functions that deal with index math to the to the right
bits or the right register instance.
Lucas De Marchi
Of course, this is only about register macros that describe the
hardware, not so much about driver implementation where I'd toss unused
code to the curb in a heartbeat.
From a purely pragmatic perspective, please proceed with the
non-controversial splitting up i915_reg.h to individual files, switching
to REG_* macros for contents, etc, etc, first. Just please don't start
with this one because it's going to be a long tedious debate that will
suck the life force right out of us and make you want to stop doing the
whole thing. (Remember, get the easy stuff done and behind you first. ;)
BR,
Jani.
Let's start cleaning up the register definition file by removing a bunch
of dead code. I used a cscope database to track down most of these via
searches like:
for x in $(grep '^#define' drivers/gpu/drm/i915/i915_reg.h | awk '{ print $2 }' | cut -d'(' -f1); do count=$(cscope -d -R -L0 $x | wc -l); if (($count <= 1)); then echo $x; fi; done
although I had to watch out for cases where register/bit names are never
referenced directly, but are constructed elsewhere by macro
concatenation (e.g., GEN3_IRQ_INIT()).
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>
--
Jani Nikula, Intel Open Source Graphics Center
