[PATCH 0/3] Add _PICK_EVEN_RANGES

[PATCH 0/3] Add _PICK_EVEN_RANGES

[Lucas De Marchi]

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,
-- 
Lucas De Marchi <lucas.demarchi@intel.com>

[PATCH 1/3] drm/i915: Add _PICK_EVEN_RANGES()

[Lucas De Marchi]

It's a constant pattern in the driver to need to use 2 ranges of MMIOs
based on port, phy, pll, etc. When that happens, instead of using
_PICK_EVEN(), _PICK() needs to be used.  Using _PICK() is discouraged
due to some reasons like:

	1) It increases the code size since the array is declared in each
	   call site
	2) Developers need to be careful not to incur an out-of-bounds array
	   access
	3) Developers need to be careful that the indexes match the
	   table. For that it may be that the table needs to contain
	   holes, making (1) even worse.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
(cherry picked from commit 55a65ca6e5d8f7f46fe4cf29c76a9f1b4ddef5ce)
---
 drivers/gpu/drm/i915/i915_reg.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3edfbe92c6dd..d157dd693e41 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -126,10 +126,24 @@
 #define _PICK_EVEN(__index, __a, __b) ((__a) + (__index) * ((__b) - (__a)))
 
 /*
- * Given the arbitrary numbers in varargs, pick the 0-based __index'th number.
+ * Like _PICK_EVEN(), but supports 2 ranges of evenly spaced addres offsets. The
+ * @__use_first_range argument selects if the first or second range should be
+ * used. It's usually in the form like ``(pll) < n``, in which ``n`` is the
+ * number of registers in the first range. Example::
  *
- * Always prefer _PICK_EVEN() over this if the numbers are evenly spaced.
+ * #define _FOO_A			0xf000
+ * #define _FOO_B			0xf004
+ * #define _FOO_C			0xf008
+ * #define _SUPER_FOO_A			0xa000
+ * #define _SUPER_FOO_B			0xaf00
+ * #define FOO(x)			_MMIO(_PICK_EVEN_RANGES(x, (x) < 3,	\
+ *					      _FOO_A, _FOO_B,			\
+ *					      _SUPER_FOO_A, _SUPER_FOO_B))
  */
+#define _PICK_EVEN_RANGES(__index, __use_first_range, __a, __b, __c, __d)	\
+	 ((__use_first_range) ? _PICK_EVEN(__index, __a, __b) :			\
+	  _PICK_EVEN(__index, __c, __d))
+
 #define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
 
 /*

-- 
b4 0.10.1

Re: [PATCH 1/3] drm/i915: Add _PICK_EVEN_RANGES()

[Lucas De Marchi]

On Tue, Oct 11, 2022 at 09:51:08PM -0700, Lucas De Marchi wrote:
>It's a constant pattern in the driver to need to use 2 ranges of MMIOs
>based on port, phy, pll, etc. When that happens, instead of using
>_PICK_EVEN(), _PICK() needs to be used.  Using _PICK() is discouraged
>due to some reasons like:
>
>	1) It increases the code size since the array is declared in each
>	   call site
>	2) Developers need to be careful not to incur an out-of-bounds array
>	   access
>	3) Developers need to be careful that the indexes match the
>	   table. For that it may be that the table needs to contain
>	   holes, making (1) even worse.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>(cherry picked from commit 55a65ca6e5d8f7f46fe4cf29c76a9f1b4ddef5ce)
>---
> drivers/gpu/drm/i915/i915_reg.h | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 3edfbe92c6dd..d157dd693e41 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -126,10 +126,24 @@
> #define _PICK_EVEN(__index, __a, __b) ((__a) + (__index) * ((__b) - (__a)))
>
> /*
>- * Given the arbitrary numbers in varargs, pick the 0-based __index'th number.
>+ * Like _PICK_EVEN(), but supports 2 ranges of evenly spaced addres offsets. The
>+ * @__use_first_range argument selects if the first or second range should be
>+ * used. It's usually in the form like ``(pll) < n``, in which ``n`` is the
>+ * number of registers in the first range. Example::
>  *
>- * Always prefer _PICK_EVEN() over this if the numbers are evenly spaced.
>+ * #define _FOO_A			0xf000
>+ * #define _FOO_B			0xf004
>+ * #define _FOO_C			0xf008
>+ * #define _SUPER_FOO_A			0xa000
>+ * #define _SUPER_FOO_B			0xaf00
>+ * #define FOO(x)			_MMIO(_PICK_EVEN_RANGES(x, (x) < 3,	\
>+ *					      _FOO_A, _FOO_B,			\
>+ *					      _SUPER_FOO_A, _SUPER_FOO_B))
>  */
>+#define _PICK_EVEN_RANGES(__index, __use_first_range, __a, __b, __c, __d)	\
>+	 ((__use_first_range) ? _PICK_EVEN(__index, __a, __b) :			\
>+	  _PICK_EVEN(__index, __c, __d))

