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

[Srivatsa, Anusha]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Monday, January 23, 2023 3:01 AM
> To: De Marchi, Lucas <lucas.demarchi at intel.com>; Srivatsa, Anusha
> <anusha.srivatsa at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 1/8] drm/i915: Add _PICK_EVEN_2RANGES()
> 
> On Sat, 21 Jan 2023, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
> > On Fri, Jan 20, 2023 at 10:14:19PM -0800, Anusha Srivatsa wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> >>> Of Lucas De Marchi
> >>> Sent: Friday, January 20, 2023 11:35 AM
> >>> To: intel-gfx at lists.freedesktop.org
> >>> Cc: De Marchi, Lucas <lucas.demarchi at intel.com>; dri-
> >>> devel at lists.freedesktop.org
> >>> Subject: [Intel-gfx] [PATCH v2 1/8] drm/i915: Add
> >>> _PICK_EVEN_2RANGES()
> >>>
> >>> 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.
> >>>
> >>> 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 at 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::
> >>> + *
> >>> + * #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,
> >>> 		\
> >>> + *					      _FOO_A, _FOO_B,
> >>> 	\
> >>> + *					      _SUPER_FOO_A, _SUPER_FOO_B))
> >>> + *
> >>> + * This expands to:
> >>> + *	0: 0xf000,
> >>> + *	1: 0xf004,
> >>> + *	2: 0xf008,
> >>> + *	3: 0xa100,
> >>You mean 3:0xa000
> >
> > doesn't really matter. This is an example of register addresses. They
> > don't need to start from 0, it's whatever the hw gives us.
> 
> I think the point is that the example is inconsistent between _SUPER_FOO_A and
> "3: 0xa100".

Yes. It's the inconsistency with _SUPER_FOO_A that I was pointing out.

Anusha
> BR,
> Jani.
> 
> >
> > Lucas De Marchi
> >
> >>
> >>> + *	4: 0xa200,
> >>4:0xa100
> >>
> >>> + *	5: 0xa300,
> >>5:0xa200
> >>
> >>Anusha
> >>> + *	...
> >>> + */
> >>> +#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.
> >>>   *
> >>> --
> >>> 2.39.0
> >>
> 
> --
> Jani Nikula, Intel Open Source Graphics Center