[igt-dev] [PATCH i-g-t 2/5] lib/igt_eld: consolidate ELD parsing
Peres, Martin
martin.peres at intel.com
Tue Jun 4 08:02:45 UTC 2019
On 03/06/2019 17:34, Ser, Simon wrote:
> 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).
Right, that was the previous code that was checking that, but you moved
it to the top with the strcmp.
>
> Is it okay if I change it to: "Skipping ELD entry with invalid=1"?
I think your original message was better. Sorry for the noise! Feel free
to merge!
>
>> 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
>>>
>
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
More information about the igt-dev
mailing list