From: Louis Chauvet <louis.chauvet@bootlin.com>
To: Mark Yacoub <markyacoub@google.com>, igt-dev@lists.freedesktop.org
Cc: kamil.konieczny@linux.intel.com, sebastian.brzezinka@intel.com
Subject: Re: [PATCH v6] lib/igt_rc: Introduce generic config parser
Date: Thu, 20 Aug 2026 19:53:21 +0200 [thread overview]
Message-ID: <e348acff-78da-44f5-b4c8-ffd3a70c6f56@bootlin.com> (raw)
In-Reply-To: <20260819142337.3232406-1-markyacoub@google.com>
On 8/19/26 16:23, Mark Yacoub wrote:
> Currently, libraries like unigraf and igt_core explicitly rely on
> GKeyFile for reading configuration from .igtrc. Android builds do
> not natively supply GLib, meaning these tools cannot be cleanly
> compiled.
>
> This patch avoids platform-specific diverging implementations by
> dropping the native GLib dependency from generic configuration access
> and parsing `.igtrc` natively using a stripped-down `igt_list`
> implementation.
>
> To prevent regressions for deeply-entrenched legacy Linux tools (such
> as Chamelium), the native GLib fallback `GKeyFile` structures are
> retained and populated parallel to the generic parser at startup.
That may be added in a TODO somewhere to show futur developers that we
want to move away from gkeyfile.
Othewise:
Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> v6:
> - Drop pthread_once dependency and expose idempotent igt_rc_init()
> to fix armhf linking (Kamil).
> - Replace __attribute__((destructor)) with explicit igt_rc_free()
> called during igt_exit().
> - Add 32-bit INT bounds checking in igt_rc_get_integer().
> - Optimize igt_rc_get_string() using reverse list traversal for fast
> INI override matching.
> - Fix trailing whitespace and header comment formatting flagged by
> checkpatch.pl.
>
> v5:
> - Rebased onto upstream 'lib/igt_core: move igt_load_igtrc to igt_rc'
> - Safely run GLib loader parallel to generic parser at startup for
> Chamelium
>
> v4:
> - Implement igt_rc_get_double() to parse floating point configs
> natively.
> - Migrate igt_core.c over to igt_rc_get_double() to drop GKeyFile
> dependencies.
> - Add robust standard bounds checking (ERANGE and unmatched digits)
> to number parsers.
>
> v3:
> - Drop Android-specific fallback paths; use IGT_CONFIG_PATH natively.
> - Drop glib Linux wrappers entirely; unify a single generic parser for
> all platforms.
> - Port manual linked-list tracking over to standard igt_list.h APIs.
> - Inherit base 0 for strtol to safely parse hex masks.
> - Use robust PATH_MAX for dynamic config paths.
> - Update pointer assignments, array indexing [0], and public
> docstrings.
>
> v2:
> - Drop the GKeyFile abstraction fakes from android/glib.h.
> - Introduce a generic wrapper (igt_rc.h) instead of modifying glib.h.
>
> Signed-off-by: Mark Yacoub <markyacoub@google.com>
> ---
> lib/igt_core.c | 39 ++----
> lib/igt_rc.c | 248 ++++++++++++++++++++++++++++++++++-
> lib/igt_rc.h | 12 +-
> lib/vendor/unigraf/unigraf.c | 92 ++++++-------
> 4 files changed, 303 insertions(+), 88 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 2f737b01a..93489dffc 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -974,39 +974,25 @@ static void oom_adjust_for_doom(void)
>
> static void common_init_config(void)
> {
> - GError *error = NULL;
> int ret = 0;
> static double timeout = 0.0;
>
> - igt_key_file = igt_load_igtrc();
> -
> - if (igt_key_file && !igt_frame_dump_path)
> - igt_frame_dump_path =
> - g_key_file_get_string(igt_key_file, "Common",
> - "FrameDumpPath", &error);
> -
> - g_clear_error(&error);
> -
> - if (igt_key_file)
> - ret = g_key_file_get_integer(igt_key_file, "DUT", "SuspendResumeDelay",
> - &error);
> - assert(!error || error->code != G_KEY_FILE_ERROR_INVALID_VALUE);
>
> - g_clear_error(&error);
> + igt_rc_init();
> + igt_key_file = igt_load_igtrc();
> + if (!igt_frame_dump_path)
> + igt_frame_dump_path = igt_rc_get_string("Common", "FrameDumpPath");
>
> - if (ret != 0)
> - igt_set_autoresume_delay(ret);
> + if (igt_rc_get_integer("DUT", "SuspendResumeDelay", &ret)) {
> + if (ret != 0)
> + igt_set_autoresume_delay(ret);
> + }
>
> - if (igt_key_file)
> - timeout = g_key_file_get_double(igt_key_file, "DUT", "DisplayDetectTimeout",
> - &error);
> - if (error) {
> + if (!igt_rc_get_double("DUT", "DisplayDetectTimeout", &timeout)) {
> igt_debug("Failed to read DisplayDetectTimeout, defaulting to %f\n",
> DEFAULT_DETECT_TIMEOUT);
> - g_clear_error(&error);
> timeout = DEFAULT_DETECT_TIMEOUT;
> }
> - g_clear_error(&error);
> igt_set_default_display_detect_timeout(timeout);
>
> /* Adding filters, order .igtrc, IGT_DEVICE, --device filter */
> @@ -1016,11 +1002,7 @@ static void common_init_config(void)
> if (igt_rc_device) {
> igt_debug("Notice: using IGT_DEVICE env:\n");
> } else {
> - if (igt_key_file)
> - igt_rc_device = g_key_file_get_string(igt_key_file,
> - "Common",
> - "Device", &error);
> - g_clear_error(&error);
> + igt_rc_device = igt_rc_get_string("Common", "Device");
> if (igt_rc_device)
> igt_debug("Notice: using .igtrc "
> "Common::Device:\n");
> @@ -2374,6 +2356,7 @@ void igt_exit(void)
>
> if (igt_key_file)
> g_key_file_free(igt_key_file);
> + igt_rc_free();
>
> if (run_single_subtest && !run_single_subtest_found) {
> igt_critical("Unknown subtest: %s\n", run_single_subtest);
> diff --git a/lib/igt_rc.c b/lib/igt_rc.c
> index b7f1731fe..4ae0acab5 100644
> --- a/lib/igt_rc.c
> +++ b/lib/igt_rc.c
> @@ -3,16 +3,252 @@
> * Copyright © 2026 Intel Corporation
> */
>
> +#include "igt_rc.h"
> +
> +#include <ctype.h>
> +#include <errno.h>
> +#include <limits.h>
> +#include <locale.h>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> +#include <strings.h>
>
> -#ifndef ANDROID
> -#include <glib.h>
> -#else
> -#include "android/glib.h"
> -#endif
> +#include "igt_list.h"
>
> -#include "igt_rc.h"
> +struct igt_key_entry {
> + char *group;
> + char *key;
> + char *value;
> + struct igt_list_head link;
> +};
> +
> +static IGT_LIST_HEAD(rc_entries);
> +
> +static char *trim_whitespace(char *str)
> +{
> + char *end;
> +
> + while (isspace((unsigned char)str[0]))
> + str++;
> +
> + if (str[0] == 0)
> + return str;
> +
> + end = str + strlen(str) - 1;
> + while (end > str && isspace((unsigned char)end[0]))
> + end--;
> +
> + end[1] = '\0';
> + return str;
> +}
> +
> +void igt_rc_init(void)
> +{
> + FILE *fp;
> + char *line = NULL;
> + size_t len = 0;
> + ssize_t read;
> + char *current_group = NULL;
> + char path[PATH_MAX];
> +
> + if (!igt_list_empty(&rc_entries))
> + return;
> +
> + char *config_path = getenv("IGT_CONFIG_PATH");
> +
> + if (config_path) {
> + snprintf(path, sizeof(path), "%s", config_path);
> + } else {
> + char *home = getenv("HOME");
> +
> + if (!home)
> + home = "";
> + snprintf(path, sizeof(path), "%s/.igtrc", home);
> + }
> +
> + fp = fopen(path, "r");
> + if (!fp)
> + return;
> +
> + while ((read = getline(&line, &len, fp)) != -1) {
> + char *trimmed = trim_whitespace(line);
> +
> + if (trimmed[0] == '\0' || trimmed[0] == '#' || trimmed[0] == ';')
> + continue;
> +
> + if (trimmed[0] == '[' && trimmed[strlen(trimmed) - 1] == ']') {
> + free(current_group);
> + trimmed[strlen(trimmed) - 1] = '\0';
> + current_group = strdup(trimmed + 1);
> + continue;
> + }
> +
> + if (current_group) {
> + char *eq = strchr(trimmed, '=');
> +
> + if (eq) {
> + char *key;
> + char *value;
> + struct igt_key_entry *entry;
> +
> + eq[0] = '\0';
> + value = eq + 1;
> + key = trim_whitespace(trimmed);
> + value = trim_whitespace(value);
> +
> + entry = calloc(1, sizeof(*entry));
> + entry->group = strdup(current_group);
> + entry->key = strdup(key);
> + entry->value = strdup(value);
> + igt_list_add_tail(&entry->link, &rc_entries);
> + }
> + }
> + }
> +
> + free(current_group);
> + free(line);
> + fclose(fp);
> +}
> +
> +void igt_rc_free(void)
> +{
> + struct igt_key_entry *curr, *tmp;
> +
> + igt_list_for_each_entry_safe(curr, tmp, &rc_entries, link) {
> + free(curr->group);
> + free(curr->key);
> + free(curr->value);
> + free(curr);
> + }
> + IGT_INIT_LIST_HEAD(&rc_entries);
> +}
> +
> +/**
> + * igt_rc_get_string:
> + * @group_name: The group name in the config file.
> + * @key: The key to look up.
> + *
> + * Looks up a string configuration value in the `.igtrc` file.
> + * The returned string is newly allocated and must be freed by
> + * the caller using free().
> + *
> + * Returns: A newly allocated string containing the value, or NULL if not found.
> + */
> +char *igt_rc_get_string(const char *group_name, const char *key)
> +{
> + struct igt_key_entry *curr;
> +
> + igt_list_for_each_entry_reverse(curr, &rc_entries, link) {
> + if (strcmp(curr->group, group_name) == 0 && strcmp(curr->key, key) == 0)
> + return strdup(curr->value);
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * igt_rc_get_boolean:
> + * @group_name: The group name in the config file.
> + * @key: The key to look up.
> + * @out: Pointer to a boolean where the result will be stored.
> + *
> + * Looks up a boolean configuration value in the `.igtrc` file.
> + * Parses standard boolean representations like "true", "false", "1", "0".
> + *
> + * Returns: true if the key exists and was successfully parsed, false otherwise.
> + */
> +bool igt_rc_get_boolean(const char *group_name, const char *key, bool *out)
> +{
> + char *val = igt_rc_get_string(group_name, key);
> +
> + if (!val)
> + return false;
> +
> + if (strcasecmp(val, "true") == 0 || strcmp(val, "1") == 0) {
> + *out = true;
> + } else if (strcasecmp(val, "false") == 0 || strcmp(val, "0") == 0) {
> + *out = false;
> + } else {
> + free(val);
> + return false;
> + }
> +
> + free(val);
> + return true;
> +}
> +
> +/**
> + * igt_rc_get_integer:
> + * @group_name: The group name in the config file.
> + * @key: The key to look up.
> + * @out: Pointer to an integer where the result will be stored.
> + *
> + * Looks up an integer configuration value in the `.igtrc` file.
> + *
> + * Returns: true if the key exists and was successfully parsed, false otherwise.
> + */
> +bool igt_rc_get_integer(const char *group_name, const char *key, int *out)
> +{
> + char *val = igt_rc_get_string(group_name, key);
> + char *endptr;
> + long lval;
> +
> + if (!val)
> + return false;
> +
> + errno = 0;
> + lval = strtol(val, &endptr, 0);
> + if (endptr == val || endptr[0] != '\0' || errno == ERANGE ||
> + lval < INT_MIN || lval > INT_MAX) {
> + free(val);
> + return false;
> + }
> +
> + *out = (int)lval;
> + free(val);
> + return true;
> +}
> +
> +/**
> + * igt_rc_get_double:
> + * @group_name: The group name in the config file.
> + * @key: The key to look up.
> + * @out: Pointer to a double where the result will be stored.
> + *
> + * Looks up a double configuration value in the `.igtrc` file.
> + *
> + * Returns: true if the key exists and was successfully parsed, false otherwise.
> + */
> +bool igt_rc_get_double(const char *group_name, const char *key, double *out)
> +{
> + char *val = igt_rc_get_string(group_name, key);
> + char *endptr;
> + double dval;
> + char *dot;
> + struct lconv *lc;
> +
> + if (!val)
> + return false;
> +
> + /* Handle locale-specific decimal separator for strtod */
> + dot = strchr(val, '.');
> + lc = localeconv();
> + if (dot && lc && lc->decimal_point && lc->decimal_point[0] != '.') {
> + *dot = lc->decimal_point[0];
> + }
> +
> + errno = 0;
> + dval = strtod(val, &endptr);
> + if (endptr == val || endptr[0] != '\0' || errno == ERANGE) {
> + free(val);
> + return false;
> + }
> +
> + *out = dval;
> + free(val);
> + return true;
> +}
>
> /**
> * igt_load_igtrc:
> diff --git a/lib/igt_rc.h b/lib/igt_rc.h
> index 3b7179808..61f883ee1 100644
> --- a/lib/igt_rc.h
> +++ b/lib/igt_rc.h
> @@ -21,10 +21,11 @@
> * IN THE SOFTWARE.
> */
>
> -
> #ifndef IGT_RC_H
> #define IGT_RC_H
>
> +#include <stdbool.h>
> +
> #ifndef ANDROID
> #include <glib.h>
> #else
> @@ -32,7 +33,14 @@
> #endif
>
> extern GKeyFile *igt_key_file;
> -
> struct _GKeyFile *igt_load_igtrc(void);
>
> +void igt_rc_init(void);
> +void igt_rc_free(void);
> +
> +char *igt_rc_get_string(const char *group_name, const char *key);
> +bool igt_rc_get_boolean(const char *group_name, const char *key, bool *out);
> +bool igt_rc_get_integer(const char *group_name, const char *key, int *out);
> +bool igt_rc_get_double(const char *group_name, const char *key, double *out);
> +
> #endif /* IGT_RC_H */
> diff --git a/lib/vendor/unigraf/unigraf.c b/lib/vendor/unigraf/unigraf.c
> index 30ee3c72b..a6b3008eb 100644
> --- a/lib/vendor/unigraf/unigraf.c
> +++ b/lib/vendor/unigraf/unigraf.c
> @@ -364,7 +364,6 @@ int unigraf_get_connector_id_by_stream(int drm_fd, int stream_id)
> bool unigraf_open_device(int drm_fd)
> {
> TSI_RESULT r;
> - GError *cfg_error = NULL;
> char *cfg_device = NULL;
> char *cfg_role = NULL;
> char *cfg_input = NULL;
> @@ -382,63 +381,52 @@ bool unigraf_open_device(int drm_fd)
>
> unigraf_init();
>
> - if (igt_key_file) {
> - cfg_device = g_key_file_get_string(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_DEVICE_NAME, &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("No device name configured, uses first device available.\n");
> - cfg_device = NULL;
> - }
> + cfg_device = igt_rc_get_string(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_DEVICE_NAME);
> + if (!cfg_device) {
> + unigraf_debug("No device name configured, uses first device available.\n");
> + cfg_device = NULL;
> + }
>
> - cfg_error = NULL;
> - cfg_role = g_key_file_get_string(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_DEVICE_ROLE, &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("No device role configured.\n");
> - cfg_role = NULL;
> - }
> + cfg_role = igt_rc_get_string(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_DEVICE_ROLE);
> + if (!cfg_role) {
> + unigraf_debug("No device role configured.\n");
> + cfg_role = NULL;
> + }
>
> - cfg_error = NULL;
> - cfg_input = g_key_file_get_string(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_INPUT_NAME, &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("No input name configured.\n");
> - cfg_input = NULL;
> - }
> + cfg_input = igt_rc_get_string(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_INPUT_NAME);
> + if (!cfg_input) {
> + unigraf_debug("No input name configured.\n");
> + cfg_input = NULL;
> + }
>
> - cfg_error = NULL;
> - unigraf_connector_name = g_key_file_get_string(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_CONNECTOR_NAME,
> - &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("No connector name configured, will autodetect.\n");
> - unigraf_connector_name = NULL;
> - }
> + unigraf_connector_name = igt_rc_get_string(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_CONNECTOR_NAME);
> + if (!unigraf_connector_name) {
> + unigraf_debug("No connector name configured, will autodetect.\n");
> + unigraf_connector_name = NULL;
> + }
>
> - cfg_error = NULL;
> - cfg_edid_name = g_key_file_get_string(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_EDID_NAME, &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("No default EDID set, use IGT default.\n");
> - cfg_edid_name = NULL;
> - }
> + cfg_edid_name = igt_rc_get_string(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_EDID_NAME);
> + if (!cfg_edid_name) {
> + unigraf_debug("No default EDID set, using IGT default.\n");
> + cfg_edid_name = NULL;
> + }
>
> - cfg_error = NULL;
> - unigraf_crc = g_key_file_get_boolean(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_USE_CRC_NAME, &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("CRC usage not configured, using unigraf CRC.\n");
> - unigraf_crc = true;
> - }
> + if (!igt_rc_get_boolean(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_USE_CRC_NAME, &unigraf_crc)) {
> + unigraf_debug("CRC usage not configured, using unigraf CRC.\n");
> + unigraf_crc = true;
> + }
>
> - cfg_error = NULL;
> - unigraf_stream_count = g_key_file_get_integer(igt_key_file, UNIGRAF_CONFIG_GROUP,
> - UNIGRAF_CONFIG_MST_STREAM_COUNT,
> - &cfg_error);
> - if (cfg_error) {
> - unigraf_debug("MST usage not configured, using SST.\n");
> - unigraf_stream_count = 0;
> - }
> + if (!igt_rc_get_integer(UNIGRAF_CONFIG_GROUP,
> + UNIGRAF_CONFIG_MST_STREAM_COUNT,
> + &unigraf_stream_count)) {
> + unigraf_debug("MST usage not configured, using SST.\n");
> + unigraf_stream_count = 0;
> }
>
> unigraf_assert(TSIX_DEV_RescanDevices(0, TSI_DEVCAP_VIDEO_CAPTURE, 0));
prev parent reply other threads:[~2026-08-20 17:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-19 14:23 [PATCH v6] lib/igt_rc: Introduce generic config parser Mark Yacoub
2026-08-19 16:15 ` ✓ Xe.CI.BAT: success for lib/igt_rc: Introduce generic config parser (rev2) Patchwork
2026-08-19 16:54 ` ✓ i915.CI.BAT: " Patchwork
2026-08-19 19:10 ` ✓ Xe.CI.FULL: " Patchwork
2026-08-19 19:45 ` ✓ i915.CI.Full: " Patchwork
2026-08-20 17:53 ` Louis Chauvet [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e348acff-78da-44f5-b4c8-ffd3a70c6f56@bootlin.com \
--to=louis.chauvet@bootlin.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=kamil.konieczny@linux.intel.com \
--cc=markyacoub@google.com \
--cc=sebastian.brzezinka@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox