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

[Lucas De Marchi]

On Wed, Oct 12, 2022 at 12:05:31PM -0700, Lucas De Marchi wrote:
>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.

Jani, any feedback here or in the possible things to do below? I'd like
to get a sketch of whatever solution we think could be the right
direction during next week.

thanks
Lucas De Marchi

>
>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