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

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

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