Re: [igt-dev] [PATCH i-g-t 4/5] lib/igt_eld: allow retrieving an ELD entry per monitor name

Martin Peres
2019-06-03T13:04:54Z

On 31/05/2019 14:21, Simon Ser wrote:
> The previous function eld_has_igt (1) assumed the monitor name was always "IGT"
> and (2) didn't provide a way to access the ELD data.
>
> (1) is an issue for EDIDs that don't have "IGT" as the monitor name. This is
> the case for the Chamelium default EDID, but this will also become an issue
> with MST where each monitor will need to have a unique name (to be able to tell
> them apart).

I dislike the idea of not checking that the EDID has been generated by
IGT, but I understand that you might want some freedom there.

How about checking that the monitor name STARTS with IGT.

Given that Arek is also complaining about something like in patch 5, how
about merging 1-4, then reviewing/merging your DP patches, then
re-spinning the last two patches of this series?

Martin

>
> (2) makes it impossible to check ELD audio parameters.
>
> This commit fixes both (1) and (2).
>
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
> lib/igt_eld.c | 12 +++++-------
> lib/igt_eld.h | 2 +-
> tests/kms_hdmi_inject.c | 8 +++-----
> 3 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/lib/igt_eld.c b/lib/igt_eld.c
> index 732bbabd2d7c..53655c5e18f1 100644
> --- a/lib/igt_eld.c
> +++ b/lib/igt_eld.c
> @@ -171,16 +171,14 @@ static bool eld_parse_entry(const char *path, struct eld_entry *eld)
> return monitor_present;
> }
>
> -/** eld_has_igt: check whether ALSA has detected the audio-capable IGT EDID by
> - * parsing ELD entries */
> -bool eld_has_igt(void)
> +/** eld_from_monitor_name: retrieve an ELD entry from a monitor name */
> +bool eld_from_monitor_name(struct eld_entry *eld, const char *monitor_name)
> {
> DIR *dir;
> struct dirent *dirent;
> int i;
> char card[64];
> char path[PATH_MAX];
> - struct eld_entry eld;
>
> for (i = 0; i < 8; i++) {
> snprintf(card, sizeof(card), "/proc/asound/card%d", i);
> @@ -195,16 +193,16 @@ bool eld_has_igt(void)
>
> snprintf(path, sizeof(path), "%s/%s", card,
> dirent->d_name);
> - if (!eld_parse_entry(path, &eld)) {
> + if (!eld_parse_entry(path, eld)) {
> continue;
> }
>
> - if (!eld.valid) {
> + if (!eld->valid) {
> igt_debug("Skipping invalid ELD: %s\n", path);
> continue;
> }
>
> - if (strcmp(eld.monitor_name, "IGT") == 0) {
> + if (strcmp(eld->monitor_name, monitor_name) == 0) {
> closedir(dir);
> return true;
> }
> diff --git a/lib/igt_eld.h b/lib/igt_eld.h
> index e16187884d4b..1f34f784749b 100644
> --- a/lib/igt_eld.h
> +++ b/lib/igt_eld.h
> @@ -49,6 +49,6 @@ struct eld_entry {
> struct eld_sad sads[ELD_SADS_CAP];
> };
>
> -bool eld_has_igt(void);
> +bool eld_from_monitor_name(struct eld_entry *eld, const char *monitor_name);
>
> #endif
> diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> index eba25046cead..31ddda731d7b 100644
> --- a/tests/kms_hdmi_inject.c
> +++ b/tests/kms_hdmi_inject.c
> @@ -146,6 +146,7 @@ hdmi_inject_audio(int drm_fd, drmModeConnector *connector)
> int fb_id, cid, ret, crtc_mask = -1;
> struct igt_fb fb;
> struct kmstest_connector_config config;
> + struct eld_entry eld;
>
> kmstest_edid_add_audio(igt_kms_get_base_edid(), EDID_LENGTH, &edid,
> &length);
> @@ -178,11 +179,8 @@ hdmi_inject_audio(int drm_fd, drmModeConnector *connector)
>
> igt_assert(ret == 0);
>
> - /*
> - * Test if we have /proc/asound/HDMI/eld#0.0 and is its contents are
> - * valid.
> - */
> - igt_assert(eld_has_igt());
> + /* Check whether ALSA has properly detected the audio-capable EDID */
> + igt_assert(eld_from_monitor_name(&eld, "IGT"));
>
> igt_remove_fb(drm_fd, &fb);