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

["Ser, Simon" <simon.ser@intel.com>]

On Mon, 2019-06-03 at 15:42 +0300, Martin Peres wrote:
> 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!

Eheh, I can imagine that :P

> > +
> > +		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?

So, this is not exactly a non-ELD file: this happens when the EDID
isn't valid (e.g. if some field is malformed).

Is it okay if I change it to: "Skipping ELD entry with invalid=1"?

> 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