Re: [igt-dev] [PATCH i-g-t 2/5] lib/igt_eld: consolidate ELD parsing

Martin Peres
2019-06-03T12:42:38Z

On 31/05/2019 14:21, Simon Ser wrote:
> Make the ELD enumeration more robust, and implement proper parsing for ELD
> fields. This will become useful when other ELD fields (formats, sample rates,
> sample sizes) will be parsed and checked.
>
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
> lib/igt_eld.c | 94 +++++++++++++++++++++++++++++++--------------------
> lib/igt_eld.h | 5 +++
> 2 files changed, 63 insertions(+), 36 deletions(-)
>
> diff --git a/lib/igt_eld.c b/lib/igt_eld.c
> index 8e0dcc306e85..a198001a56d7 100644
> --- a/lib/igt_eld.c
> +++ b/lib/igt_eld.c
> @@ -30,8 +30,12 @@
> #include <stdio.h>
> #include <string.h>
>
> +#include "igt_core.h"
> #include "igt_eld.h"
>
> +#define ELD_PREFIX "eld#"
> +#define ELD_DELIM " \t"
> +
> /**
> * EDID-Like Data (ELD) is metadata parsed and exposed by ALSA for HDMI and
> * DisplayPort connectors supporting audio. This includes the monitor name and
> @@ -39,39 +43,49 @@
> * on).
> */
>
> -/** eld_entry_is_igt: checks whether an ELD entry is mapped to the IGT EDID */

Posting an example of eld here would be helpful:

$ cat /proc/asound/card0/eld#0.2
monitor_present 1
eld_valid 1
monitor_name U2879G6

connection_type DisplayPort
eld_version [0x2] CEA-861D or below
edid_version [0x3] CEA-861-B, C or D
manufacture_id 0xe305
product_id 0x2879
port_id 0x800
support_hdcp 0
support_ai 0
audio_sync_delay 0
speakers [0x1] FL/FR
sad_count 1
sad0_coding_type [0x1] LPCM
sad0_channels 2
sad0_rates [0xe0] 32000 44100 48000
sad0_bits [0xe0000] 16 20 24

> -static bool eld_entry_is_igt(const char *path)
> +static bool eld_parse_entry(const char *path, struct eld_entry *eld)
> {
> - FILE *in;
> + FILE *f;
> char buf[1024];
> - uint8_t eld_valid = 0;
> - uint8_t mon_valid = 0;
> -
> - in = fopen(path, "r");
> - if (!in)
> - return false;
> + char *key, *value;
> + size_t len;
> + bool monitor_present;
>
> - memset(buf, 0, 1024);
> + memset(eld, 0, sizeof(*eld));
>
> - while ((fgets(buf, 1024, in)) != NULL) {
> - char *line = buf;
> + f = fopen(path, "r");
> + if (!f) {
> + igt_debug("Failed to open ELD file: %s\n", path);
> + return false;
> + }
>
> - if (!strncasecmp(line, "eld_valid", 9) &&
> - strstr(line, "1")) {
> - eld_valid++;
> - }

Good riddance! Ignore my comment from patch 1!

> + while ((fgets(buf, sizeof(buf), f)) != NULL) {
> + len = strlen(buf);
> + if (buf[len - 1] == '\n')
> + buf[len - 1] = '\0';
> +
> + key = strtok(buf, ELD_DELIM);
> + value = strtok(NULL, "");

Hmm, I had forgotten that strok was stateful when str == NULL!

> + /* Skip whitespace at the beginning */
> + value += strspn(value, ELD_DELIM);

Thanks for teaching me a new function :) This is quite practical!

After doing more python work, going back to parsing with C is a little
rough :D Not as bad as ASM though!

> +
> + if (strcmp(key, "monitor_present") == 0)
> + monitor_present = strcmp(value, "1") == 0;
> + else if (strcmp(key, "eld_valid") == 0)
> + eld->valid = strcmp(value, "1") == 0;
> + else if (strcmp(key, "monitor_name") == 0)
> + snprintf(eld->monitor_name, sizeof(eld->monitor_name),
> + "%s", value);
> + }
>
> - if (!strncasecmp(line, "monitor_name", 12) &&
> - strstr(line, "IGT")) {
> - mon_valid++;
> - }
> + if (ferror(f) != 0) {
> + igt_debug("Failed to read ELD file: %d\n", ferror(f));
> + return false;
> }
>
> - fclose(in);
> - if (mon_valid && eld_valid)
> - return true;
> + fclose(f);
>
> - return false;
> + return monitor_present;
> }
>
> /** eld_has_igt: check whether ALSA has detected the audio-capable IGT EDID by
> @@ -79,27 +93,35 @@ static bool eld_entry_is_igt(const char *path)
> bool eld_has_igt(void)
> {
> DIR *dir;
> - struct dirent *snd_hda;
> + struct dirent *dirent;
> int i;
> + char card[64];
> + char path[PATH_MAX];
> + struct eld_entry eld;
>
> for (i = 0; i < 8; i++) {
> - char cards[128];
> -
> - snprintf(cards, sizeof(cards), "/proc/asound/card%d", i);
> - dir = opendir(cards);
> + snprintf(card, sizeof(card), "/proc/asound/card%d", i);
> + dir = opendir(card);
> if (!dir)
> continue;
>
> - while ((snd_hda = readdir(dir))) {
> - char fpath[PATH_MAX];
> + while ((dirent = readdir(dir))) {
> + if (strncmp(dirent->d_name, ELD_PREFIX,
> + strlen(ELD_PREFIX)) != 0)
> + continue;
> +
> + snprintf(path, sizeof(path), "%s/%s", card,
> + dirent->d_name);
> + if (!eld_parse_entry(path, &eld)) {
> + continue;
> + }
>
> - if (*snd_hda->d_name == '.' ||
> - strstr(snd_hda->d_name, "eld") == 0)
> + if (!eld.valid) {
> + igt_debug("Skipping invalid ELD: %s\n", path);

The message is a little confusing. How about "Skipping non-ELD file:
%s\n" instead?

Anyway, I like where this is going!

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>

> continue;
> + }
>
> - snprintf(fpath, sizeof(fpath), "%s/%s", cards,
> - snd_hda->d_name);
> - if (eld_entry_is_igt(fpath)) {
> + if (strcmp(eld.monitor_name, "IGT") == 0) {
> closedir(dir);
> return true;
> }
> diff --git a/lib/igt_eld.h b/lib/igt_eld.h
> index 27f876d9f631..21fe3537acf9 100644
> --- a/lib/igt_eld.h
> +++ b/lib/igt_eld.h
> @@ -30,6 +30,11 @@
>
> #include <stdbool.h>
>
> +struct eld_entry {
> + bool valid;
> + char monitor_name[16];
> +};
> +
> bool eld_has_igt(void);
>
> #endif