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

[Lucas De Marchi <lucas.demarchi@intel.com>]

(I missed this review you did before I had sent a v2.1, I will incorporate
what is missing in the next version)

On Mon, Jan 23, 2023 at 12:38:28PM +0200, Jani Nikula wrote:
>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])

if the compiler is smart, it would be at least 1 per compilation unit.
gcc doesn't seem smart enough to even compile it though:

../drivers/gpu/drm/i915/i915_reg_defs.h:155:52: error: expected ‘)’ before ‘{’ token                                                                                                                               
   155 | #define _PICK(__index, ...) (((static const u32 []){ __VA_ARGS__ })[__index])                                                                                                                              
       |                              ~                     ^    

What I'm thinking for the remaining uses of _PICK() is to be explicit
and statically define them in a good .c depending on the use.
Then use that in the macro.

>
>> 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.

ok, what about this?

  * Like _PICK_EVEN(), but supports 2 ranges of evenly spaced address offsets.
  * @__c_index corresponds to the index in which the second range starts
  * to be used. Using math interval notation, the first range is used
  * for indexes [ 0, @__c_index), while the second range is used for
  * [ @__c_index, ... ). 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,		\
>
>The example uses a different name for the macro.

yeah, that was the previous name I was using... good catch, will fix.

>
>> + *					      _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.

fixed


thanks
Lucas De Marchi

>
>> + *	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