[igt-dev] [PATCH i-g-t 2/5] lib/igt_eld: consolidate ELD parsing
Ser, Simon
simon.ser at intel.com
Mon Jun 3 14:34:46 UTC 2019
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 at 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 at 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
> >
More information about the igt-dev
mailing list