[PATCH v2 0/3] Rename IS_ACTIVE() and move to kconfig.h

[PATCH v2 0/3] Rename IS_ACTIVE() and move to kconfig.h

[Lucas De Marchi]

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

[PATCH v2 1/3] drm/i915: rename IS_ACTIVE

[Lucas De Marchi]

It took me some time to understand the need for IS_ACTIVE and why we
couldn't use kconfig.h. Rename it to something else that would be more
suitable to include in kconfig.h and shared with other subsystems rather
than maintaining it only in i915.

Name here is pretty open for suggestions, but I think this is slightly
better than "active".

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 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                  |  2 +-
 11 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 8208fd5b72c3..ff748441a0e2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -761,7 +761,7 @@ static int intel_context_set_gem(struct intel_context *ce,
 	    intel_engine_has_semaphores(ce->engine))
 		__set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
 
-	if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
+	if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
 	    ctx->i915->params.request_timeout_ms) {
 		unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 5130e8ed9564..9e12c026e59f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -395,7 +395,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf)
 	/* Track the mmo associated with the fenced vma */
 	vma->mmo = mmo;
 
-	if (IS_ACTIVE(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND))
+	if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND))
 		intel_wakeref_auto(&i915->ggtt.userfault_wakeref,
 				   msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
 
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 87579affb952..f181c8654cbf 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -273,7 +273,7 @@ static inline bool intel_engine_uses_guc(const struct intel_engine_cs *engine)
 static inline bool
 intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
 {
-	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
 		return false;
 
 	return intel_engine_has_preemption(engine);
@@ -300,7 +300,7 @@ intel_virtual_engine_has_heartbeat(const struct intel_engine_cs *engine)
 static inline bool
 intel_engine_has_heartbeat(const struct intel_engine_cs *engine)
 {
-	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
 		return false;
 
 	if (intel_engine_is_virtual(engine))
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
index 74775ae961b2..9b2eb0491c9d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
@@ -207,7 +207,7 @@ static void heartbeat(struct work_struct *wrk)
 
 void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine)
 {
-	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
 		return;
 
 	next_heartbeat(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 5ae1207c363b..938b49e41e48 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -556,7 +556,7 @@ intel_engine_has_semaphores(const struct intel_engine_cs *engine)
 static inline bool
 intel_engine_has_timeslices(const struct intel_engine_cs *engine)
 {
-	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
 		return false;
 
 	return engine->flags & I915_ENGINE_HAS_TIMESLICES;
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 7147fe80919e..851dce6bfc6f 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -3339,7 +3339,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
 		engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
 		if (can_preempt(engine)) {
 			engine->flags |= I915_ENGINE_HAS_PREEMPTION;
-			if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
+			if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
 				engine->flags |= I915_ENGINE_HAS_TIMESLICES;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
index 317eebf086c3..7ca44d0105b8 100644
--- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
@@ -290,7 +290,7 @@ static int live_heartbeat_fast(void *arg)
 	int err = 0;
 
 	/* Check that the heartbeat ticks at the desired rate. */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
 		return 0;
 
 	for_each_engine(engine, gt, id) {
@@ -352,7 +352,7 @@ static int live_heartbeat_off(void *arg)
 	int err = 0;
 
 	/* Check that we can turn off heartbeat and not interrupt VIP */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
 		return 0;
 
 	for_each_engine(engine, gt, id) {
diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
index b3863abc51f5..d7daded90e35 100644
--- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
+++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
@@ -992,7 +992,7 @@ static int live_timeslice_preempt(void *arg)
 	 * need to preempt the current task and replace it with another
 	 * ready task.
 	 */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
 		return 0;
 
 	obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
@@ -1122,7 +1122,7 @@ static int live_timeslice_rewind(void *arg)
 	 * but only a few of those requests, forcing us to rewind the
 	 * RING_TAIL of the original request.
 	 */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
 		return 0;
 
 	for_each_engine(engine, gt, id) {
@@ -1299,7 +1299,7 @@ static int live_timeslice_queue(void *arg)
 	 * ELSP[1] is already occupied, so must rely on timeslicing to
 	 * eject ELSP[0] in favour of the queue.)
 	 */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
 		return 0;
 
 	obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
@@ -1420,7 +1420,7 @@ static int live_timeslice_nopreempt(void *arg)
 	 * We should not timeslice into a request that is marked with
 	 * I915_REQUEST_NOPREEMPT.
 	 */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
 		return 0;
 
 	if (igt_spinner_init(&spin, gt))
@@ -2260,7 +2260,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg)
 	int err;
 
 	/* Preempt cancel non-preemptible spinner in ELSP0 */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
 		return 0;
 
 	if (!intel_has_reset_engine(arg->engine->gt))
@@ -2316,7 +2316,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg)
 	struct i915_request *rq;
 	int err;
 
-	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
 		return 0;
 
 	if (!intel_has_reset_engine(engine->gt))
@@ -3375,7 +3375,7 @@ static int live_preempt_timeout(void *arg)
 	 * Check that we force preemption to occur by cancelling the previous
 	 * context if it refuses to yield the GPU.
 	 */
-	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
+	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
 		return 0;
 
 	if (!intel_has_reset_engine(gt))
diff --git a/drivers/gpu/drm/i915/i915_config.c b/drivers/gpu/drm/i915/i915_config.c
index b79b5f6d2cfa..3566d26f2068 100644
--- a/drivers/gpu/drm/i915/i915_config.c
+++ b/drivers/gpu/drm/i915/i915_config.c
@@ -8,7 +8,7 @@
 unsigned long
 i915_fence_context_timeout(const struct drm_i915_private *i915, u64 context)
 {
-	if (context && IS_ACTIVE(CONFIG_DRM_I915_FENCE_TIMEOUT))
+	if (context && IS_CONFIG_NONZERO(CONFIG_DRM_I915_FENCE_TIMEOUT))
 		return msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 79da5eca60af..4745d3efadca 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1852,7 +1852,7 @@ long i915_request_wait(struct i915_request *rq,
 	 * completion. That requires having a good predictor for the request
 	 * duration, which we currently lack.
 	 */
-	if (IS_ACTIVE(CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT) &&
+	if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT) &&
 	    __i915_spin_request(rq, state))
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 5259edacde38..02bbfa4d68d3 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -469,6 +469,6 @@ static inline bool timer_expired(const struct timer_list *t)
  *
  * Returns 0 if @config is 0, 1 if set to any value.
  */
-#define IS_ACTIVE(config) ((config) != 0)
+#define IS_CONFIG_NONZERO(config) ((config) != 0)
 
 #endif /* !__I915_UTILS_H */
-- 
2.33.0

[PATCH v2 2/3] drm/i915/utils: do not depend on config being defined

[Lucas De Marchi]

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 <lucas.demarchi@intel.com>
---
 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

[PATCH v2 3/3] Move IS_CONFIG_NONZERO() to kconfig.h

[Lucas De Marchi]

The check for config value doesn't really belong to i915_utils.h - we
are trying to eliminate that utils helper and share them when possible
with other drivers and subsystems.

Rationale for having such macro is in commit
babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates")
whereas later it is improved to not break the build if used with
undefined configs. The caveat is detailed in the documentation: unlike
IS_ENABLED(): it's not preprocessor-only logic so can't be used for
things like `#if IS_CONFIG_NONZERO(...)`

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/i915_utils.h | 17 -----------------
 include/linux/kconfig.h           | 16 ++++++++++++++--
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 436ce612c46a..62f189e064a9 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -28,7 +28,6 @@
 #include <linux/list.h>
 #include <linux/overflow.h>
 #include <linux/sched.h>
-#include <linux/stringify.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -459,20 +458,4 @@ static inline bool timer_expired(const struct timer_list *t)
 	return timer_active(t) && !timer_pending(t);
 }
 
-/*
- * This is a lookalike for IS_ENABLED() that takes a kconfig value,
- * e.g. CONFIG_DRM_I915_SPIN_REQUEST, and evaluates whether it is non-zero
- * i.e. whether the configuration is active. Wrapping up the config inside
- * a boolean context prevents clang and smatch from complaining about potential
- * issues in confusing logical-&& with bitwise-& for constants.
- *
- * Sadly IS_ENABLED() itself does not work with kconfig values.
- *
- * Returns 0 if @config is 0, 1 if set to any value.
- */
-#define IS_CONFIG_NONZERO(config) (						\
-	(__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') ||	\
-	__stringify_1(config)[0] == '-'						\
-)
-
 #endif /* !__I915_UTILS_H */
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index 20d1079e92b4..e84f7c1c8e26 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -2,6 +2,7 @@
 #ifndef __LINUX_KCONFIG_H
 #define __LINUX_KCONFIG_H
 
+#include <linux/stringify.h>
 #include <generated/autoconf.h>
 
 #ifdef CONFIG_CPU_BIG_ENDIAN
@@ -26,8 +27,8 @@
 #define ____or(arg1_or_junk, y)		__take_second_arg(arg1_or_junk 1, y)
 
 /*
- * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
- * these only work with boolean and tristate options.
+ * Helper macros to use CONFIG_ options in C/CPP expressions. Note that except
+ * for IS_CONFIG_NONZERO, these only work with boolean and tristate options.
  */
 
 /*
@@ -72,4 +73,15 @@
  */
 #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
 
+/*
+ * This is a lookalike for IS_ENABLED(), but works with int kconfig options
+ * with the caveat that it can't be used on preprocessor checks.
+ *
+ * Returns 0 if @config is 0 or undefined, 1 if set to any value.
+ */
+#define IS_CONFIG_NONZERO(config) (						\
+	(__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') ||	\
+	__stringify_1(config)[0] == '-'						\
+)
+
 #endif /* __LINUX_KCONFIG_H */
-- 
2.33.0

Re: [PATCH v2 2/3] drm/i915/utils: do not depend on config being defined

[Andrzej Hajda]

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 <lucas.demarchi@intel.com>
> ---
>   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 */

Re: [PATCH v2 2/3] drm/i915/utils: do not depend on config being defined

[Lucas De Marchi]

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 <lucas.demarchi@intel.com>
>> ---
>>   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 */

Re: [PATCH v2 2/3] drm/i915/utils: do not depend on config being defined

[Andrzej Hajda]

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 <lucas.demarchi@intel.com>
>>> ---
>>>   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 */
>

Re: [PATCH v2 2/3] drm/i915/utils: do not depend on config being defined

[Steven Price]

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 <lucas.demarchi@intel.com>
> ---
>  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 */
> 

Re: [PATCH v2 1/3] drm/i915: rename IS_ACTIVE

[Jani Nikula]

On Wed, 29 Sep 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> It took me some time to understand the need for IS_ACTIVE and why we
> couldn't use kconfig.h.

For anyone else wondering, the clues are in babaab2f4738 ("drm/i915:
Encapsulate kconfig constant values inside boolean predicates").

But this series tries to take it further; we currently don't have a need
for checking whether the config is defined or not. It always is. I mean
it's probably a useful feature, but not the original problem we had.

BR,
Jani.


> Rename it to something else that would be more
> suitable to include in kconfig.h and shared with other subsystems rather
> than maintaining it only in i915.
>
> Name here is pretty open for suggestions, but I think this is slightly
> better than "active".
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  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                  |  2 +-
>  11 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 8208fd5b72c3..ff748441a0e2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -761,7 +761,7 @@ static int intel_context_set_gem(struct intel_context *ce,
>  	    intel_engine_has_semaphores(ce->engine))
>  		__set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags);
>  
> -	if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
> +	if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
>  	    ctx->i915->params.request_timeout_ms) {
>  		unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
>  
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 5130e8ed9564..9e12c026e59f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -395,7 +395,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf)
>  	/* Track the mmo associated with the fenced vma */
>  	vma->mmo = mmo;
>  
> -	if (IS_ACTIVE(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND))
> +	if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND))
>  		intel_wakeref_auto(&i915->ggtt.userfault_wakeref,
>  				   msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index 87579affb952..f181c8654cbf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -273,7 +273,7 @@ static inline bool intel_engine_uses_guc(const struct intel_engine_cs *engine)
>  static inline bool
>  intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
>  {
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
>  		return false;
>  
>  	return intel_engine_has_preemption(engine);
> @@ -300,7 +300,7 @@ intel_virtual_engine_has_heartbeat(const struct intel_engine_cs *engine)
>  static inline bool
>  intel_engine_has_heartbeat(const struct intel_engine_cs *engine)
>  {
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
>  		return false;
>  
>  	if (intel_engine_is_virtual(engine))
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index 74775ae961b2..9b2eb0491c9d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -207,7 +207,7 @@ static void heartbeat(struct work_struct *wrk)
>  
>  void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine)
>  {
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
>  		return;
>  
>  	next_heartbeat(engine);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 5ae1207c363b..938b49e41e48 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -556,7 +556,7 @@ intel_engine_has_semaphores(const struct intel_engine_cs *engine)
>  static inline bool
>  intel_engine_has_timeslices(const struct intel_engine_cs *engine)
>  {
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
>  		return false;
>  
>  	return engine->flags & I915_ENGINE_HAS_TIMESLICES;
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 7147fe80919e..851dce6bfc6f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -3339,7 +3339,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>  		engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
>  		if (can_preempt(engine)) {
>  			engine->flags |= I915_ENGINE_HAS_PREEMPTION;
> -			if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> +			if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
>  				engine->flags |= I915_ENGINE_HAS_TIMESLICES;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> index 317eebf086c3..7ca44d0105b8 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> @@ -290,7 +290,7 @@ static int live_heartbeat_fast(void *arg)
>  	int err = 0;
>  
>  	/* Check that the heartbeat ticks at the desired rate. */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
>  		return 0;
>  
>  	for_each_engine(engine, gt, id) {
> @@ -352,7 +352,7 @@ static int live_heartbeat_off(void *arg)
>  	int err = 0;
>  
>  	/* Check that we can turn off heartbeat and not interrupt VIP */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_HEARTBEAT_INTERVAL))
>  		return 0;
>  
>  	for_each_engine(engine, gt, id) {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> index b3863abc51f5..d7daded90e35 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> @@ -992,7 +992,7 @@ static int live_timeslice_preempt(void *arg)
>  	 * need to preempt the current task and replace it with another
>  	 * ready task.
>  	 */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
>  		return 0;
>  
>  	obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> @@ -1122,7 +1122,7 @@ static int live_timeslice_rewind(void *arg)
>  	 * but only a few of those requests, forcing us to rewind the
>  	 * RING_TAIL of the original request.
>  	 */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
>  		return 0;
>  
>  	for_each_engine(engine, gt, id) {
> @@ -1299,7 +1299,7 @@ static int live_timeslice_queue(void *arg)
>  	 * ELSP[1] is already occupied, so must rely on timeslicing to
>  	 * eject ELSP[0] in favour of the queue.)
>  	 */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
>  		return 0;
>  
>  	obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> @@ -1420,7 +1420,7 @@ static int live_timeslice_nopreempt(void *arg)
>  	 * We should not timeslice into a request that is marked with
>  	 * I915_REQUEST_NOPREEMPT.
>  	 */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_TIMESLICE_DURATION))
>  		return 0;
>  
>  	if (igt_spinner_init(&spin, gt))
> @@ -2260,7 +2260,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg)
>  	int err;
>  
>  	/* Preempt cancel non-preemptible spinner in ELSP0 */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
>  		return 0;
>  
>  	if (!intel_has_reset_engine(arg->engine->gt))
> @@ -2316,7 +2316,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg)
>  	struct i915_request *rq;
>  	int err;
>  
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
>  		return 0;
>  
>  	if (!intel_has_reset_engine(engine->gt))
> @@ -3375,7 +3375,7 @@ static int live_preempt_timeout(void *arg)
>  	 * Check that we force preemption to occur by cancelling the previous
>  	 * context if it refuses to yield the GPU.
>  	 */
> -	if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
> +	if (!IS_CONFIG_NONZERO(CONFIG_DRM_I915_PREEMPT_TIMEOUT))
>  		return 0;
>  
>  	if (!intel_has_reset_engine(gt))
> diff --git a/drivers/gpu/drm/i915/i915_config.c b/drivers/gpu/drm/i915/i915_config.c
> index b79b5f6d2cfa..3566d26f2068 100644
> --- a/drivers/gpu/drm/i915/i915_config.c
> +++ b/drivers/gpu/drm/i915/i915_config.c
> @@ -8,7 +8,7 @@
>  unsigned long
>  i915_fence_context_timeout(const struct drm_i915_private *i915, u64 context)
>  {
> -	if (context && IS_ACTIVE(CONFIG_DRM_I915_FENCE_TIMEOUT))
> +	if (context && IS_CONFIG_NONZERO(CONFIG_DRM_I915_FENCE_TIMEOUT))
>  		return msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 79da5eca60af..4745d3efadca 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1852,7 +1852,7 @@ long i915_request_wait(struct i915_request *rq,
>  	 * completion. That requires having a good predictor for the request
>  	 * duration, which we currently lack.
>  	 */
> -	if (IS_ACTIVE(CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT) &&
> +	if (IS_CONFIG_NONZERO(CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT) &&
>  	    __i915_spin_request(rq, state))
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 5259edacde38..02bbfa4d68d3 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -469,6 +469,6 @@ static inline bool timer_expired(const struct timer_list *t)
>   *
>   * Returns 0 if @config is 0, 1 if set to any value.
>   */
> -#define IS_ACTIVE(config) ((config) != 0)
> +#define IS_CONFIG_NONZERO(config) ((config) != 0)
>  
>  #endif /* !__I915_UTILS_H */

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 3/3] Move IS_CONFIG_NONZERO() to kconfig.h

[Masahiro Yamada]

On Thu, Sep 30, 2021 at 3:34 AM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> The check for config value doesn't really belong to i915_utils.h - we
> are trying to eliminate that utils helper and share them when possible
> with other drivers and subsystems.
>
> Rationale for having such macro is in commit
> babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates")
> whereas later it is improved to not break the build if used with
> undefined configs. The caveat is detailed in the documentation: unlike
> IS_ENABLED(): it's not preprocessor-only logic so can't be used for
> things like `#if IS_CONFIG_NONZERO(...)`
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>


Hypothetical "it would be nice to have ..." is really unneeded.

       if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0)
                     return
msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT);


is enough, and much cleaner.



This warning is shown only when a constant is used
together with '&&'.

Most of IS_ACTIVE can go away.

Given that, there are not many places where the IS_ACTIVE macro
is useful, even in the i915 driver.

For a few sources of the warnings,
replacing it with  != 0 or > 0 is just fine.

Of course, such an ugly macro is not worth being moved to <linux/kconfig.h>




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 2/3] drm/i915/utils: do not depend on config being defined

[Lucas De Marchi]

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 <lucas.demarchi@intel.com>
>> ---
>>  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

Re: [PATCH v2 1/3] drm/i915: rename IS_ACTIVE

[Lucas De Marchi]

On Thu, Sep 30, 2021 at 01:46:21PM +0300, Jani Nikula wrote:
>On Wed, 29 Sep 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> It took me some time to understand the need for IS_ACTIVE and why we
>> couldn't use kconfig.h.
>
>For anyone else wondering, the clues are in babaab2f4738 ("drm/i915:
>Encapsulate kconfig constant values inside boolean predicates").

yeah, I had added that info on the third patch when I try to move the
macro to kconfig.h since it would give information for kconfig
developers on why the macro is being used.

>
>But this series tries to take it further; we currently don't have a need
>for checking whether the config is defined or not. It always is. I mean
>it's probably a useful feature, but not the original problem we had.

yep, not trying to push hard on that direction... just tried to have the
same thing the other macros on kconfig.h have.

thanks
Lucas De Marchi

Re: [PATCH v2 3/3] Move IS_CONFIG_NONZERO() to kconfig.h

[Lucas De Marchi]

On Thu, Sep 30, 2021 at 11:01:36PM +0900, Masahiro Yamada wrote:
>On Thu, Sep 30, 2021 at 3:34 AM Lucas De Marchi
><lucas.demarchi@intel.com> wrote:
>>
>> The check for config value doesn't really belong to i915_utils.h - we
>> are trying to eliminate that utils helper and share them when possible
>> with other drivers and subsystems.
>>
>> Rationale for having such macro is in commit
>> babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates")
>> whereas later it is improved to not break the build if used with
>> undefined configs. The caveat is detailed in the documentation: unlike
>> IS_ENABLED(): it's not preprocessor-only logic so can't be used for
>> things like `#if IS_CONFIG_NONZERO(...)`
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>
>Hypothetical "it would be nice to have ..." is really unneeded.
>
>       if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0)
>                     return
>msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT);
>
>
>is enough, and much cleaner.
>
>
>
>This warning is shown only when a constant is used
>together with '&&'.
>
>Most of IS_ACTIVE can go away.
>
>Given that, there are not many places where the IS_ACTIVE macro
>is useful, even in the i915 driver.
>
>For a few sources of the warnings,
>replacing it with  != 0 or > 0 is just fine.

humn... maybe. Let me do a conversion in that direction and see what is
the outcome.

My original intention was to make IS_ENABLED() even uglier to cover the
int case, but after some tries it seems impossible to do on preprocessor
context, so I thought maybe it would be ok as a separate one.

>
>Of course, such an ugly macro is not worth being moved to <linux/kconfig.h>

if we don't handle the undefined case and only worry about encapsulating
it inside a boolean predicate, the macro would be very simple. Would
that be worth having in kconfig.h maybe?


thanks
Lucas De Marchi

Re: [PATCH v2 3/3] Move IS_CONFIG_NONZERO() to kconfig.h

[Masahiro Yamada]

On Fri, Oct 1, 2021 at 12:55 AM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> On Thu, Sep 30, 2021 at 11:01:36PM +0900, Masahiro Yamada wrote:
> >On Thu, Sep 30, 2021 at 3:34 AM Lucas De Marchi
> ><lucas.demarchi@intel.com> wrote:
> >>
> >> The check for config value doesn't really belong to i915_utils.h - we
> >> are trying to eliminate that utils helper and share them when possible
> >> with other drivers and subsystems.
> >>
> >> Rationale for having such macro is in commit
> >> babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates")
> >> whereas later it is improved to not break the build if used with
> >> undefined configs. The caveat is detailed in the documentation: unlike
> >> IS_ENABLED(): it's not preprocessor-only logic so can't be used for
> >> things like `#if IS_CONFIG_NONZERO(...)`
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >
> >
> >Hypothetical "it would be nice to have ..." is really unneeded.
> >
> >       if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0)
> >                     return
> >msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT);
> >
> >
> >is enough, and much cleaner.
> >
> >
> >
> >This warning is shown only when a constant is used
> >together with '&&'.
> >
> >Most of IS_ACTIVE can go away.
> >
> >Given that, there are not many places where the IS_ACTIVE macro
> >is useful, even in the i915 driver.
> >
> >For a few sources of the warnings,
> >replacing it with  != 0 or > 0 is just fine.
>
> humn... maybe. Let me do a conversion in that direction and see what is
> the outcome.
>
> My original intention was to make IS_ENABLED() even uglier to cover the
> int case, but after some tries it seems impossible to do on preprocessor
> context, so I thought maybe it would be ok as a separate one.
>
> >
> >Of course, such an ugly macro is not worth being moved to <linux/kconfig.h>
>
> if we don't handle the undefined case and only worry about encapsulating
> it inside a boolean predicate, the macro would be very simple. Would
> that be worth having in kconfig.h maybe?


I do not think so.

#define IS_CONFIG_NONZERO(config) ((config) != 0)

seems like a stupid macro.


What is bad about writing the direct code?

     if (x && CONFIG_FOO > 0)
                   ....





>
>
> thanks
> Lucas De Marchi



-- 
Best Regards
Masahiro Yamada