humn.. I tried to simplify this with a "__use_first_range" but now this
is broken as the index doesn't start on zero for the second range. This
should actually be something like:

#define _PICK_EVEN_RANGES(__index, __c_idx, __a, __b, __c, __d)		\
	 ((__index < __c_idx) ? _PICK_EVEN(__index, __a, __b) :		\
	  _PICK_EVEN((__index) - __c_idx, __c, __d))

Lucas De Marchi

>+
> #define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
>
> /*
>
>-- 
>b4 0.10.1

[PATCH 2/3] drm/i915: Fix coding style on DPLL*_ENABLE defines

[Lucas De Marchi]

Abide by the rules in the top of the header: 2 spaces for bitfield,
prefix offsets with underscore and prefer the use of REG_BIT().

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
(cherry picked from commit c5545ec37a7f5b928f3f6e3993f1f24b9e70ba32)
---
 drivers/gpu/drm/i915/i915_reg.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d157dd693e41..ad8f839046f5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7305,20 +7305,20 @@ enum skl_power_gate {
 							ADLS_DPCLKA_DDIK_SEL_MASK)
 
 /* ICL PLL */
-#define DPLL0_ENABLE		0x46010
-#define DPLL1_ENABLE		0x46014
+#define _DPLL0_ENABLE		0x46010
+#define _DPLL1_ENABLE		0x46014
 #define _ADLS_DPLL2_ENABLE	0x46018
 #define _ADLS_DPLL3_ENABLE	0x46030
-#define  PLL_ENABLE		(1 << 31)
-#define  PLL_LOCK		(1 << 30)
-#define  PLL_POWER_ENABLE	(1 << 27)
-#define  PLL_POWER_STATE	(1 << 26)
-#define ICL_DPLL_ENABLE(pll)	_MMIO_PLL3(pll, DPLL0_ENABLE, DPLL1_ENABLE, \
+#define   PLL_ENABLE		REG_BIT(31)
+#define   PLL_LOCK		REG_BIT(30)
+#define   PLL_POWER_ENABLE	REG_BIT(27)
+#define   PLL_POWER_STATE	REG_BIT(26)
+#define ICL_DPLL_ENABLE(pll)	_MMIO_PLL3(pll, _DPLL0_ENABLE, _DPLL1_ENABLE, \
 					   _ADLS_DPLL2_ENABLE, _ADLS_DPLL3_ENABLE)
 
 #define _DG2_PLL3_ENABLE	0x4601C
 
-#define DG2_PLL_ENABLE(pll) _MMIO_PLL3(pll, DPLL0_ENABLE, DPLL1_ENABLE, \
+#define DG2_PLL_ENABLE(pll) _MMIO_PLL3(pll, _DPLL0_ENABLE, _DPLL1_ENABLE, \
 				       _ADLS_DPLL2_ENABLE, _DG2_PLL3_ENABLE)
 
 #define TBT_PLL_ENABLE		_MMIO(0x46020)
@@ -7327,12 +7327,12 @@ enum skl_power_gate {
 #define _MG_PLL2_ENABLE		0x46034
 #define _MG_PLL3_ENABLE		0x46038
 #define _MG_PLL4_ENABLE		0x4603C
-/* Bits are the same as DPLL0_ENABLE */
+/* Bits are the same as _DPLL0_ENABLE */
 #define MG_PLL_ENABLE(tc_port)	_MMIO_PORT((tc_port), _MG_PLL1_ENABLE, \
 					   _MG_PLL2_ENABLE)
 
 /* DG1 PLL */
-#define DG1_DPLL_ENABLE(pll)    _MMIO_PLL3(pll, DPLL0_ENABLE, DPLL1_ENABLE, \
+#define DG1_DPLL_ENABLE(pll)    _MMIO_PLL3(pll, _DPLL0_ENABLE, _DPLL1_ENABLE, \
 					   _MG_PLL1_ENABLE, _MG_PLL2_ENABLE)
 
 /* ADL-P Type C PLL */

