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

[Martin Peres <martin.peres@linux.intel.com>]

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
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev