[Intel-gfx] [PATCH 0/3] Add _PICK_EVEN_RANGES

[Lucas De Marchi]

On Wed, Oct 12, 2022 at 11:51:48AM +0300, Jani Nikula wrote:
>On Tue, 11 Oct 2022, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> Add a new macro, _PICK_EVEN_RANGES, that supports using 2 address
>> ranges. This should cover most of our needs for _MMIO_PLL3 and such.
>> To show what is achieved with the new macro, convert some PLL-related
>> macros to use it instead of _MMIO_PLL3.
>
>While there's nothing particularly wrong about the solution when looked
>at in isolation, I do have pretty strong reservations on the whole.
>
>We have:
>
>1) _PICK_EVEN() used in _PIPE() and friends
>
>2) _PICK() used in _MMIO_PIPE3() and friends
>
>3) ->pipe_offsets[] etc. adjustment used in _MMIO_PIPE2() and friends
>
>4) ->ddi_index[] mapping proposed in [1]
>
>5) _PICK_EVEN_RANGES() proposed here
>
>Originally we only had the first one, when the hardware was
>simpler. Every single addition since then made sense at the time, but if
>we add 4 & 5 to the mix, I think it's just too many options.
>
>I think it's time to take a step back and figure out if there's a more
>generic approach that could be used.

true... I actually see this as replacing most of the uses of _PICK()
and giving and extra benefit of removing the worry we are doing
out-of-bounds array access. It also allows to more easily move ranges
for new platforms, which is my intention here.

So I think that we could have something like this if changing it to
something else means a bigger refactor. Talking about a big refactor, I
still think my series from a few years back would make sense:

drm/i915/display: start description-based ddi initialization
(https://lore.kernel.org/all/20191223195850.25997-1-lucas.demarchi@intel.com/)

I think that got stalled due to initialization in the intel_ddi.c trying
too much to group together the if/else ladder. But the overall intention
of the patch series I believe is still valid today:

	(...) create a table-based initialization approach in
	which I keep the useful indexes for each platform: these indexes work
	similarly to what we have on the pll part. "enum port" is mostly a
	"driver thing" and when all the conversions take place, it would allow
	us to stop using the port as indexes to register or register bits. "enum
	tc_port", "enum phy", etc are not meaningful numbers from the spec POV
	and change with every other platform.

+Bala who apparently is going to a similar approach in the ddi_index
approach.

Other possible approaches hat come to mind (just dumping some thoughts,
with no actual code/poc):

1) Inside display strut we have:

	struct {
		u8 version;
		union {
			struct {
				i915_reg_t foo;
				i915_reg_t bar;
				i915_reg_t bla;
			} v1;
			struct {
				i915_reg_t xyz;
				i915_reg_t ijk;
			} v2;
		}
	} regs;

instead of vesion it could be the "first platform to use it" like we
currently have. Those registers would then be initialized during module
bind and then we stop doing these conversions to map a platform to a
register offset.  It still needs some per-platform change for the
bitfields though.

idea would be then to enforce using the right struct inside the union by
splitting the code in differen compilation units. One platform can
evolve from the other with the same compilation unit as long as it is
backward-compatible, i.e. we can add more registers, change offsets,
etc. But if the HW interface completely changes, it would need to use a
different version.

2) Looking around what other teams do. In mesa the registers are actually
maintained in a xml. Example: gen12.xml

<register name="HIZ_CHICKEN" length="1" num="0x7018">
   <field name="HZ Depth Test LE/GE Optimization Disable" start="13" end="13" type="bool"/>
   <field name="HZ Depth Test LE/GE Optimization Disable Mask" start="29" end="29" type="bool"/>
</register>

In code it's used like this:

reg.HZDepthTestLEGEOptimizationDisable = true;

3) Kind of going in the same direction, but more in the kernel side. Maybe
switching to regmap?


I think one of the things that block this kind of refactors is having to
bring them back to all the previous platforms. Maybe going back only
until HAS_DDI() would be a good approach. Or maybe even spliting it on
DISPLAY_VER == 12?  That might help more radical changes.


Lucas De Marchi

>
>
>BR,
>Jani.
>
>
>[1] https://patchwork.freedesktop.org/series/108833/
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center