-- 
b4 0.10.1

[PATCH 3/3] drm/i915: Convert pll macros to _PICK_EVEN_RANGES

[Lucas De Marchi]

Avoid the array lookup, converting the PLL macros after ICL to
_PICK_EVEN_RANGES. This provides the following reduction in code size:

	$ size build64/drivers/gpu/drm/i915/i915.o{.old,.new}
	   text    data     bss     dec     hex filename
	3570297  131232    6824 3708353  3895c1 build64/drivers/gpu/drm/i915/i915.o.old
	3569686  131232    6824 3707742  38935e build64/drivers/gpu/drm/i915/i915.o.new

At the same time it's safer, avoiding out-of-bounds array access.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
(cherry picked from commit 592d15e3d72009bfb9f7a933c292510f8564a4cf)
---
 drivers/gpu/drm/i915/i915_reg.h | 59 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ad8f839046f5..df30bcc53489 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7313,13 +7313,15 @@ enum skl_power_gate {
 #define   PLL_LOCK		REG_BIT(30)
 #define   PLL_POWER_ENABLE	REG_BIT(27)
 #define   PLL_POWER_STATE	REG_BIT(26)
-#define ICL_DPLL_ENABLE(pll)	_MMIO_PLL3(pll, _DPLL0_ENABLE, _DPLL1_ENABLE, \
-					   _ADLS_DPLL2_ENABLE, _ADLS_DPLL3_ENABLE)
+#define ICL_DPLL_ENABLE(pll)	_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 3,			\
+							_DPLL0_ENABLE, _DPLL1_ENABLE,	\
+							_ADLS_DPLL3_ENABLE, _ADLS_DPLL3_ENABLE))
 
 #define _DG2_PLL3_ENABLE	0x4601C
 
-#define DG2_PLL_ENABLE(pll) _MMIO_PLL3(pll, _DPLL0_ENABLE, _DPLL1_ENABLE, \
-				       _ADLS_DPLL2_ENABLE, _DG2_PLL3_ENABLE)
+#define DG2_PLL_ENABLE(pll)	_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 3,			\
+							_DPLL0_ENABLE, _DPLL1_ENABLE,	\
+							_DG2_PLL3_ENABLE, _DG2_PLL3_ENABLE))
 
 #define TBT_PLL_ENABLE		_MMIO(0x46020)
 
@@ -7332,8 +7334,9 @@ enum skl_power_gate {
 					   _MG_PLL2_ENABLE)
 
 /* DG1 PLL */
-#define DG1_DPLL_ENABLE(pll)    _MMIO_PLL3(pll, _DPLL0_ENABLE, _DPLL1_ENABLE, \
-					   _MG_PLL1_ENABLE, _MG_PLL2_ENABLE)
+#define DG1_DPLL_ENABLE(pll)    _MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,			\
+							_DPLL0_ENABLE, _DPLL1_ENABLE,	\
+							_MG_PLL1_ENABLE, _MG_PLL2_ENABLE))
 
 /* ADL-P Type C PLL */
 #define PORTTC1_PLL_ENABLE	0x46038
@@ -7393,9 +7396,9 @@ enum skl_power_gate {
 #define _TGL_DPLL0_CFGCR0		0x164284
 #define _TGL_DPLL1_CFGCR0		0x16428C
 #define _TGL_TBTPLL_CFGCR0		0x16429C
-#define TGL_DPLL_CFGCR0(pll)		_MMIO_PLL3(pll, _TGL_DPLL0_CFGCR0, \
-						  _TGL_DPLL1_CFGCR0, \
-						  _TGL_TBTPLL_CFGCR0)
+#define TGL_DPLL_CFGCR0(pll)		_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,		\
+					      _TGL_DPLL0_CFGCR0, _TGL_DPLL1_CFGCR0,	\
+					      _TGL_TBTPLL_CFGCR0, _TGL_TBTPLL_CFGCR0))
 #define RKL_DPLL_CFGCR0(pll)		_MMIO_PLL(pll, _TGL_DPLL0_CFGCR0, \
 						  _TGL_DPLL1_CFGCR0)
 
