On Fri, 21 Oct 2022, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote: > 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@xxxxxxxxx> 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. Considering that I basically stalled this but couldn't provide a decision on a concrete better path forward either, Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx> on the original approach here. Needs a rebase, but it doesn't block us from the other ideas later either. Thanks, and sorry, Jani. > > 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@xxxxxxxxx/) >> >>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 -- Jani Nikula, Intel Open Source Graphics Center
