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

[Lucas De Marchi]

On Mon, Jan 23, 2023 at 08:15:16AM -0800, Anusha Srivatsa wrote:
>
>
>> -----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.

ah, ok. I will send a fixup to this patch.

thanks
Lucas De Marchi