@@ -7408,40 +7411,36 @@ enum skl_power_gate {
 #define _TGL_DPLL0_CFGCR1		0x164288
 #define _TGL_DPLL1_CFGCR1		0x164290
 #define _TGL_TBTPLL_CFGCR1		0x1642A0
-#define TGL_DPLL_CFGCR1(pll)		_MMIO_PLL3(pll, _TGL_DPLL0_CFGCR1, \
-						   _TGL_DPLL1_CFGCR1, \
-						   _TGL_TBTPLL_CFGCR1)
+#define TGL_DPLL_CFGCR1(pll)		_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,		\
+					      _TGL_DPLL0_CFGCR1, _TGL_DPLL1_CFGCR1,	\
+					      _TGL_TBTPLL_CFGCR1, _TGL_TBTPLL_CFGCR1))
 #define RKL_DPLL_CFGCR1(pll)		_MMIO_PLL(pll, _TGL_DPLL0_CFGCR1, \
 						  _TGL_DPLL1_CFGCR1)
 
 #define _DG1_DPLL2_CFGCR0		0x16C284
 #define _DG1_DPLL3_CFGCR0		0x16C28C
-#define DG1_DPLL_CFGCR0(pll)		_MMIO_PLL3(pll, _TGL_DPLL0_CFGCR0, \
-						   _TGL_DPLL1_CFGCR0, \
-						   _DG1_DPLL2_CFGCR0, \
-						   _DG1_DPLL3_CFGCR0)
+#define DG1_DPLL_CFGCR0(pll)		_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,		\
+					      _TGL_DPLL0_CFGCR0, _TGL_DPLL1_CFGCR0,	\
+					      _DG1_DPLL2_CFGCR0, _DG1_DPLL3_CFGCR0))
 
 #define _DG1_DPLL2_CFGCR1               0x16C288
 #define _DG1_DPLL3_CFGCR1               0x16C290
-#define DG1_DPLL_CFGCR1(pll)            _MMIO_PLL3(pll, _TGL_DPLL0_CFGCR1, \
-						   _TGL_DPLL1_CFGCR1, \
-						   _DG1_DPLL2_CFGCR1, \
-						   _DG1_DPLL3_CFGCR1)
+#define DG1_DPLL_CFGCR1(pll)            _MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,		\
+					      _TGL_DPLL0_CFGCR1, _TGL_DPLL1_CFGCR1,	\
+					      _DG1_DPLL2_CFGCR1, _DG1_DPLL3_CFGCR1))
 
 /* For ADL-S DPLL4_CFGCR0/1 are used to control DPLL2 */
-#define _ADLS_DPLL3_CFGCR0		0x1642C0
 #define _ADLS_DPLL4_CFGCR0		0x164294
-#define ADLS_DPLL_CFGCR0(pll)		_MMIO_PLL3(pll, _TGL_DPLL0_CFGCR0, \
-						   _TGL_DPLL1_CFGCR0, \
-						   _ADLS_DPLL4_CFGCR0, \
-						   _ADLS_DPLL3_CFGCR0)
+#define _ADLS_DPLL3_CFGCR0		0x1642C0
+#define ADLS_DPLL_CFGCR0(pll)		_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,		\
+					      _TGL_DPLL0_CFGCR0, _TGL_DPLL1_CFGCR0,	\
+					      _ADLS_DPLL4_CFGCR0, _ADLS_DPLL3_CFGCR0))
 
-#define _ADLS_DPLL3_CFGCR1		0x1642C4
 #define _ADLS_DPLL4_CFGCR1		0x164298
-#define ADLS_DPLL_CFGCR1(pll)		_MMIO_PLL3(pll, _TGL_DPLL0_CFGCR1, \
-						   _TGL_DPLL1_CFGCR1, \
-						   _ADLS_DPLL4_CFGCR1, \
-						   _ADLS_DPLL3_CFGCR1)
+#define _ADLS_DPLL3_CFGCR1		0x1642C4
+#define ADLS_DPLL_CFGCR1(pll)		_MMIO(_PICK_EVEN_RANGES(pll, (pll) < 2,		\
+					      _TGL_DPLL0_CFGCR1, _TGL_DPLL1_CFGCR1,	\
+					      _ADLS_DPLL4_CFGCR1, _ADLS_DPLL3_CFGCR1))
 
 #define _DKL_PHY1_BASE			0x168000
 #define _DKL_PHY2_BASE			0x169000

-- 
b4 0.10.1

Re: [PATCH 0/3] Add _PICK_EVEN_RANGES

[Jani Nikula]

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/

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [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@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

Re: [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@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

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center