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.
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(-)
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 */
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 */
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 */
dri-devel@lists.freedesktop.org
-
Lucas De Marchi