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

Ser, Simon
2019-06-03T15:10:23Z

On Mon, 2019-06-03 at 16:04 +0300, Martin Peres wrote:
> 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?

Good idea!

> 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);
> >
> >