As we try to reduce our i915-only helpers, let's try to improve
IS_ACTIVE() logic and move to kconfig.h.
I'm not 100% happy with the name, but it's the best I could come up
with, hopefully a little better than trying to add IS_ACTIVE() to be
used broadly.
v2: now with Cc/To list fixed up - no changes to the patches.
Lucas De Marchi (3):
drm/i915: rename IS_ACTIVE
drm/i915/utils: do not depend on config being defined
Move IS_CONFIG_NONZERO() to kconfig.h
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
drivers/gpu/drm/i915/gt/intel_engine.h | 4 ++--
drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 2 +-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +-
.../gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
.../gpu/drm/i915/gt/selftest_engine_heartbeat.c | 4 ++--
drivers/gpu/drm/i915/gt/selftest_execlists.c | 14 +++++++-------
drivers/gpu/drm/i915/i915_config.c | 2 +-
drivers/gpu/drm/i915/i915_request.c | 2 +-
drivers/gpu/drm/i915/i915_utils.h | 13 -------------
include/linux/kconfig.h | 16 ++++++++++++++--
12 files changed, 32 insertions(+), 33 deletions(-)
--
2.33.0
Like the IS_ENABLED() counterpart, we can make IS_CONFIG_NONZERO() to
return the right thing when the config is not defined rather than a
build error, with the limitation that it can't be used on preprocessor
context.
The trick here is that macro names can't start with a number or dash, so
we stringify the argument and check that the first char is a number != 0
(or starting with a dash to cover negative numbers). Except for -O0
builds the strings are all eliminated.
Taking CONFIG_DRM_I915_REQUEST_TIMEOUT in
drivers/gpu/drm/i915/gem/i915_gem_context.c as example, we have the
following output of the preprocessor:
old:
if (((20000) != 0) &&
new:
if (( ("20000"[0] > '0' && "20000"[0] < '9') || "20000"[0] == '-' ) &&
New one looks worse, but is also eliminated from the object:
$ size drivers/gpu/drm/i915/gem/i915_gem_context.o.*
text data bss dec hex filename
52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.new
52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.old
Signed-off-by: Lucas De Marchi <[email protected]>
---
drivers/gpu/drm/i915/i915_utils.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 02bbfa4d68d3..436ce612c46a 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -28,6 +28,7 @@
#include <linux/list.h>
#include <linux/overflow.h>
#include <linux/sched.h>
+#include <linux/stringify.h>
#include <linux/types.h>
#include <linux/workqueue.h>
@@ -469,6 +470,9 @@ static inline bool timer_expired(const struct timer_list *t)
*
* Returns 0 if @config is 0, 1 if set to any value.
*/
-#define IS_CONFIG_NONZERO(config) ((config) != 0)
+#define IS_CONFIG_NONZERO(config) ( \
+ (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \
+ __stringify_1(config)[0] == '-' \
+)
#endif /* !__I915_UTILS_H */
--
2.33.0
W dniu 29.09.2021 o 20:33, Lucas De Marchi pisze:
> Like the IS_ENABLED() counterpart, we can make IS_CONFIG_NONZERO() to
> return the right thing when the config is not defined rather than a
> build error, with the limitation that it can't be used on preprocessor
> context.
>
> The trick here is that macro names can't start with a number or dash, so
> we stringify the argument and check that the first char is a number != 0
> (or starting with a dash to cover negative numbers). Except for -O0
> builds the strings are all eliminated.
>
> Taking CONFIG_DRM_I915_REQUEST_TIMEOUT in
> drivers/gpu/drm/i915/gem/i915_gem_context.c as example, we have the
> following output of the preprocessor:
>
> old:
> if (((20000) != 0) &&
> new:
> if (( ("20000"[0] > '0' && "20000"[0] < '9') || "20000"[0] == '-' ) &&
>
> New one looks worse, but is also eliminated from the object:
>
> $ size drivers/gpu/drm/i915/gem/i915_gem_context.o.*
> text data bss dec hex filename
> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.new
> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.old
>
> Signed-off-by: Lucas De Marchi <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_utils.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 02bbfa4d68d3..436ce612c46a 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -28,6 +28,7 @@
> #include <linux/list.h>
> #include <linux/overflow.h>
> #include <linux/sched.h>
> +#include <linux/stringify.h>
> #include <linux/types.h>
> #include <linux/workqueue.h>
>
> @@ -469,6 +470,9 @@ static inline bool timer_expired(const struct timer_list *t)
> *
> * Returns 0 if @config is 0, 1 if set to any value.
> */
> -#define IS_CONFIG_NONZERO(config) ((config) != 0)
> +#define IS_CONFIG_NONZERO(config) ( \
> + (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \
> + __stringify_1(config)[0] == '-' \
> +)
Quite clever trick, but I see two issues:
- gcc < 8.1 treats expressions with string indices (ex. "abc"[0]) as
non-constant expressions, so they cannot be used everywhere, for example
in global variable initializations,
- it does not work with hex (0x1) or octal values (01)
It is probably OK for private macro, but it can hurt in kconfig.h,
especially the 2nd issue
Regards
Andrzej
>
> #endif /* !__I915_UTILS_H */
On Wed, Sep 29, 2021 at 11:08:18PM +0200, Andrzej Hajda wrote:
>
>W dniu 29.09.2021 o?20:33, Lucas De Marchi pisze:
>> Like the IS_ENABLED() counterpart, we can make IS_CONFIG_NONZERO() to
>> return the right thing when the config is not defined rather than a
>> build error, with the limitation that it can't be used on preprocessor
>> context.
>>
>> The trick here is that macro names can't start with a number or dash, so
>> we stringify the argument and check that the first char is a number != 0
>> (or starting with a dash to cover negative numbers). Except for -O0
>> builds the strings are all eliminated.
>>
>> Taking CONFIG_DRM_I915_REQUEST_TIMEOUT in
>> drivers/gpu/drm/i915/gem/i915_gem_context.c as example, we have the
>> following output of the preprocessor:
>>
>> old:
>> if (((20000) != 0) &&
>> new:
>> if (( ("20000"[0] > '0' && "20000"[0] < '9') || "20000"[0] == '-' ) &&
>>
>> New one looks worse, but is also eliminated from the object:
>>
>> $ size drivers/gpu/drm/i915/gem/i915_gem_context.o.*
>> text data bss dec hex filename
>> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.new
>> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.old
>>
>> Signed-off-by: Lucas De Marchi <[email protected]>
>> ---
>> drivers/gpu/drm/i915/i915_utils.h | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
>> index 02bbfa4d68d3..436ce612c46a 100644
>> --- a/drivers/gpu/drm/i915/i915_utils.h
>> +++ b/drivers/gpu/drm/i915/i915_utils.h
>> @@ -28,6 +28,7 @@
>> #include <linux/list.h>
>> #include <linux/overflow.h>
>> #include <linux/sched.h>
>> +#include <linux/stringify.h>
>> #include <linux/types.h>
>> #include <linux/workqueue.h>
>>
>> @@ -469,6 +470,9 @@ static inline bool timer_expired(const struct timer_list *t)
>> *
>> * Returns 0 if @config is 0, 1 if set to any value.
>> */
>> -#define IS_CONFIG_NONZERO(config) ((config) != 0)
>> +#define IS_CONFIG_NONZERO(config) ( \
>> + (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \
>> + __stringify_1(config)[0] == '-' \
>> +)
>
>
>Quite clever trick, but I see two issues:
>
>- gcc < 8.1 treats expressions with string indices (ex. "abc"[0]) as
>non-constant expressions, so they cannot be used everywhere, for example
>in global variable initializations,
ugh, that would kill the idea - having the strings and additional
runtime checks would not be good. Maybe if we check with
__builtin_constant_p() and do the simpler expansion if it's not
constant?
>
>- it does not work with hex (0x1) or octal values (01)
indeed, but I guess that would be fixable by checking (s[0] == '0' && s[1] == '\0')?
However, it seems kconfig doesn't support setting int options to hex or
octal.
If I try an hex value in menuconfig it says "You have made an invalid entry."
If I try editing .config or setting via scripts/config --set-val, it
just gets reset when trying to generate include/generated/autoconf.h
Lucas De Marchi
>
>It is probably OK for private macro, but it can hurt in kconfig.h,
>especially the 2nd issue
>
>
>Regards
>
>Andrzej
>
>>
>> #endif /* !__I915_UTILS_H */
W dniu 30.09.2021 o 00:54, Lucas De Marchi pisze:
> On Wed, Sep 29, 2021 at 11:08:18PM +0200, Andrzej Hajda wrote:
>>
>> W dniu 29.09.2021 o 20:33, Lucas De Marchi pisze:
>>> Like the IS_ENABLED() counterpart, we can make IS_CONFIG_NONZERO() to
>>> return the right thing when the config is not defined rather than a
>>> build error, with the limitation that it can't be used on preprocessor
>>> context.
>>>
>>> The trick here is that macro names can't start with a number or
>>> dash, so
>>> we stringify the argument and check that the first char is a number
>>> != 0
>>> (or starting with a dash to cover negative numbers). Except for -O0
>>> builds the strings are all eliminated.
>>>
>>> Taking CONFIG_DRM_I915_REQUEST_TIMEOUT in
>>> drivers/gpu/drm/i915/gem/i915_gem_context.c as example, we have the
>>> following output of the preprocessor:
>>>
>>> old:
>>> if (((20000) != 0) &&
>>> new:
>>> if (( ("20000"[0] > '0' && "20000"[0] < '9') || "20000"[0] == '-'
>>> ) &&
>>>
>>> New one looks worse, but is also eliminated from the object:
>>>
>>> $ size drivers/gpu/drm/i915/gem/i915_gem_context.o.*
>>> text data bss dec hex filename
>>> 52021 1070 232 53323 d04b
>>> drivers/gpu/drm/i915/gem/i915_gem_context.o.new
>>> 52021 1070 232 53323 d04b
>>> drivers/gpu/drm/i915/gem/i915_gem_context.o.old
>>>
>>> Signed-off-by: Lucas De Marchi <[email protected]>
>>> ---
>>> drivers/gpu/drm/i915/i915_utils.h | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_utils.h
>>> b/drivers/gpu/drm/i915/i915_utils.h
>>> index 02bbfa4d68d3..436ce612c46a 100644
>>> --- a/drivers/gpu/drm/i915/i915_utils.h
>>> +++ b/drivers/gpu/drm/i915/i915_utils.h
>>> @@ -28,6 +28,7 @@
>>> #include <linux/list.h>
>>> #include <linux/overflow.h>
>>> #include <linux/sched.h>
>>> +#include <linux/stringify.h>
>>> #include <linux/types.h>
>>> #include <linux/workqueue.h>
>>>
>>> @@ -469,6 +470,9 @@ static inline bool timer_expired(const struct
>>> timer_list *t)
>>> *
>>> * Returns 0 if @config is 0, 1 if set to any value.
>>> */
>>> -#define IS_CONFIG_NONZERO(config) ((config) != 0)
>>> +#define IS_CONFIG_NONZERO(config) ( \
>>> + (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] <
>>> '9') || \
>>> + __stringify_1(config)[0] == '-' \
>>> +)
>>
>>
>> Quite clever trick, but I see two issues:
>>
>> - gcc < 8.1 treats expressions with string indices (ex. "abc"[0]) as
>> non-constant expressions, so they cannot be used everywhere, for example
>> in global variable initializations,
>
> ugh, that would kill the idea - having the strings and additional
> runtime checks would not be good. Maybe if we check with
> __builtin_constant_p() and do the simpler expansion if it's not
> constant?
I think it is just matter of disallowing such construct in places where
compiler expects constant expression.
If accepted, the expression is apparently evaluated in compile time. See
[1].
[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69960#c18
>
>>
>> - it does not work with hex (0x1) or octal values (01)
>
> indeed, but I guess that would be fixable by checking (s[0] == '0' &&
> s[1] == '\0')?
> However, it seems kconfig doesn't support setting int options to hex or
> octal.
I've spotted in include/generated/autoconf.h following line:
#define CONFIG_ILLEGAL_POINTER_VALUE 0xdead000000000000
It corresponds to following kconfig entry:
config ILLEGAL_POINTER_VALUE
hex
default 0 if X86_32
default 0xdead000000000000 if X86_64
Grepping shows more: grep -r --include=Kconfig -3 -P '^\s*hex' .
Anyway do you really need to handle undefined case? If not, the macro
can stay simple, w/o hacky constructs.
Regards
Andrzej
>
> If I try an hex value in menuconfig it says "You have made an invalid
> entry."
> If I try editing .config or setting via scripts/config --set-val, it
> just gets reset when trying to generate include/generated/autoconf.h
>
> Lucas De Marchi
>
>>
>> It is probably OK for private macro, but it can hurt in kconfig.h,
>> especially the 2nd issue
>>
>>
>> Regards
>>
>> Andrzej
>>
>>>
>>> #endif /* !__I915_UTILS_H */
>
On 29/09/2021 19:33, Lucas De Marchi wrote:
> Like the IS_ENABLED() counterpart, we can make IS_CONFIG_NONZERO() to
> return the right thing when the config is not defined rather than a
> build error, with the limitation that it can't be used on preprocessor
> context.
>
> The trick here is that macro names can't start with a number or dash, so
> we stringify the argument and check that the first char is a number != 0
> (or starting with a dash to cover negative numbers). Except for -O0
> builds the strings are all eliminated.
>
> Taking CONFIG_DRM_I915_REQUEST_TIMEOUT in
> drivers/gpu/drm/i915/gem/i915_gem_context.c as example, we have the
> following output of the preprocessor:
>
> old:
> if (((20000) != 0) &&
> new:
> if (( ("20000"[0] > '0' && "20000"[0] < '9') || "20000"[0] == '-' ) &&
>
> New one looks worse, but is also eliminated from the object:
>
> $ size drivers/gpu/drm/i915/gem/i915_gem_context.o.*
> text data bss dec hex filename
> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.new
> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.old
>
> Signed-off-by: Lucas De Marchi <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_utils.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 02bbfa4d68d3..436ce612c46a 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -28,6 +28,7 @@
> #include <linux/list.h>
> #include <linux/overflow.h>
> #include <linux/sched.h>
> +#include <linux/stringify.h>
> #include <linux/types.h>
> #include <linux/workqueue.h>
>
> @@ -469,6 +470,9 @@ static inline bool timer_expired(const struct timer_list *t)
> *
> * Returns 0 if @config is 0, 1 if set to any value.
> */
> -#define IS_CONFIG_NONZERO(config) ((config) != 0)
> +#define IS_CONFIG_NONZERO(config) ( \
> + (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \
Shouldn't this be "<= '9'". Otherwise numbers starting with a 9 are not
"non zero".
Steve
> + __stringify_1(config)[0] == '-' \
> +)
>
> #endif /* !__I915_UTILS_H */
>
On Thu, Sep 30, 2021 at 11:00:06AM +0100, Steven Price wrote:
>On 29/09/2021 19:33, Lucas De Marchi wrote:
>> Like the IS_ENABLED() counterpart, we can make IS_CONFIG_NONZERO() to
>> return the right thing when the config is not defined rather than a
>> build error, with the limitation that it can't be used on preprocessor
>> context.
>>
>> The trick here is that macro names can't start with a number or dash, so
>> we stringify the argument and check that the first char is a number != 0
>> (or starting with a dash to cover negative numbers). Except for -O0
>> builds the strings are all eliminated.
>>
>> Taking CONFIG_DRM_I915_REQUEST_TIMEOUT in
>> drivers/gpu/drm/i915/gem/i915_gem_context.c as example, we have the
>> following output of the preprocessor:
>>
>> old:
>> if (((20000) != 0) &&
>> new:
>> if (( ("20000"[0] > '0' && "20000"[0] < '9') || "20000"[0] == '-' ) &&
>>
>> New one looks worse, but is also eliminated from the object:
>>
>> $ size drivers/gpu/drm/i915/gem/i915_gem_context.o.*
>> text data bss dec hex filename
>> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.new
>> 52021 1070 232 53323 d04b drivers/gpu/drm/i915/gem/i915_gem_context.o.old
>>
>> Signed-off-by: Lucas De Marchi <[email protected]>
>> ---
>> drivers/gpu/drm/i915/i915_utils.h | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
>> index 02bbfa4d68d3..436ce612c46a 100644
>> --- a/drivers/gpu/drm/i915/i915_utils.h
>> +++ b/drivers/gpu/drm/i915/i915_utils.h
>> @@ -28,6 +28,7 @@
>> #include <linux/list.h>
>> #include <linux/overflow.h>
>> #include <linux/sched.h>
>> +#include <linux/stringify.h>
>> #include <linux/types.h>
>> #include <linux/workqueue.h>
>>
>> @@ -469,6 +470,9 @@ static inline bool timer_expired(const struct timer_list *t)
>> *
>> * Returns 0 if @config is 0, 1 if set to any value.
>> */
>> -#define IS_CONFIG_NONZERO(config) ((config) != 0)
>> +#define IS_CONFIG_NONZERO(config) ( \
>> + (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \
>
>Shouldn't this be "<= '9'". Otherwise numbers starting with a 9 are not
>"non zero".
yes! thanks for catching it. However from the other discussion it seems
we can either
a) just remove the macro, or
b) use the simpler version that doesn't cover undefined values
I will investigate those options.
Lucas De Marchi