| Message ID | 20221011-pick-even-ranges-v1-0-1aaea52752ed@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add _PICK_EVEN_RANGES | expand |
On Tue, 11 Oct 2022, Lucas De Marchi <lucas.demarchi@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. BR, Jani. [1] https://patchwork.freedesktop.org/series/108833/
On Wed, Oct 12, 2022 at 11:51:48AM +0300, Jani Nikula wrote: >On Tue, 11 Oct 2022, Lucas De Marchi <lucas.demarchi@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
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@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
On Fri, 21 Oct 2022, Lucas De Marchi <lucas.demarchi@intel.com> 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@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. 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@intel.com> 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@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
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. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- Lucas De Marchi (3): drm/i915: Add _PICK_EVEN_RANGES() drm/i915: Fix coding style on DPLL*_ENABLE defines drm/i915: Convert pll macros to _PICK_EVEN_RANGES drivers/gpu/drm/i915/i915_reg.h | 91 +++++++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 39 deletions(-) --- base-commit: caaf8c4c270b6b9ce1b8610b4eea888190fc087f change-id: 20221011-pick-even-ranges-76ad8a5007e9 Best regards,