Re: [Intel-gfx] [PATCH v2 1/8] drm/i915: Add _PICK_EVEN_2RANGES()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 20 Jan 2023, Lucas De Marchi <lucas.demarchi@intel.com> 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

Would be interesting to see what this does, and whether the compiler has
the smarts to combine these within each file:

-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
+#define _PICK(__index, ...) (((static const u32 []){ __VA_ARGS__ })[__index])

> 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.
>
> Add a variant of _PICK_EVEN() that works with 2 ranges and selects which
> one to use depending on the index value.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg_defs.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
> index be43580a6979..b7ec87464d69 100644
> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> @@ -119,6 +119,34 @@
>   */
>  #define _PICK_EVEN(__index, __a, __b) ((__a) + (__index) * ((__b) - (__a)))
>  
> +/*
> + * Like _PICK_EVEN(), but supports 2 ranges of evenly spaced address offsets.
> + * The first range is used for indexes below @__c_index, and the second
> + * range is used for anything above it. Example::

I'd like this to be clear about which range is used for index ==
__c_index, instead of saying "below" and "above".

No need for the double colon :: because this isn't a kernel-doc comment.

> + *
> + * #define _FOO_A			0xf000
> + * #define _FOO_B			0xf004
> + * #define _FOO_C			0xf008
> + * #define _SUPER_FOO_A			0xa000
> + * #define _SUPER_FOO_B			0xa100
> + * #define FOO(x)			_MMIO(_PICK_EVEN_RANGES(x, 3,		\

The example uses a different name for the macro.

> + *					      _FOO_A, _FOO_B,			\
> + *					      _SUPER_FOO_A, _SUPER_FOO_B))
> + *
> + * This expands to:
> + *	0: 0xf000,
> + *	1: 0xf004,
> + *	2: 0xf008,
> + *	3: 0xa100,

With the above definitions, this would be 3: 0xa000.

> + *	4: 0xa200,
> + *	5: 0xa300,
> + *	...
> + */
> +#define _PICK_EVEN_2RANGES(__index, __c_index, __a, __b, __c, __d)		\
> +	(BUILD_BUG_ON_ZERO(!__is_constexpr(__c_index)) +			\
> +	 ((__index) < (__c_index) ? _PICK_EVEN(__index, __a, __b) :		\
> +				   _PICK_EVEN((__index) - (__c_index), __c, __d)))
> +
>  /*
>   * Given the arbitrary numbers in varargs, pick the 0-based __index'th number.
>   *

-- 
Jani Nikula, Intel Open Source Graphics Center