Re: [Intel-gfx] [PATCH 0/6] i915: Simplify mmio handling & add new DG2 shadow table

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/09/2021 06:33, Matt Roper wrote:
> Our uncore MMIO functions for reading/writing registers have become very
> complicated over time.  There's significant macro magic used to generate
> several nearly-identical functions that only really differ in terms of
> which platform-specific shadow register table they should check on write
> operations.  We can significantly simplify our MMIO handlers by storing
> a reference to the current platform's shadow table within the 'struct
> intel_uncore' the same way we already do for forcewake; this allows us
> to consolidate the multiple variants of each 'write' function down to
> just a single 'fwtable' version that gets the shadow table out of the
> uncore struct rather than hardcoding the name of a specific platform's
> table.  We can do similar consolidation on the MMIO read side by
> creating a single-entry forcewake table to replace the open-coded range
> check they had been using previously.
> 
> The final patch of the series adds a new shadow table for DG2; this
> becomes quite clean and simple now, given the refactoring in the first
> five patches.

Tidy and it ends up saving kernel binary size.

However I am undecided yet, because one thing to note is that the trade 
off is source code and kernel text consolidation at the expense of more 
indirect calls at runtime and larger common read/write functions.

To expand, current code generates a bunch of per gen functions but in 
doing so it manages to inline a bunch of checks like NEEDS_FORCE_WAKE 
and BSEARCH (from find_fw_domain) so at runtime each platform mmio 
read/write does not have to do indirect calls to do lookups.

It may matter a lot in the grand scheme of things but this trade off is 
something to note in the cover letter I think.

Regards,

Tvrtko

> Matt Roper (6):
>    drm/i915/uncore: Convert gen6/gen7 read operations to fwtable
>    drm/i915/uncore: Associate shadow table with uncore
>    drm/i915/uncore: Replace gen8 write functions with general fwtable
>    drm/i915/uncore: Drop gen11/gen12 mmio write handlers
>    drm/i915/uncore: Drop gen11 mmio read handlers
>    drm/i915/dg2: Add DG2-specific shadow register table
> 
>   drivers/gpu/drm/i915/intel_uncore.c           | 190 ++++++++++--------
>   drivers/gpu/drm/i915/intel_uncore.h           |   7 +
>   drivers/gpu/drm/i915/selftests/intel_uncore.c |   1 +
>   3 files changed, 110 insertions(+), 88 deletions(-)
>