[PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Shashank Sharma]

From: Shashank Sharma <shashank.sharma@amd.com>

This patch does some small changes to make the core logic of
edid-decode tool available to a shared library wrapper. With
these changes, the EDID's 'state' variable will be avialble
to another process via some library API calls.

Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>

Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
---
 Makefile        | 22 +++++++++++++++++++++-
 edid-decode.cpp | 15 ++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 1843700..ebf3370 100644
--- a/Makefile
+++ b/Makefile
@@ -1,14 +1,20 @@
 ifeq ($(OS),Windows_NT)
 	bindir ?= /usr/bin
 	mandir ?= /usr/share/man
+	libdir ?= /usr/lib
+	includedir ?= /usr/include
 else
 	UNAME_S := $(shell uname -s)
 	ifeq ($(UNAME_S),Darwin)
 		bindir ?= /usr/local/sbin
 		mandir ?= /usr/local/share/man
+		libdir ?= /usr/local/lib
+		includedir ?= /usr/include
 	else
 		bindir ?= /usr/bin
 		mandir ?= /usr/share/man
+		libdir ?= /usr/lib
+		includedir ?= /usr/include
 	endif
 endif
 
@@ -19,6 +25,11 @@ SOURCES = edid-decode.cpp parse-base-block.cpp parse-cta-block.cpp \
 	  parse-di-ext-block.cpp parse-vtb-ext-block.cpp calc-gtf-cvt.cpp
 WARN_FLAGS = -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wimplicit-fallthrough
 
+LIB_NAME = libedid-decode.so
+LIB_FLAGS = -fPIC
+LIBLINK_FLAGS = -shared
+LIB_SOURCES = libedid-decode-api.cpp
+
 all: edid-decode
 
 sha = -DSHA=$(shell if test -d .git ; then git rev-parse --short=12 HEAD ; fi)
@@ -30,11 +41,20 @@ edid-decode: $(SOURCES) edid-decode.h oui.h Makefile
 edid-decode.js: $(SOURCES) edid-decode.h oui.h Makefile
 	$(EMXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) $(sha) $(date) -s EXPORTED_FUNCTIONS='["_parse_edid"]' -s EXTRA_EXPORTED_RUNTIME_METHODS='["ccall", "cwrap"]' -o $@ $(SOURCES) -lm
 
+libedid-decode: $(SOURCES) edid-decode.h oui.h Makefile
+	$(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(LIB_FLAGS) $(sha) $(date) $(LIBLINK_FLAGS) -o $(LIB_NAME) $(LIB_SOURCES) $(SOURCES) -lm
+
 clean:
-	rm -f edid-decode
+	rm -f edid-decode libedid-decode.so
 
 install:
 	mkdir -p $(DESTDIR)$(bindir)
 	install -m 0755 edid-decode $(DESTDIR)$(bindir)
 	mkdir -p $(DESTDIR)$(mandir)/man1
 	install -m 0644 edid-decode.1 $(DESTDIR)$(mandir)/man1
+
+install-lib:
+	mkdir -p $(DESTDIR)$(libdir)
+	mkdir -p $(DESTDIR)$(includedir)
+	install -m 0755 libedid-decode.so $(DESTDIR)$(libdir)
+	install -m 0644 libedid-decode-api.h $(DESTDIR)$(includedir)
diff --git a/edid-decode.cpp b/edid-decode.cpp
index 4a90aba..babff4a 100644
--- a/edid-decode.cpp
+++ b/edid-decode.cpp
@@ -21,7 +21,7 @@
 #define STR(x) #x
 #define STRING(x) STR(x)
 
-static edid_state state;
+edid_state state;
 
 static unsigned char edid[EDID_PAGE_SIZE * EDID_MAX_BLOCKS];
 static bool odd_hex_digits;
@@ -1012,6 +1012,19 @@ static bool extract_edid(int fd, FILE *error)
 	state.edid_size = edid_data.size();
 	return true;
 }
+struct edid_state *extract_edid_state(int fd, FILE *error)
+{
+	bool ret;
+
+	ret = extract_edid(fd, error);
+	if (ret) {
+		/* update the number of blocks */
+		state.num_blocks = state.edid_size / EDID_PAGE_SIZE;
+		return &state;
+	}
+
+	return NULL;
+}
 
 static unsigned char crc_calc(const unsigned char *b)
 {
-- 
2.32.0

[PATCH 2/3] edid-decode: Introduce libedid-decode APIs

[Shashank Sharma]

From: Shashank Sharma <shashank.sharma@amd.com>

This patch adds a shared library wrapper for edid-decode
tool. With this library acting as an interface, other Linux
processes would also be able to analyze their EDIDs using the
core logic of edid-decode tools.

This would be particularly useful for applications like a Compositor
who wants to extract the information from an EDID, but doesn't
want to add tons of code to do that.

The initial version of the library APIs are basic and fundamental to
understand the response of the community. The long term plan is to
introduce more capable APIs which can:
- extract color correction and colorspace capabilities of the display
  from their respective CTA-861 blocks.
- extract advance information like static and dynamic HDR capabilities,
  YCBCR 4:2:0 support, color depth and bpc, max pixel clocks for
  HDMI 2.0, 2.1 etc.

This infomration will help a display manager or compositor to take
several decisions related to display states and modeset.

Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
---
 libedid-decode-api.cpp | 174 +++++++++++++++++++++++++++++++++++++++++
 libedid-decode-api.h   |  27 +++++++
 2 files changed, 201 insertions(+)
 create mode 100644 libedid-decode-api.cpp
 create mode 100644 libedid-decode-api.h

diff --git a/libedid-decode-api.cpp b/libedid-decode-api.cpp
new file mode 100644
index 0000000..ce06ba6
--- /dev/null
+++ b/libedid-decode-api.cpp
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Author: Shashank Sharma <contactshashanksharma@gmail.com>
+ */
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include "libedid-decode-api.h"
+
+extern struct edid_state *extract_edid_state(int fd, FILE *error);
+
+/*
+ * This is the init function for the API, a user must call
+ * this function with the EDID file node, to extract the edid
+ * into a state, and then call the rest of the APIs with that state
+ * to extract information about EDID.
+ */
+struct edid_state *libedid_parse_edid(const char *edid_path)
+{
+    int edid_fd, ret;
+    struct edid_state *estate;
+
+    if (!edid_path) {
+        printf("No EDID path provided\n");
+        return NULL;
+    }
+
+    /* Expecting path to a connector's EDID file like /sys/class/drm/..../edid */
+    edid_fd = open(edid_path, O_RDONLY);
+    if (edid_fd < 0) {
+        printf("Failed to open fd at path %s\n", edid_path);
+        return NULL;
+    }
+
+    /* Extract the infomrmation from edid node and prepare it's state */
+    estate = extract_edid_state(edid_fd, stderr);
+    if (!estate) {
+        printf("Failed to extract EDID\n");
+        return NULL;
+    }
+    printf("EDID extracted\n");
+
+    /* Now parse edid blocks */
+    ret = estate->parse_edid();
+    if (ret < 0) {
+        printf("Error parsing edid, err=%d \n", ret);
+        estate = NULL;
+    }
+
+    close(edid_fd);
+    return estate;
+}
+
+int libedid_num_blks(struct edid_state *estate)
+{
+    if (estate)
+        return estate->num_blocks;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_has_cta_blks(struct edid_state *estate)
+{
+    if (estate)
+        return estate->has_cta;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate)
+{
+    if (estate)
+        return estate->max_hor_freq_hz;
+
+    printf("EDID state not initialized\n");
+    return 0;
+}
+
+unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate)
+{
+    if (estate)
+        return estate->max_vert_freq_hz;
+
+    printf("EDID state not initialized\n");
+    return 0;
+}
+
+unsigned int libedid_get_max_pclk_khz(struct edid_state *estate)
+{
+    if (estate)
+        return estate->max_pixclk_khz;
+
+    printf("EDID state not initialized\n");
+    return 0;
+}
+
+int libedid_get_edid_version_minor(struct edid_state *estate)
+{
+    if (estate)
+        return estate->base.edid_minor;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_get_edid_get_num_dtd(struct edid_state *estate)
+{
+    if (estate)
+        return estate->base.dtd_cnt;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_if_preferred_mode_native(struct edid_state *estate)
+{
+    if (estate)
+        return estate->base.preferred_is_also_native;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm)
+{
+    if (estate && hmm && wmm) {
+        *hmm = estate->base.max_display_height_mm;
+        *wmm = estate->base.max_display_width_mm;
+        return 0;
+    }
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_ctablk_has_hdmi(struct edid_state *estate)
+{
+    if (estate)
+        return estate->cta.has_hdmi;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_ctablk_has_vcdb(struct edid_state *estate)
+{
+    if (estate)
+        return estate->cta.has_vcdb;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+int libedid_ctablk_has_hfvsdb(struct edid_state *estate)
+{
+    if (estate)
+        return estate->cta.have_hf_vsdb;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
+
+unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate)
+{
+    if (estate)
+        return estate->cta.supported_hdmi_vic_codes;
+
+    printf("EDID state not initialized\n");
+    return -1;
+}
\ No newline at end of file
diff --git a/libedid-decode-api.h b/libedid-decode-api.h
new file mode 100644
index 0000000..742b4a4
--- /dev/null
+++ b/libedid-decode-api.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: MIT
+ *
+ * Author: Shashank Sharma <contactshashanksharma@gmail.com>
+ */
+
+#ifndef __LIBEDID_DECODE_API_H_
+#define __LIBEDID_DECODE_API_H_
+
+#include "edid-decode.h"
+
+struct edid_state *libedid_parse_edid(const char *edid_path);
+int libedid_num_blks(struct edid_state *estate);
+int libedid_has_cta_blks(struct edid_state *estate);
+int libedid_get_edid_version_minor(struct edid_state *estate);
+int libedid_get_edid_get_num_dtd(struct edid_state *estate);
+int libedid_if_preferred_mode_native(struct edid_state *estate);
+int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm);
+int libedid_ctablk_has_hdmi(struct edid_state *estate);
+int libedid_ctablk_has_vcdb(struct edid_state *estate);
+int libedid_ctablk_has_hfvsdb(struct edid_state *estate);
+
+unsigned int libedid_get_max_pclk_khz(struct edid_state *estate);
+unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate);
+unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate);
+unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate);
+
+#endif
-- 
2.32.0

[PATCH 3/3] edid-decode: Add test utility for libedid-decode

[Shashank Sharma]

From: Shashank Sharma <shashank.sharma@amd.com>

This patch adds a small test utility to show usage of
libedid-decode.

Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>

Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
---
 Makefile     |   3 ++
 test-lib.cpp | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 test-lib.cpp

diff --git a/Makefile b/Makefile
index ebf3370..fdd02a0 100644
--- a/Makefile
+++ b/Makefile
@@ -44,6 +44,9 @@ edid-decode.js: $(SOURCES) edid-decode.h oui.h Makefile
 libedid-decode: $(SOURCES) edid-decode.h oui.h Makefile
 	$(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(LIB_FLAGS) $(sha) $(date) $(LIBLINK_FLAGS) -o $(LIB_NAME) $(LIB_SOURCES) $(SOURCES) -lm
 
+libedid-test:
+	$(CXX)  -o test-lib test-lib.cpp -g -L$(PWD) -ledid-decode
+
 clean:
 	rm -f edid-decode libedid-decode.so
 
diff --git a/test-lib.cpp b/test-lib.cpp
new file mode 100644
index 0000000..9b86adb
--- /dev/null
+++ b/test-lib.cpp
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Author: Shashank Sharma <contactshashanksharma@gmail.com>
+ */
+
+#include <stdio.h>
+#include <sys/types.h>
+#include "libedid-decode-api.h"
+#define EDID_PATH "/sys/class/drm/card0-HDMI-A-1/edid"
+
+/* This is a sample program to demo the usages of libedid-decode */
+
+int main(void)
+{
+    int ret;
+    unsigned int u_ret;
+    struct edid_state *estate;
+    int w = 0, h = 0;
+
+    estate = libedid_parse_edid(EDID_PATH);
+    if (!estate) {
+        printf("Failed to parse EDID\n");
+        return -1;
+    }
+
+    printf("EDID parsing success\n");
+
+
+    ret = libedid_num_blks(estate);
+    if (ret < 0) {
+        printf("Error: failed to get num blocks\n");
+        return -1;
+    }
+    printf("EDID: No of blocks: %d\n", ret);
+
+    ret = libedid_has_cta_blks(estate);
+    if (ret < 0) {
+        printf("Error: failed to check CTA block\n");
+        return -1;
+    }
+    printf("EDID: EDID %s CTA blocks\n", ret ? "has" : "doesn't have");
+
+    ret = libedid_get_edid_version_minor(estate);
+    if (ret < 0) {
+        printf("Error: failed to check EDID version\n");
+        return -1;
+    }
+    printf("EDID: EDID version: 1.%d\n", ret);
+
+    ret = libedid_get_edid_get_num_dtd(estate);
+    if (ret < 0) {
+        printf("Error: failed to check EDID num DTDs\n");
+        return -1;
+    }
+    printf("EDID: EDID has %d DTDs\n", ret);
+
+    ret = libedid_if_preferred_mode_native(estate);
+    if (ret < 0) {
+        printf("Error: failed to check preferred mode\n");
+        return -1;
+    }
+    printf("EDID: EDID preferred mode is %s\n", ret ? "native" : "not native");
+
+    ret = libedid_get_max_display_w_h_mm(estate, &w, &h);
+    if (ret < 0) {
+        printf("Error: failed to check height/width\n");
+        return -1;
+    }
+    printf("EDID: EDID hxw is %dx%d\n", w, h);
+
+    ret = libedid_ctablk_has_hdmi(estate);
+    if (ret < 0) {
+        printf("Error: failed to check CTA HDMI support\n");
+        return -1;
+    }
+    printf("EDID: CTA block %s HDMI\n", ret ? "supports" : "doesn't support");
+
+    ret = libedid_ctablk_has_vcdb(estate);
+    if (ret < 0) {
+        printf("Error: failed to check CTA VCDB support\n");
+        return -1;
+    }
+    printf("EDID: CTA block %s VCDB\n", ret ? "has" : "doesn't have");
+
+    ret = libedid_ctablk_has_hfvsdb(estate);
+    if (ret < 0) {
+        printf("Error: failed to check CTA HF-VSDB support\n");
+        return -1;
+    }
+    printf("EDID: CTA block %s HF-VSDB\n", ret ? "has" : "doesn't have");
+
+    u_ret = libedid_get_max_pclk_khz(estate);
+    if (u_ret == 0) {
+        printf("Error: failed to get max pixel clock\n");
+        return -1;
+    }
+    printf("EDID: EDID's max pixel clock is %u KHz\n", u_ret);
+
+    u_ret = libedid_get_max_hfreq_hz(estate);
+    if (u_ret == 0) {
+        printf("Error: failed to get max hfreq\n");
+        return -1;
+    }
+    printf("EDID: EDID's max hfreq is %u Hz\n", u_ret);
+
+    u_ret = libedid_get_max_vfreq_hz(estate);
+    if (u_ret == 0) {
+        printf("Error: failed to get max vfreq\n");
+        return -1;
+    }
+    printf("EDID: EDID's max vfreq is %u Hz\n", u_ret);
+
+    u_ret = libedid_ctablk_supported_hdmi_vics(estate);
+    if (u_ret == 0) {
+        printf("Error: failed to get supported VICs\n");
+        return -1;
+    }
+    printf("EDID: EDID's supported HDMI VICs map is %u\n", u_ret);
+
+    return 0;
+}
\ No newline at end of file
-- 
2.32.0

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4584 bytes --]

On Fri,  4 Mar 2022 13:49:59 +0100
Shashank Sharma <contactshashanksharma@gmail.com> wrote:

> From: Shashank Sharma <shashank.sharma@amd.com>
> 
> This patch does some small changes to make the core logic of
> edid-decode tool available to a shared library wrapper. With
> these changes, the EDID's 'state' variable will be avialble
> to another process via some library API calls.
> 
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>

Hi Shashank,

thank you very much for working on this!

> ---
>  Makefile        | 22 +++++++++++++++++++++-
>  edid-decode.cpp | 15 ++++++++++++++-
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 1843700..ebf3370 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,14 +1,20 @@
>  ifeq ($(OS),Windows_NT)
>  	bindir ?= /usr/bin
>  	mandir ?= /usr/share/man
> +	libdir ?= /usr/lib
> +	includedir ?= /usr/include
>  else
>  	UNAME_S := $(shell uname -s)
>  	ifeq ($(UNAME_S),Darwin)
>  		bindir ?= /usr/local/sbin
>  		mandir ?= /usr/local/share/man
> +		libdir ?= /usr/local/lib
> +		includedir ?= /usr/include
>  	else
>  		bindir ?= /usr/bin
>  		mandir ?= /usr/share/man
> +		libdir ?= /usr/lib
> +		includedir ?= /usr/include
>  	endif
>  endif
>  
> @@ -19,6 +25,11 @@ SOURCES = edid-decode.cpp parse-base-block.cpp parse-cta-block.cpp \
>  	  parse-di-ext-block.cpp parse-vtb-ext-block.cpp calc-gtf-cvt.cpp
>  WARN_FLAGS = -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wimplicit-fallthrough
>  
> +LIB_NAME = libedid-decode.so
> +LIB_FLAGS = -fPIC
> +LIBLINK_FLAGS = -shared
> +LIB_SOURCES = libedid-decode-api.cpp

libedid-decode-api.cpp does not exist yet in this patch.

> +
>  all: edid-decode
>  
>  sha = -DSHA=$(shell if test -d .git ; then git rev-parse --short=12 HEAD ; fi)
> @@ -30,11 +41,20 @@ edid-decode: $(SOURCES) edid-decode.h oui.h Makefile
>  edid-decode.js: $(SOURCES) edid-decode.h oui.h Makefile
>  	$(EMXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) $(sha) $(date) -s EXPORTED_FUNCTIONS='["_parse_edid"]' -s EXTRA_EXPORTED_RUNTIME_METHODS='["ccall", "cwrap"]' -o $@ $(SOURCES) -lm
>  
> +libedid-decode: $(SOURCES) edid-decode.h oui.h Makefile
> +	$(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(LIB_FLAGS) $(sha) $(date) $(LIBLINK_FLAGS) -o $(LIB_NAME) $(LIB_SOURCES) $(SOURCES) -lm
> +
>  clean:
> -	rm -f edid-decode
> +	rm -f edid-decode libedid-decode.so
>  
>  install:
>  	mkdir -p $(DESTDIR)$(bindir)
>  	install -m 0755 edid-decode $(DESTDIR)$(bindir)
>  	mkdir -p $(DESTDIR)$(mandir)/man1
>  	install -m 0644 edid-decode.1 $(DESTDIR)$(mandir)/man1
> +
> +install-lib:
> +	mkdir -p $(DESTDIR)$(libdir)
> +	mkdir -p $(DESTDIR)$(includedir)
> +	install -m 0755 libedid-decode.so $(DESTDIR)$(libdir)
> +	install -m 0644 libedid-decode-api.h $(DESTDIR)$(includedir)

libedid-decode-api.h does not exist yet in this patch.

I find it a little odd to have these targets here without the actual
files. Maybe the first patch could already have a library building but
expose just parse and destroy functions without any getters yet?

> diff --git a/edid-decode.cpp b/edid-decode.cpp
> index 4a90aba..babff4a 100644
> --- a/edid-decode.cpp
> +++ b/edid-decode.cpp
> @@ -21,7 +21,7 @@
>  #define STR(x) #x
>  #define STRING(x) STR(x)
>  
> -static edid_state state;
> +edid_state state;
>  
>  static unsigned char edid[EDID_PAGE_SIZE * EDID_MAX_BLOCKS];
>  static bool odd_hex_digits;
> @@ -1012,6 +1012,19 @@ static bool extract_edid(int fd, FILE *error)
>  	state.edid_size = edid_data.size();
>  	return true;
>  }
> +struct edid_state *extract_edid_state(int fd, FILE *error)
> +{
> +	bool ret;
> +
> +	ret = extract_edid(fd, error);
> +	if (ret) {
> +		/* update the number of blocks */
> +		state.num_blocks = state.edid_size / EDID_PAGE_SIZE;
> +		return &state;

A library should not give out pointers to global mutable data. That
would break having multiple EDIDs loaded at the same time.

I would expect to be able to keep and cache 'struct edid_state'
instances created by this library until I explicitly destroy them.
I would not expect parsing a new EDID to overwrite the previously
returned object. IOW, I would expect to own the object created by the
library.


Thanks,
pq

> +	}
> +
> +	return NULL;
> +}
>  
>  static unsigned char crc_calc(const unsigned char *b)
>  {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] edid-decode: Introduce libedid-decode APIs

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 10325 bytes --]

On Fri,  4 Mar 2022 13:50:00 +0100
Shashank Sharma <contactshashanksharma@gmail.com> wrote:

> From: Shashank Sharma <shashank.sharma@amd.com>
> 
> This patch adds a shared library wrapper for edid-decode
> tool. With this library acting as an interface, other Linux
> processes would also be able to analyze their EDIDs using the
> core logic of edid-decode tools.
> 
> This would be particularly useful for applications like a Compositor
> who wants to extract the information from an EDID, but doesn't
> want to add tons of code to do that.
> 
> The initial version of the library APIs are basic and fundamental to
> understand the response of the community. The long term plan is to
> introduce more capable APIs which can:
> - extract color correction and colorspace capabilities of the display
>   from their respective CTA-861 blocks.
> - extract advance information like static and dynamic HDR capabilities,
>   YCBCR 4:2:0 support, color depth and bpc, max pixel clocks for
>   HDMI 2.0, 2.1 etc.
> 
> This infomration will help a display manager or compositor to take
> several decisions related to display states and modeset.
> 
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
> ---
>  libedid-decode-api.cpp | 174 +++++++++++++++++++++++++++++++++++++++++
>  libedid-decode-api.h   |  27 +++++++

You forgot to add the header in the Makefile as a dependency.


>  2 files changed, 201 insertions(+)
>  create mode 100644 libedid-decode-api.cpp
>  create mode 100644 libedid-decode-api.h
> 
> diff --git a/libedid-decode-api.cpp b/libedid-decode-api.cpp
> new file mode 100644
> index 0000000..ce06ba6
> --- /dev/null
> +++ b/libedid-decode-api.cpp
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Author: Shashank Sharma <contactshashanksharma@gmail.com>
> + */
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include "libedid-decode-api.h"
> +
> +extern struct edid_state *extract_edid_state(int fd, FILE *error);
> +
> +/*
> + * This is the init function for the API, a user must call
> + * this function with the EDID file node, to extract the edid
> + * into a state, and then call the rest of the APIs with that state
> + * to extract information about EDID.
> + */
> +struct edid_state *libedid_parse_edid(const char *edid_path)
> +{
> +    int edid_fd, ret;
> +    struct edid_state *estate;
> +
> +    if (!edid_path) {
> +        printf("No EDID path provided\n");
> +        return NULL;
> +    }
> +
> +    /* Expecting path to a connector's EDID file like /sys/class/drm/..../edid */
> +    edid_fd = open(edid_path, O_RDONLY);
> +    if (edid_fd < 0) {
> +        printf("Failed to open fd at path %s\n", edid_path);
> +        return NULL;
> +    }
> +
> +    /* Extract the infomrmation from edid node and prepare it's state */
> +    estate = extract_edid_state(edid_fd, stderr);
> +    if (!estate) {
> +        printf("Failed to extract EDID\n");
> +        return NULL;
> +    }
> +    printf("EDID extracted\n");
> +
> +    /* Now parse edid blocks */
> +    ret = estate->parse_edid();
> +    if (ret < 0) {
> +        printf("Error parsing edid, err=%d \n", ret);
> +        estate = NULL;
> +    }
> +
> +    close(edid_fd);
> +    return estate;
> +}
> +
> +int libedid_num_blks(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->num_blocks;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;

Can you not simply document and require that 'estate' must be a valid
pointer created with libedid_parse_edid()?

A library probably should not printf() anything. I was just discussing
how libinput has a context object that you have to pass to every
function, but in libedid-decode case that context object might as well
be 'estate'. If there is a reason to log messages, the context object
carries callback hooks that the user can set.

A context object is also the place to put all the variables that would
otherwise need to be global. Global variables are not good for
libraries. Then again, I would not expect a EDID parsing library to
need any global data, and very few context data. Logging functions at
most.

> +}
> +
> +int libedid_has_cta_blks(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->has_cta;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->max_hor_freq_hz;
> +
> +    printf("EDID state not initialized\n");
> +    return 0;
> +}
> +
> +unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->max_vert_freq_hz;
> +
> +    printf("EDID state not initialized\n");
> +    return 0;
> +}
> +
> +unsigned int libedid_get_max_pclk_khz(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->max_pixclk_khz;
> +
> +    printf("EDID state not initialized\n");
> +    return 0;
> +}
> +
> +int libedid_get_edid_version_minor(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->base.edid_minor;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +int libedid_get_edid_get_num_dtd(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->base.dtd_cnt;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +int libedid_if_preferred_mode_native(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->base.preferred_is_also_native;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm)
> +{
> +    if (estate && hmm && wmm) {
> +        *hmm = estate->base.max_display_height_mm;
> +        *wmm = estate->base.max_display_width_mm;
> +        return 0;
> +    }
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +int libedid_ctablk_has_hdmi(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->cta.has_hdmi;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +int libedid_ctablk_has_vcdb(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->cta.has_vcdb;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +int libedid_ctablk_has_hfvsdb(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->cta.have_hf_vsdb;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> +
> +unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate)
> +{
> +    if (estate)
> +        return estate->cta.supported_hdmi_vic_codes;
> +
> +    printf("EDID state not initialized\n");
> +    return -1;
> +}
> \ No newline at end of file
> diff --git a/libedid-decode-api.h b/libedid-decode-api.h
> new file mode 100644
> index 0000000..742b4a4
> --- /dev/null
> +++ b/libedid-decode-api.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Author: Shashank Sharma <contactshashanksharma@gmail.com>
> + */
> +
> +#ifndef __LIBEDID_DECODE_API_H_
> +#define __LIBEDID_DECODE_API_H_
> +
> +#include "edid-decode.h"

You cannot include edid-decode.h in this intended-for-public header for
a couple of reasons:

- It is C++, while the library API/ABI needs to be C.
- You probably do not want to expose all those details as library ABI.

All the below should be declared inside an extern "C" block when
compiling as C++.

The most important bit is to document all the public functions so we
can understand how you expect the API to be used.

Would it be possible to use bool from stdbool.h?

> +
> +struct edid_state *libedid_parse_edid(const char *edid_path);

KMS clients get the raw bytes array from the kernel. Wayland
compositors would have no use for loading a file path, nor do they have
an fd pointing to the EDID data.

(const void *edid_data, size_t len) would be a more usable signature.

> +int libedid_num_blks(struct edid_state *estate);
> +int libedid_has_cta_blks(struct edid_state *estate);

How are these supposed to be used?

Do you expect the user to poke into struct edid_state internals?
If you don't, then 'struct edid_state' should be an opaque type. Right
now it's not opaque.

> +int libedid_get_edid_version_minor(struct edid_state *estate);
> +int libedid_get_edid_get_num_dtd(struct edid_state *estate);
> +int libedid_if_preferred_mode_native(struct edid_state *estate);
> +int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm);
> +int libedid_ctablk_has_hdmi(struct edid_state *estate);
> +int libedid_ctablk_has_vcdb(struct edid_state *estate);
> +int libedid_ctablk_has_hfvsdb(struct edid_state *estate);
> +
> +unsigned int libedid_get_max_pclk_khz(struct edid_state *estate);
> +unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate);
> +unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate);
> +unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate);
> +
> +#endif

Getters are fine, but maybe some of them could be grouped like the max
display width&height mm are. However, for each getter you have to ask:
how would the caller use the returned value? If there is no clear idea
of that, then the getter is not yet needed.

Some immediately useful getters would be for monitor make, model and
serial. If some of those fields could have multiple different source
blocks in EDID, then this would also be a nice demonstration of the
low-level vs. high-level API. In low-level API one might ask if a
certain block exists, and if so, access its fields. High-level API just
checks all the places and returns the most accurate information
available when the caller does not care where it comes from.

How do I destroy the struct edid_state I created?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Shashank Sharma]

Hello Pekka,

On 07.03.22 16:54, Pekka Paalanen wrote:
> On Fri,  4 Mar 2022 13:49:59 +0100
> Shashank Sharma <contactshashanksharma@gmail.com> wrote:
>
>> From: Shashank Sharma <shashank.sharma@amd.com>
>>
>> This patch does some small changes to make the core logic of
>> edid-decode tool available to a shared library wrapper. With
>> these changes, the EDID's 'state' variable will be avialble
>> to another process via some library API calls.
>>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>>
>> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
> Hi Shashank,
>
> thank you very much for working on this!
>
>> ---
>>   Makefile        | 22 +++++++++++++++++++++-
>>   edid-decode.cpp | 15 ++++++++++++++-
>>   2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 1843700..ebf3370 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1,14 +1,20 @@
>>   ifeq ($(OS),Windows_NT)
>>   	bindir ?= /usr/bin
>>   	mandir ?= /usr/share/man
>> +	libdir ?= /usr/lib
>> +	includedir ?= /usr/include
>>   else
>>   	UNAME_S := $(shell uname -s)
>>   	ifeq ($(UNAME_S),Darwin)
>>   		bindir ?= /usr/local/sbin
>>   		mandir ?= /usr/local/share/man
>> +		libdir ?= /usr/local/lib
>> +		includedir ?= /usr/include
>>   	else
>>   		bindir ?= /usr/bin
>>   		mandir ?= /usr/share/man
>> +		libdir ?= /usr/lib
>> +		includedir ?= /usr/include
>>   	endif
>>   endif
>>   
>> @@ -19,6 +25,11 @@ SOURCES = edid-decode.cpp parse-base-block.cpp parse-cta-block.cpp \
>>   	  parse-di-ext-block.cpp parse-vtb-ext-block.cpp calc-gtf-cvt.cpp
>>   WARN_FLAGS = -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wimplicit-fallthrough
>>   
>> +LIB_NAME = libedid-decode.so
>> +LIB_FLAGS = -fPIC
>> +LIBLINK_FLAGS = -shared
>> +LIB_SOURCES = libedid-decode-api.cpp
> libedid-decode-api.cpp does not exist yet in this patch.
Yes, the API is introduced in patch 2, Noted.
>> +
>>   all: edid-decode
>>   
>>   sha = -DSHA=$(shell if test -d .git ; then git rev-parse --short=12 HEAD ; fi)
>> @@ -30,11 +41,20 @@ edid-decode: $(SOURCES) edid-decode.h oui.h Makefile
>>   edid-decode.js: $(SOURCES) edid-decode.h oui.h Makefile
>>   	$(EMXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) $(sha) $(date) -s EXPORTED_FUNCTIONS='["_parse_edid"]' -s EXTRA_EXPORTED_RUNTIME_METHODS='["ccall", "cwrap"]' -o $@ $(SOURCES) -lm
>>   
>> +libedid-decode: $(SOURCES) edid-decode.h oui.h Makefile
>> +	$(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(LIB_FLAGS) $(sha) $(date) $(LIBLINK_FLAGS) -o $(LIB_NAME) $(LIB_SOURCES) $(SOURCES) -lm
>> +
>>   clean:
>> -	rm -f edid-decode
>> +	rm -f edid-decode libedid-decode.so
>>   
>>   install:
>>   	mkdir -p $(DESTDIR)$(bindir)
>>   	install -m 0755 edid-decode $(DESTDIR)$(bindir)
>>   	mkdir -p $(DESTDIR)$(mandir)/man1
>>   	install -m 0644 edid-decode.1 $(DESTDIR)$(mandir)/man1
>> +
>> +install-lib:
>> +	mkdir -p $(DESTDIR)$(libdir)
>> +	mkdir -p $(DESTDIR)$(includedir)
>> +	install -m 0755 libedid-decode.so $(DESTDIR)$(libdir)
>> +	install -m 0644 libedid-decode-api.h $(DESTDIR)$(includedir)
> libedid-decode-api.h does not exist yet in this patch.
>
> I find it a little odd to have these targets here without the actual
> files. Maybe the first patch could already have a library building but
> expose just parse and destroy functions without any getters yet?
Yes, this is more like a formatting error. I need to move these files to 
patch 2, where they are introduced.
>> diff --git a/edid-decode.cpp b/edid-decode.cpp
>> index 4a90aba..babff4a 100644
>> --- a/edid-decode.cpp
>> +++ b/edid-decode.cpp
>> @@ -21,7 +21,7 @@
>>   #define STR(x) #x
>>   #define STRING(x) STR(x)
>>   
>> -static edid_state state;
>> +edid_state state;
>>   
>>   static unsigned char edid[EDID_PAGE_SIZE * EDID_MAX_BLOCKS];
>>   static bool odd_hex_digits;
>> @@ -1012,6 +1012,19 @@ static bool extract_edid(int fd, FILE *error)
>>   	state.edid_size = edid_data.size();
>>   	return true;
>>   }
>> +struct edid_state *extract_edid_state(int fd, FILE *error)
>> +{
>> +	bool ret;
>> +
>> +	ret = extract_edid(fd, error);
>> +	if (ret) {
>> +		/* update the number of blocks */
>> +		state.num_blocks = state.edid_size / EDID_PAGE_SIZE;
>> +		return &state;
> A library should not give out pointers to global mutable data. That
> would break having multiple EDIDs loaded at the same time.
>
> I would expect to be able to keep and cache 'struct edid_state'
> instances created by this library until I explicitly destroy them.
> I would not expect parsing a new EDID to overwrite the previously
> returned object. IOW, I would expect to own the object created by the
> library.

Till now, I was under the impression of a design where a compositor 
parses the EDID, and saves all the information in its state immediately, 
so that when the second EDID is loaded, it can override first one. But 
based on your inputs I myself feel that its a bit rigid. Now I am 
thinking about extending it to something which remains until the process 
lifetime. How does this look to you:

- The compositor passes the EDID file node to library.

- The library parses the EDID, creates a state variable and caches it, 
and gives back a handle(unique) to the compositor.

   /* in compositor's display/connector init part */

  connector.handle = libedid_parse_edid(EDID_NODE);


- While calling the subsequent APIs, compositor passes the handle with 
the API, like

  /* Somewhere later in the same compositor */

ret = libedid_is_ycbcr420_supported(connector.handle);

if (ret) {

     /* Prepare a YCBCR420 modeset */

}

and so on .....


This should address your concerns as well.

- Shashank

>
>
> Thanks,
> pq
>
>> +	}
>> +
>> +	return NULL;
>> +}
>>   
>>   static unsigned char crc_calc(const unsigned char *b)
>>   {

Re: [PATCH 2/3] edid-decode: Introduce libedid-decode APIs

[Shashank Sharma]

On 07.03.22 17:11, Pekka Paalanen wrote:
> On Fri,  4 Mar 2022 13:50:00 +0100
> Shashank Sharma <contactshashanksharma@gmail.com> wrote:
>
>> From: Shashank Sharma <shashank.sharma@amd.com>
>>
>> This patch adds a shared library wrapper for edid-decode
>> tool. With this library acting as an interface, other Linux
>> processes would also be able to analyze their EDIDs using the
>> core logic of edid-decode tools.
>>
>> This would be particularly useful for applications like a Compositor
>> who wants to extract the information from an EDID, but doesn't
>> want to add tons of code to do that.
>>
>> The initial version of the library APIs are basic and fundamental to
>> understand the response of the community. The long term plan is to
>> introduce more capable APIs which can:
>> - extract color correction and colorspace capabilities of the display
>>    from their respective CTA-861 blocks.
>> - extract advance information like static and dynamic HDR capabilities,
>>    YCBCR 4:2:0 support, color depth and bpc, max pixel clocks for
>>    HDMI 2.0, 2.1 etc.
>>
>> This infomration will help a display manager or compositor to take
>> several decisions related to display states and modeset.
>>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
>> ---
>>   libedid-decode-api.cpp | 174 +++++++++++++++++++++++++++++++++++++++++
>>   libedid-decode-api.h   |  27 +++++++
> You forgot to add the header in the Makefile as a dependency.
Noted.
>
>
>>   2 files changed, 201 insertions(+)
>>   create mode 100644 libedid-decode-api.cpp
>>   create mode 100644 libedid-decode-api.h
>>
>> diff --git a/libedid-decode-api.cpp b/libedid-decode-api.cpp
>> new file mode 100644
>> index 0000000..ce06ba6
>> --- /dev/null
>> +++ b/libedid-decode-api.cpp
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Author: Shashank Sharma <contactshashanksharma@gmail.com>
>> + */
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +#include "libedid-decode-api.h"
>> +
>> +extern struct edid_state *extract_edid_state(int fd, FILE *error);
>> +
>> +/*
>> + * This is the init function for the API, a user must call
>> + * this function with the EDID file node, to extract the edid
>> + * into a state, and then call the rest of the APIs with that state
>> + * to extract information about EDID.
>> + */
>> +struct edid_state *libedid_parse_edid(const char *edid_path)
>> +{
>> +    int edid_fd, ret;
>> +    struct edid_state *estate;
>> +
>> +    if (!edid_path) {
>> +        printf("No EDID path provided\n");
>> +        return NULL;
>> +    }
>> +
>> +    /* Expecting path to a connector's EDID file like /sys/class/drm/..../edid */
>> +    edid_fd = open(edid_path, O_RDONLY);
>> +    if (edid_fd < 0) {
>> +        printf("Failed to open fd at path %s\n", edid_path);
>> +        return NULL;
>> +    }
>> +
>> +    /* Extract the infomrmation from edid node and prepare it's state */
>> +    estate = extract_edid_state(edid_fd, stderr);
>> +    if (!estate) {
>> +        printf("Failed to extract EDID\n");
>> +        return NULL;
>> +    }
>> +    printf("EDID extracted\n");
>> +
>> +    /* Now parse edid blocks */
>> +    ret = estate->parse_edid();
>> +    if (ret < 0) {
>> +        printf("Error parsing edid, err=%d \n", ret);
>> +        estate = NULL;
>> +    }
>> +
>> +    close(edid_fd);
>> +    return estate;
>> +}
>> +
>> +int libedid_num_blks(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->num_blocks;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
> Can you not simply document and require that 'estate' must be a valid
> pointer created with libedid_parse_edid()?
>
> A library probably should not printf() anything.

Yes, eventually when we have a mature API, I will hide this under a 
compile time debug option, to a file (or may be entirely remove it). Was 
a bit lazy to do it in the initial version :).

> I was just discussing
> how libinput has a context object that you have to pass to every
> function, but in libedid-decode case that context object might as well
> be 'estate'. If there is a reason to log messages, the context object
> carries callback hooks that the user can set.
>
> A context object is also the place to put all the variables that would
> otherwise need to be global. Global variables are not good for
> libraries. Then again, I would not expect a EDID parsing library to
> need any global data, and very few context data. Logging functions at
> most.
Agree, if we move to the new state caching API, this should be managed 
by library layer internally, so no global state sharing.
>
>> +}
>> +
>> +int libedid_has_cta_blks(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->has_cta;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->max_hor_freq_hz;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return 0;
>> +}
>> +
>> +unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->max_vert_freq_hz;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return 0;
>> +}
>> +
>> +unsigned int libedid_get_max_pclk_khz(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->max_pixclk_khz;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return 0;
>> +}
>> +
>> +int libedid_get_edid_version_minor(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->base.edid_minor;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_get_edid_get_num_dtd(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->base.dtd_cnt;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_if_preferred_mode_native(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->base.preferred_is_also_native;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm)
>> +{
>> +    if (estate && hmm && wmm) {
>> +        *hmm = estate->base.max_display_height_mm;
>> +        *wmm = estate->base.max_display_width_mm;
>> +        return 0;
>> +    }
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_ctablk_has_hdmi(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.has_hdmi;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_ctablk_has_vcdb(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.has_vcdb;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_ctablk_has_hfvsdb(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.have_hf_vsdb;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.supported_hdmi_vic_codes;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> \ No newline at end of file
>> diff --git a/libedid-decode-api.h b/libedid-decode-api.h
>> new file mode 100644
>> index 0000000..742b4a4
>> --- /dev/null
>> +++ b/libedid-decode-api.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: MIT
>> + *
>> + * Author: Shashank Sharma <contactshashanksharma@gmail.com>
>> + */
>> +
>> +#ifndef __LIBEDID_DECODE_API_H_
>> +#define __LIBEDID_DECODE_API_H_
>> +
>> +#include "edid-decode.h"
> You cannot include edid-decode.h in this intended-for-public header for
> a couple of reasons:
>
> - It is C++, while the library API/ABI needs to be C.
> - You probably do not want to expose all those details as library ABI.
>
> All the below should be declared inside an extern "C" block when
> compiling as C++.
>
> The most important bit is to document all the public functions so we
> can understand how you expect the API to be used.
Noted, will do these changes.
> Would it be possible to use bool from stdbool.h?
Sure.
>> +
>> +struct edid_state *libedid_parse_edid(const char *edid_path);
> KMS clients get the raw bytes array from the kernel. Wayland
> compositors would have no use for loading a file path, nor do they have
> an fd pointing to the EDID data.
This is interesting dependency, in my understanding core edid-decode is 
written to always read from a file (either stdin or file path), so it 
appeared to be the least-deviation route for me to pass the path of edid 
node itself. If not, I will have to convert the raw-edid-data into a 
file, and pass to edid-decode.
>
> (const void *edid_data, size_t len) would be a more usable signature.
As mentioned above.
>> +int libedid_num_blks(struct edid_state *estate);
>> +int libedid_has_cta_blks(struct edid_state *estate);
> How are these supposed to be used?
>
> Do you expect the user to poke into struct edid_state internals?
> If you don't, then 'struct edid_state' should be an opaque type. Right
> now it's not opaque.

No, of course not. The idea was that:

- a user first parses the edid and gets an edid-state

- user uses this edid-state as a handle for all subsequent calls, to 
extract information using APIs.

It will change with a library API with cachable EDID state, and handle.

>
>> +int libedid_get_edid_version_minor(struct edid_state *estate);
>> +int libedid_get_edid_get_num_dtd(struct edid_state *estate);
>> +int libedid_if_preferred_mode_native(struct edid_state *estate);
>> +int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm);
>> +int libedid_ctablk_has_hdmi(struct edid_state *estate);
>> +int libedid_ctablk_has_vcdb(struct edid_state *estate);
>> +int libedid_ctablk_has_hfvsdb(struct edid_state *estate);
>> +
>> +unsigned int libedid_get_max_pclk_khz(struct edid_state *estate);
>> +unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate);
>> +unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate);
>> +unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate);
>> +
>> +#endif
> Getters are fine, but maybe some of them could be grouped like the max
> display width&height mm are. However, for each getter you have to ask:
> how would the caller use the returned value? If there is no clear idea
> of that, then the getter is not yet needed.
>
> Some immediately useful getters would be for monitor make, model and
> serial. If some of those fields could have multiple different source
> blocks in EDID, then this would also be a nice demonstration of the
> low-level vs. high-level API. In low-level API one might ask if a
> certain block exists, and if so, access its fields. High-level API just
> checks all the places and returns the most accurate information
> available when the caller does not care where it comes from.
>
> How do I destroy the struct edid_state I created?

In this version, there was no dynamic allocation, so it gets overwritten 
with every new EDID node, and last one will destroy with the process.

Now, I will add an exclusive destroy.

- Shashank

>
>
> Thanks,
> pq

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4402 bytes --]

On Mon, 7 Mar 2022 17:48:38 +0100
Shashank Sharma <contactshashanksharma@gmail.com> wrote:

> Hello Pekka,
> 
> On 07.03.22 16:54, Pekka Paalanen wrote:
> > On Fri,  4 Mar 2022 13:49:59 +0100
> > Shashank Sharma <contactshashanksharma@gmail.com> wrote:
> >  
> >> From: Shashank Sharma <shashank.sharma@amd.com>
> >>
> >> This patch does some small changes to make the core logic of
> >> edid-decode tool available to a shared library wrapper. With
> >> these changes, the EDID's 'state' variable will be avialble
> >> to another process via some library API calls.
> >>
> >> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >>
> >> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>  
> > Hi Shashank,
> >
> > thank you very much for working on this!
> >  
> >> ---
> >>   Makefile        | 22 +++++++++++++++++++++-
> >>   edid-decode.cpp | 15 ++++++++++++++-
> >>   2 files changed, 35 insertions(+), 2 deletions(-)

...

> >> diff --git a/edid-decode.cpp b/edid-decode.cpp
> >> index 4a90aba..babff4a 100644
> >> --- a/edid-decode.cpp
> >> +++ b/edid-decode.cpp
> >> @@ -21,7 +21,7 @@
> >>   #define STR(x) #x
> >>   #define STRING(x) STR(x)
> >>   
> >> -static edid_state state;
> >> +edid_state state;
> >>   
> >>   static unsigned char edid[EDID_PAGE_SIZE * EDID_MAX_BLOCKS];
> >>   static bool odd_hex_digits;
> >> @@ -1012,6 +1012,19 @@ static bool extract_edid(int fd, FILE *error)
> >>   	state.edid_size = edid_data.size();
> >>   	return true;
> >>   }
> >> +struct edid_state *extract_edid_state(int fd, FILE *error)
> >> +{
> >> +	bool ret;
> >> +
> >> +	ret = extract_edid(fd, error);
> >> +	if (ret) {
> >> +		/* update the number of blocks */
> >> +		state.num_blocks = state.edid_size / EDID_PAGE_SIZE;
> >> +		return &state;  
> > A library should not give out pointers to global mutable data. That
> > would break having multiple EDIDs loaded at the same time.
> >
> > I would expect to be able to keep and cache 'struct edid_state'
> > instances created by this library until I explicitly destroy them.
> > I would not expect parsing a new EDID to overwrite the previously
> > returned object. IOW, I would expect to own the object created by the
> > library.  
> 
> Till now, I was under the impression of a design where a compositor 
> parses the EDID, and saves all the information in its state immediately, 

It may well be that all compositors will work like that. However, from
a library API design point of view it makes no sense for to require
that. It would be surprising. Surprises lead to bugs.

If you are thinking of optimizing away a few mallocs of few kB of data
for each new EDID to parse, that would be completely premature. Ease of
use wins this one.

> so that when the second EDID is loaded, it can override first one. But 
> based on your inputs I myself feel that its a bit rigid. Now I am 
> thinking about extending it to something which remains until the process 
> lifetime. How does this look to you:
> 
> - The compositor passes the EDID file node to library.

As mentioned, compositors don't have files for EDID.

> 
> - The library parses the EDID, creates a state variable and caches it, 
> and gives back a handle(unique) to the compositor.
> 
>    /* in compositor's display/connector init part */
> 
>   connector.handle = libedid_parse_edid(EDID_NODE);

Why play with handles when you can simply return a pointer to an opaque
type?

I don't see a good reason to make the library more complicated in order
to guard against invalid handles, nor to garbage-collect everything
allocated even if the user of the library forgot to do so. Doing the
latter would just make memory leaks in the callers undetectable when the
library frees them all on exit.

Handles are just not how C works, contrary to what OpenGL tries to make
us think. Handles (that are not just opaque pointers in disguise) do
have their uses, but this does not seem like one.

> - While calling the subsequent APIs, compositor passes the handle with 
> the API, like
> 
>   /* Somewhere later in the same compositor */
> 
> ret = libedid_is_ycbcr420_supported(connector.handle);
> 
> if (ret) {
> 
>      /* Prepare a YCBCR420 modeset */
> 
> }
> 
> and so on .....

That is good.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Hans Verkuil]

Hi Shashank,

There is no cover letter for this series, so I'll just reply to the
first patch, but my comments are high-level and not specific to this
patch.

To be honest, I am not at all convinced that using edid-decode as a
parser library is the right thing to do. It was never written with that
in mind. The two purposes of edid-decode are to:

1) Convert the EDID to a human readable text, and
2) Verify if the EDID conforms to the various standards and is internally
   consistent.

As a result the state information that edid-decode stores is just the
state that it needs to check conformity across Extension Blocks and/or
Data Blocks. Most of the parsed data is just printed to stdout and checked
and then forgotten.

I have considered if it would make sense to make a library to parse and
store the EDID data and have edid-decode sit on top of that, but that will
make the conformity tests much harder. It's kind of interwoven with the
parsing and a parser library is really not interested in that anyway.

I think edid-decode can function very well as a reference source for
a real EDID parser since edid-decode is very complete, but not as a
EDID parser library.

BTW, if anyone has detailed info for the AMD Freesync Data Block, then
I'd love to have that. The cta_amd() function is based on reverse engineering
and not everything could be decoded.

Regards,

	Hans

On 3/4/22 13:49, Shashank Sharma wrote:
> From: Shashank Sharma <shashank.sharma@amd.com>
> 
> This patch does some small changes to make the core logic of
> edid-decode tool available to a shared library wrapper. With
> these changes, the EDID's 'state' variable will be avialble
> to another process via some library API calls.
> 
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
> ---
>  Makefile        | 22 +++++++++++++++++++++-
>  edid-decode.cpp | 15 ++++++++++++++-
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 1843700..ebf3370 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,14 +1,20 @@
>  ifeq ($(OS),Windows_NT)
>  	bindir ?= /usr/bin
>  	mandir ?= /usr/share/man
> +	libdir ?= /usr/lib
> +	includedir ?= /usr/include
>  else
>  	UNAME_S := $(shell uname -s)
>  	ifeq ($(UNAME_S),Darwin)
>  		bindir ?= /usr/local/sbin
>  		mandir ?= /usr/local/share/man
> +		libdir ?= /usr/local/lib
> +		includedir ?= /usr/include
>  	else
>  		bindir ?= /usr/bin
>  		mandir ?= /usr/share/man
> +		libdir ?= /usr/lib
> +		includedir ?= /usr/include
>  	endif
>  endif
>  
> @@ -19,6 +25,11 @@ SOURCES = edid-decode.cpp parse-base-block.cpp parse-cta-block.cpp \
>  	  parse-di-ext-block.cpp parse-vtb-ext-block.cpp calc-gtf-cvt.cpp
>  WARN_FLAGS = -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wimplicit-fallthrough
>  
> +LIB_NAME = libedid-decode.so
> +LIB_FLAGS = -fPIC
> +LIBLINK_FLAGS = -shared
> +LIB_SOURCES = libedid-decode-api.cpp
> +
>  all: edid-decode
>  
>  sha = -DSHA=$(shell if test -d .git ; then git rev-parse --short=12 HEAD ; fi)
> @@ -30,11 +41,20 @@ edid-decode: $(SOURCES) edid-decode.h oui.h Makefile
>  edid-decode.js: $(SOURCES) edid-decode.h oui.h Makefile
>  	$(EMXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) $(sha) $(date) -s EXPORTED_FUNCTIONS='["_parse_edid"]' -s EXTRA_EXPORTED_RUNTIME_METHODS='["ccall", "cwrap"]' -o $@ $(SOURCES) -lm
>  
> +libedid-decode: $(SOURCES) edid-decode.h oui.h Makefile
> +	$(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(LIB_FLAGS) $(sha) $(date) $(LIBLINK_FLAGS) -o $(LIB_NAME) $(LIB_SOURCES) $(SOURCES) -lm
> +
>  clean:
> -	rm -f edid-decode
> +	rm -f edid-decode libedid-decode.so
>  
>  install:
>  	mkdir -p $(DESTDIR)$(bindir)
>  	install -m 0755 edid-decode $(DESTDIR)$(bindir)
>  	mkdir -p $(DESTDIR)$(mandir)/man1
>  	install -m 0644 edid-decode.1 $(DESTDIR)$(mandir)/man1
> +
> +install-lib:
> +	mkdir -p $(DESTDIR)$(libdir)
> +	mkdir -p $(DESTDIR)$(includedir)
> +	install -m 0755 libedid-decode.so $(DESTDIR)$(libdir)
> +	install -m 0644 libedid-decode-api.h $(DESTDIR)$(includedir)
> diff --git a/edid-decode.cpp b/edid-decode.cpp
> index 4a90aba..babff4a 100644
> --- a/edid-decode.cpp
> +++ b/edid-decode.cpp
> @@ -21,7 +21,7 @@
>  #define STR(x) #x
>  #define STRING(x) STR(x)
>  
> -static edid_state state;
> +edid_state state;
>  
>  static unsigned char edid[EDID_PAGE_SIZE * EDID_MAX_BLOCKS];
>  static bool odd_hex_digits;
> @@ -1012,6 +1012,19 @@ static bool extract_edid(int fd, FILE *error)
>  	state.edid_size = edid_data.size();
>  	return true;
>  }
> +struct edid_state *extract_edid_state(int fd, FILE *error)
> +{
> +	bool ret;
> +
> +	ret = extract_edid(fd, error);
> +	if (ret) {
> +		/* update the number of blocks */
> +		state.num_blocks = state.edid_size / EDID_PAGE_SIZE;
> +		return &state;
> +	}
> +
> +	return NULL;
> +}
>  
>  static unsigned char crc_calc(const unsigned char *b)
>  {

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6117 bytes --]

On Tue, 8 Mar 2022 13:09:37 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> Hi Shashank,
> 
> There is no cover letter for this series, so I'll just reply to the
> first patch, but my comments are high-level and not specific to this
> patch.
> 
> To be honest, I am not at all convinced that using edid-decode as a
> parser library is the right thing to do. It was never written with that
> in mind.

Hi Hans,

in https://www.spinics.net/lists/linux-media/msg190064.html you wrote:

	"I would be open to that. The best way would be to create a C
	library that turns the EDID blocks into C structures, while
	edid-decode itself remains C++ and uses the C library to do the
	parsing. While edid-decode supports a large range of Extension
	Blocks, a C library could probably limit itself to the base
	block, CTA-861 blocks and DisplayID blocks."

and

	"I think it would make sense if it is grown as a library used
	by edid-decode. The edid-decode utility is under active
	maintenance and follows the latest EDID standards, so that will
	probably help the quality of the library. My main requirement
	would be that the edid-decode functionality is not affected,
	especially the conformity checks are still performed. And that
	support for new/updated EDID standards can easily be
	implemented, but that's exactly what you would want in an edid
	library."

EDID blocks as C structures is not the API we are looking for from a
library, but more like what edid-decode already prints out yet in
native C types rather than strings or bit patterns. The former could
still be the low-level library API while the latter is the high-level
API. So perhaps edid-decode would be using the low-level API directly.
Then the high-level C API is implemented on top of the low-level C API.
Time would tell how much of edid-decode will move behind the low-level
C API.

On the down-side, the high-level API implementation would need to
duplicate the logic that already(?) exists in edid-decode to find the
most accurate source for a piece of information in case one block
overrides another or information from multiple blocks have to be
combined.

In my opinion this draft does not yet have enough structure to tell
what the interfacing between edid-decode tool and library will look
like.

> The two purposes of edid-decode are to:
> 
> 1) Convert the EDID to a human readable text, and
> 2) Verify if the EDID conforms to the various standards and is internally
>    consistent.
> 
> As a result the state information that edid-decode stores is just the
> state that it needs to check conformity across Extension Blocks and/or
> Data Blocks. Most of the parsed data is just printed to stdout and checked
> and then forgotten.

Sounds like it should be easy to store the data everywhere where
anything is printed. Is something wrong with that? (This would be a
different approach than what you drafted a year ago.)

> I have considered if it would make sense to make a library to parse and
> store the EDID data and have edid-decode sit on top of that, but that will
> make the conformity tests much harder. It's kind of interwoven with the
> parsing and a parser library is really not interested in that anyway.

Why would conformity testing be contradictory to a parsing library?

Does edid-decode just stop when it finds a problem without looking at
the rest of the data, and would doing the latter be somehow difficult?

I would naively think that conformity testing would be easy to make
conditional, or leave it unconditional but redirect the reports when
the user needs to use the information even when it is broken.

The more I think of it, the more I think that display servers should do
EDID conformance testing as part of their normal operations and log the
results. A desktop environment could even have an UI for that: "We
found something strange with your monitor, it might not work as
expected. Details here..." in the more serious cases.

In the long run, maybe it would make people return more monitors to
sellers, which might cause manufacturers to pay more attention to
getting EDID/DisplayID right. I can dream, right? :-)

> I think edid-decode can function very well as a reference source for
> a real EDID parser since edid-decode is very complete, but not as a
> EDID parser library.

It would be a shame to have to fork edid-decode into something else and
then play catch-up with the real edid-decode for all times to come. Or
are you perhaps hoping that the fork would eventually completely
supersede the original project and developers would migrate to the new
one?

It would be really nice to be able to involve the community around
edid-decode to make sure we get the library right, but if the library
is somewhere else, would that happen? Or are we left with yet another
half-written ad hoc EDID parsing code base used by maybe two display
servers?

Maybe we could at least work on this proposal for a while to see what
it will start to look like before dismissing it?

If all that fails and there is still someone left to do some work, it's
not unthinkable to set up a completely new project with the goal to
replicate exactly the output of edid-decode with the full EDID sample
database you have gathered. That just feels like a lot of work without
any help until it's perfect.


Thanks,
pq

> On 3/4/22 13:49, Shashank Sharma wrote:
> > From: Shashank Sharma <shashank.sharma@amd.com>
> > 
> > This patch does some small changes to make the core logic of
> > edid-decode tool available to a shared library wrapper. With
> > these changes, the EDID's 'state' variable will be avialble
> > to another process via some library API calls.
> > 
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > 
> > Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
> > ---
> >  Makefile        | 22 +++++++++++++++++++++-
> >  edid-decode.cpp | 15 ++++++++++++++-
> >  2 files changed, 35 insertions(+), 2 deletions(-)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Hans Verkuil]

Hi Pekka,

On 3/8/22 15:30, Pekka Paalanen wrote:
> On Tue, 8 Mar 2022 13:09:37 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> 
>> Hi Shashank,
>>
>> There is no cover letter for this series, so I'll just reply to the
>> first patch, but my comments are high-level and not specific to this
>> patch.
>>
>> To be honest, I am not at all convinced that using edid-decode as a
>> parser library is the right thing to do. It was never written with that
>> in mind.
> 
> Hi Hans,
> 
> in https://www.spinics.net/lists/linux-media/msg190064.html you wrote:
> 
> 	"I would be open to that. The best way would be to create a C
> 	library that turns the EDID blocks into C structures, while
> 	edid-decode itself remains C++ and uses the C library to do the
> 	parsing. While edid-decode supports a large range of Extension
> 	Blocks, a C library could probably limit itself to the base
> 	block, CTA-861 blocks and DisplayID blocks."
> 
> and
> 
> 	"I think it would make sense if it is grown as a library used
> 	by edid-decode. The edid-decode utility is under active
> 	maintenance and follows the latest EDID standards, so that will
> 	probably help the quality of the library. My main requirement
> 	would be that the edid-decode functionality is not affected,
> 	especially the conformity checks are still performed. And that
> 	support for new/updated EDID standards can easily be
> 	implemented, but that's exactly what you would want in an edid
> 	library."

Yeah, that's written a year ago. I think I was a bit too optimistic.

> 
> EDID blocks as C structures is not the API we are looking for from a
> library, but more like what edid-decode already prints out yet in
> native C types rather than strings or bit patterns. The former could
> still be the low-level library API while the latter is the high-level
> API. So perhaps edid-decode would be using the low-level API directly.
> Then the high-level C API is implemented on top of the low-level C API.
> Time would tell how much of edid-decode will move behind the low-level
> C API.
> 
> On the down-side, the high-level API implementation would need to
> duplicate the logic that already(?) exists in edid-decode to find the
> most accurate source for a piece of information in case one block
> overrides another or information from multiple blocks have to be
> combined.
> 
> In my opinion this draft does not yet have enough structure to tell
> what the interfacing between edid-decode tool and library will look
> like.

I agree. If this can be done in a reasonable way, then I have no objection.
But I will have to see some proof-of-concept code that isn't as trivial as
this.

You also need to think about which information you want to extract from the
EDID. Some things like DI-EXT, LS-EXT and VTB-EXT make no sense today as
it is rarely if ever used.

> 
>> The two purposes of edid-decode are to:
>>
>> 1) Convert the EDID to a human readable text, and
>> 2) Verify if the EDID conforms to the various standards and is internally
>>    consistent.
>>
>> As a result the state information that edid-decode stores is just the
>> state that it needs to check conformity across Extension Blocks and/or
>> Data Blocks. Most of the parsed data is just printed to stdout and checked
>> and then forgotten.
> 
> Sounds like it should be easy to store the data everywhere where
> anything is printed. Is something wrong with that? (This would be a
> different approach than what you drafted a year ago.)

I suspect that the code will become *very* messy.

> 
>> I have considered if it would make sense to make a library to parse and
>> store the EDID data and have edid-decode sit on top of that, but that will
>> make the conformity tests much harder. It's kind of interwoven with the
>> parsing and a parser library is really not interested in that anyway.
> 
> Why would conformity testing be contradictory to a parsing library?

Because some of the things that are tested are e.g. checks if padding
bytes are valid. You don't care about that when you just want to parse
an EDID into a form usable by code.

> 
> Does edid-decode just stop when it finds a problem without looking at
> the rest of the data, and would doing the latter be somehow difficult?

It continues parsing.

> 
> I would naively think that conformity testing would be easy to make
> conditional, or leave it unconditional but redirect the reports when
> the user needs to use the information even when it is broken.
> 
> The more I think of it, the more I think that display servers should do
> EDID conformance testing as part of their normal operations and log the
> results. A desktop environment could even have an UI for that: "We
> found something strange with your monitor, it might not work as
> expected. Details here..." in the more serious cases.

That's a good point.

> 
> In the long run, maybe it would make people return more monitors to
> sellers, which might cause manufacturers to pay more attention to
> getting EDID/DisplayID right. I can dream, right? :-)
> 
>> I think edid-decode can function very well as a reference source for
>> a real EDID parser since edid-decode is very complete, but not as a
>> EDID parser library.
> 
> It would be a shame to have to fork edid-decode into something else and
> then play catch-up with the real edid-decode for all times to come. Or
> are you perhaps hoping that the fork would eventually completely
> supersede the original project and developers would migrate to the new
> one?
> 
> It would be really nice to be able to involve the community around
> edid-decode to make sure we get the library right, but if the library
> is somewhere else, would that happen? Or are we left with yet another
> half-written ad hoc EDID parsing code base used by maybe two display
> servers?
> 
> Maybe we could at least work on this proposal for a while to see what
> it will start to look like before dismissing it?

If you are willing to put in the effort, then I think you would have to
first rework the code bit by bit into different layers:

E.g. parse_base_block() would be split into two functions: a parse_base_block()
that parses the base block into C structures, and it also does the conformity
checks, where the output of that is just written to an internal buffer, as
happens today. The --check-inline option functionality would be hard to support,
I suspect, but I think it is OK to drop that. I at least rarely use it.

And on top of that there is a print_base_block that produces the human
readable output based on the result of the parse_base_block.

Later the parse functions can be put in a library which edid-decode uses.

It should be possible to do this conversion bit by bit, so it's easier to merge
and maintain.

But it is a *lot* of work since you will also have to make C headers for all
the EDID structures.

Can the library be C++ or do you need C structs only? If C++ is OK, then that
will simplify matters.

In any case, I think I would like to see a proof-of-concept where the base
block parsing is modified in such a way as I described above. If that makes
sense, then this can be extended to the other extension blocks. And for the
CTA and DisplayID extension blocks you can probably do the conversion one
Data Block type at a time.

In any case, this series is just not useful as proof-of-concept.

Regards,

	Hans

> If all that fails and there is still someone left to do some work, it's
> not unthinkable to set up a completely new project with the goal to
> replicate exactly the output of edid-decode with the full EDID sample
> database you have gathered. That just feels like a lot of work without
> any help until it's perfect.
> 
> 
> Thanks,
> pq
> 
>> On 3/4/22 13:49, Shashank Sharma wrote:
>>> From: Shashank Sharma <shashank.sharma@amd.com>
>>>
>>> This patch does some small changes to make the core logic of
>>> edid-decode tool available to a shared library wrapper. With
>>> these changes, the EDID's 'state' variable will be avialble
>>> to another process via some library API calls.
>>>
>>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>
>>> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
>>> ---
>>>  Makefile        | 22 +++++++++++++++++++++-
>>>  edid-decode.cpp | 15 ++++++++++++++-
>>>  2 files changed, 35 insertions(+), 2 deletions(-)

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5297 bytes --]

On Tue, 8 Mar 2022 17:36:47 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> Hi Pekka,
> 
> On 3/8/22 15:30, Pekka Paalanen wrote:
> > On Tue, 8 Mar 2022 13:09:37 +0100
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >   
> >> Hi Shashank,
> >>
> >> There is no cover letter for this series, so I'll just reply to the
> >> first patch, but my comments are high-level and not specific to this
> >> patch.
> >>
> >> To be honest, I am not at all convinced that using edid-decode as a
> >> parser library is the right thing to do. It was never written with that
> >> in mind.  
> > 
> > Hi Hans,
> > 
> > in https://www.spinics.net/lists/linux-media/msg190064.html you wrote:

...

> >> I think edid-decode can function very well as a reference source for
> >> a real EDID parser since edid-decode is very complete, but not as a
> >> EDID parser library.  
> > 
> > It would be a shame to have to fork edid-decode into something else and
> > then play catch-up with the real edid-decode for all times to come. Or
> > are you perhaps hoping that the fork would eventually completely
> > supersede the original project and developers would migrate to the new
> > one?
> > 
> > It would be really nice to be able to involve the community around
> > edid-decode to make sure we get the library right, but if the library
> > is somewhere else, would that happen? Or are we left with yet another
> > half-written ad hoc EDID parsing code base used by maybe two display
> > servers?
> > 
> > Maybe we could at least work on this proposal for a while to see what
> > it will start to look like before dismissing it?  
> 
> If you are willing to put in the effort, then I think you would have to
> first rework the code bit by bit into different layers:

Hi Hans,

thanks! If Shashank agrees, we can see how this would start to look
like. I suppose there would be the occasional patch series sent to this
mailing list and publicly discussed between me and Shashank while we
iterate. You could mostly ignore it if you want until the two of us
need your guidance.

Even if it turns out that this cannot go into edid-decode upstream, I
don't think the exercise is going to go to waste. It would be the
beginnings of a new project.

> E.g. parse_base_block() would be split into two functions: a parse_base_block()
> that parses the base block into C structures, and it also does the conformity
> checks, where the output of that is just written to an internal buffer, as
> happens today. The --check-inline option functionality would be hard to support,
> I suspect, but I think it is OK to drop that. I at least rarely use it.

For --check-inline, maybe, maybe not. open_memstream() is a thing,
giving us a FILE*. Depending on --check-inline, the FILE* to complain
to could be either stderr or an internal buffer from open_memstream().
Or the C++ equivalent.

> And on top of that there is a print_base_block that produces the human
> readable output based on the result of the parse_base_block.
> 
> Later the parse functions can be put in a library which edid-decode uses.
> 
> It should be possible to do this conversion bit by bit, so it's easier to merge
> and maintain.
> 
> But it is a *lot* of work since you will also have to make C headers for all
> the EDID structures.

Thank you for the suggestions and warnings. I suspect we shouldn't aim
to land the first part until we have a good idea of the later parts, so
that edid-decode does not end up with half a conversion if the later
parts turn out too messy.

> Can the library be C++ or do you need C structs only? If C++ is OK, then that
> will simplify matters.

The only thing that absolutely must be C is the library public API.
What I've been imagining so far is that we would have a low-level and a
high-level API, as I alluded to in my previous email. Other than that,
I don't know yet.

Internals are totally fine to keep as C++.

> In any case, I think I would like to see a proof-of-concept where the base
> block parsing is modified in such a way as I described above. If that makes
> sense, then this can be extended to the other extension blocks. And for the
> CTA and DisplayID extension blocks you can probably do the conversion one
> Data Block type at a time.
> 
> In any case, this series is just not useful as proof-of-concept.

I agree. A cover letter to set up your expectations would have been in order.

Btw. how does edid-decode regression testing work? I thought I asked in
the past, but I can't find my question or answer. I know what
edid-decode README and test/README says about it, but how does one
actually run through the tests?

One thing I'm a little wary of is the edid-decode.js target in the
Makefile. How do you test that?

On the other hand, if merging into edid-decode does not work, a new
project could have several benefits if I get to decide:

- Meson build system
- automated test suite in the project
- Gitlab workflow hosted by freedesktop.org
- CI

I must admit it is really tempting, but I'm scared of the amount of
work needed to establish a new project.

I assume you are not interested in any of that in the current upstream
project, are you?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Sharma, Shashank]

Hello Hans, Pekka,

Thank you for providing your feedbacks on the first level draft of the 
library, and for your inputs.

On 3/9/2022 3:09 PM, Pekka Paalanen wrote:
> Hi Hans,
> 
> thanks! If Shashank agrees, we can see how this would start to look
> like. I suppose there would be the occasional patch series sent to this
> mailing list and publicly discussed between me and Shashank while we
> iterate. You could mostly ignore it if you want until the two of us
> need your guidance.
> 
> Even if it turns out that this cannot go into edid-decode upstream, I
> don't think the exercise is going to go to waste. It would be the
> beginnings of a new project.

Based on what I could understand from the discussion so far, I could see 
that we have some basic requirements which are suggested by both of you, 
like:

- We want to keep the current structure of EDID-decode as unchanged as 
possible, and want to keep the C++ states internal.
- We want to make sure that the new library (if any) is C API, and apart 
from parsing the EDID, should be independent of EDID-decode core logic.

May I propose something which might be able to keep both the 
expectations maintained upto a certain point, and does solve the purpose 
as well ? Please consider this and let me know how does it sounds:

- We add a C wrapper library with following set of functions:
	- parse_edid_init()
	- query_a_particular_info_from_edid()
	- destroy_edid()
- At init, Client app calls the library parse_edid_init() function with 
EDID (file node or raw data), this is when The library layer allocates a 
C struct for this EDID, which has two parts
	- base block stuff,
	- extension blocks stuff,
- The library calls the internal edid-decode core function just to parse 
EDID, and get the edid-state, and then fills this C structure with all 
the information from edid-state.
- The library caches the C structure for the EDID, and gives user an 
identifier for this EDID.
- At a later stage, when this client tries to extract a particular 
infomration from EDID (like does this display support YCBCR420), the 
library identifies the EDID from cached EDID, and extracts the 
information from cached C struct and responds to the caller API.
- During the display disconnection, client calls and asks the library to 
destroy the EDID structures, and it does.

In this way, this library becomes the CPP->C translation layer, and it 
takes all the overheads like, and will use the edid-decode core APIs 
just for parsing the EDID. The edid-decode state remains internal, used 
immediately, and not being exposed to another process.

Will that be something you guys would like to see as a prototype code ?

- Shashank

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Hans Verkuil]

On 3/9/22 15:09, Pekka Paalanen wrote:
> On Tue, 8 Mar 2022 17:36:47 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> 
>> Hi Pekka,
>>
>> On 3/8/22 15:30, Pekka Paalanen wrote:
>>> On Tue, 8 Mar 2022 13:09:37 +0100
>>> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>   
>>>> Hi Shashank,
>>>>
>>>> There is no cover letter for this series, so I'll just reply to the
>>>> first patch, but my comments are high-level and not specific to this
>>>> patch.
>>>>
>>>> To be honest, I am not at all convinced that using edid-decode as a
>>>> parser library is the right thing to do. It was never written with that
>>>> in mind.  
>>>
>>> Hi Hans,
>>>
>>> in https://www.spinics.net/lists/linux-media/msg190064.html you wrote:
> 
> ...
> 
>>>> I think edid-decode can function very well as a reference source for
>>>> a real EDID parser since edid-decode is very complete, but not as a
>>>> EDID parser library.  
>>>
>>> It would be a shame to have to fork edid-decode into something else and
>>> then play catch-up with the real edid-decode for all times to come. Or
>>> are you perhaps hoping that the fork would eventually completely
>>> supersede the original project and developers would migrate to the new
>>> one?
>>>
>>> It would be really nice to be able to involve the community around
>>> edid-decode to make sure we get the library right, but if the library
>>> is somewhere else, would that happen? Or are we left with yet another
>>> half-written ad hoc EDID parsing code base used by maybe two display
>>> servers?
>>>
>>> Maybe we could at least work on this proposal for a while to see what
>>> it will start to look like before dismissing it?  
>>
>> If you are willing to put in the effort, then I think you would have to
>> first rework the code bit by bit into different layers:
> 
> Hi Hans,
> 
> thanks! If Shashank agrees, we can see how this would start to look
> like. I suppose there would be the occasional patch series sent to this
> mailing list and publicly discussed between me and Shashank while we
> iterate. You could mostly ignore it if you want until the two of us
> need your guidance.

I am generally available on irc (channel #linux-media at irc.oftc.net)
during office hours (CET), so if you want to discuss this a bit more
interactively, then contact me there.

> Even if it turns out that this cannot go into edid-decode upstream, I
> don't think the exercise is going to go to waste. It would be the
> beginnings of a new project.
> 
>> E.g. parse_base_block() would be split into two functions: a parse_base_block()
>> that parses the base block into C structures, and it also does the conformity
>> checks, where the output of that is just written to an internal buffer, as
>> happens today. The --check-inline option functionality would be hard to support,
>> I suspect, but I think it is OK to drop that. I at least rarely use it.
> 
> For --check-inline, maybe, maybe not. open_memstream() is a thing,
> giving us a FILE*. Depending on --check-inline, the FILE* to complain
> to could be either stderr or an internal buffer from open_memstream().
> Or the C++ equivalent.
> 
>> And on top of that there is a print_base_block that produces the human
>> readable output based on the result of the parse_base_block.
>>
>> Later the parse functions can be put in a library which edid-decode uses.
>>
>> It should be possible to do this conversion bit by bit, so it's easier to merge
>> and maintain.
>>
>> But it is a *lot* of work since you will also have to make C headers for all
>> the EDID structures.
> 
> Thank you for the suggestions and warnings. I suspect we shouldn't aim
> to land the first part until we have a good idea of the later parts, so
> that edid-decode does not end up with half a conversion if the later
> parts turn out too messy.

Definitely.

Just to make expectations clear: I'm happy to give advice, and of course review
patches, but I don't have the time to help with the actual coding.

> 
>> Can the library be C++ or do you need C structs only? If C++ is OK, then that
>> will simplify matters.
> 
> The only thing that absolutely must be C is the library public API.
> What I've been imagining so far is that we would have a low-level and a
> high-level API, as I alluded to in my previous email. Other than that,
> I don't know yet.
> 
> Internals are totally fine to keep as C++.

The main reason C++ is used for edid-decode (originally it was written in plain
C) are the STL containers. It's a pain to do that in C.

Creating data structures for the parsed EDID data is definitely going to be
tricky. And remember that e.g. new CTA/DisplayID Data Block types appear
regularly, or new fields are added to existing Data Block types. So this
will need some careful thought.

> 
>> In any case, I think I would like to see a proof-of-concept where the base
>> block parsing is modified in such a way as I described above. If that makes
>> sense, then this can be extended to the other extension blocks. And for the
>> CTA and DisplayID extension blocks you can probably do the conversion one
>> Data Block type at a time.
>>
>> In any case, this series is just not useful as proof-of-concept.
> 
> I agree. A cover letter to set up your expectations would have been in order.
> 
> Btw. how does edid-decode regression testing work? I thought I asked in
> the past, but I can't find my question or answer. I know what
> edid-decode README and test/README says about it, but how does one
> actually run through the tests?

I clone https://github.com/linuxhw/EDID.git, then I have a little create.sh script
located in the checked-out EDID directory:

$ cat create.sh
rm -rf data test x.pl update.sh lst
cp -r ../edid-decode/data .
cp -r ../edid-decode/test .
rm test/README
find Analog Digital data test -type f >lst
cat <<'EOF' >x.pl
while (<>) {
        chomp;
        $f = $_;
        printf("( edid-decode --skip-sha -c -p -n \"$f\" >\"$f.new\" ; mv \"$f.new\" \"$f\" ) &\n");
        if (++$cnt % 5000 == 0) {
                printf("sleep 5;\n");
        }
}
EOF
perl x.pl lst >update.sh

echo >>update.sh
echo 'echo Test for non-ASCII characters:' >>update.sh
echo "git grep '[^ -~]' Analog Digital data" >>update.sh
chmod +x update.sh

rm x.pl lst
------------------------------------------------------

It assumes the edid-decode directory is a sibling directory.

Run this, and it will generate an update.sh script. Then run that in turn
and it will update all EDIDs using the currently installed edid-decode.
Then do 'git add data test' to add the data and test directories, and
'git commit -an' to commit it all (you probably want to make a branch
first).

Then, whenever I make changes to edid-decode I install it and run update.sh
again and check with 'git diff' that the changes are what I expected.

> 
> One thing I'm a little wary of is the edid-decode.js target in the
> Makefile. How do you test that?

Not :-)

Someone else contributed that code, and it worked for him. I really should
try to set something up so I can check it locally.

> 
> On the other hand, if merging into edid-decode does not work, a new
> project could have several benefits if I get to decide:
> 
> - Meson build system
> - automated test suite in the project
> - Gitlab workflow hosted by freedesktop.org
> - CI
> 
> I must admit it is really tempting, but I'm scared of the amount of
> work needed to establish a new project.
> 
> I assume you are not interested in any of that in the current upstream
> project, are you?

It's currently too small of a project for Meson, but if this library thing
becomes a reality, then that makes sense.

Better automated testing is always welcome. I don't want to move it to
freedesktop, mostly because as media kernel developer I do all my work
on linuxtv.org. So as long as I remain maintainer that's unlikely to change.

CI is already done: it's build every day together with the kernel media code
and v4l-utils in my daily build. Results of that are posted on the linux-media
mailinglist.

Regards,

	Hans

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3717 bytes --]

On Wed, 9 Mar 2022 15:31:11 +0100
"Sharma, Shashank" <shashank.sharma@amd.com> wrote:

> Hello Hans, Pekka,
> 
> Thank you for providing your feedbacks on the first level draft of the 
> library, and for your inputs.
> 
> On 3/9/2022 3:09 PM, Pekka Paalanen wrote:
> > Hi Hans,
> > 
> > thanks! If Shashank agrees, we can see how this would start to look
> > like. I suppose there would be the occasional patch series sent to this
> > mailing list and publicly discussed between me and Shashank while we
> > iterate. You could mostly ignore it if you want until the two of us
> > need your guidance.
> > 
> > Even if it turns out that this cannot go into edid-decode upstream, I
> > don't think the exercise is going to go to waste. It would be the
> > beginnings of a new project.  
> 
> Based on what I could understand from the discussion so far, I could see 
> that we have some basic requirements which are suggested by both of you, 
> like:
> 
> - We want to keep the current structure of EDID-decode as unchanged as 
> possible, and want to keep the C++ states internal.
> - We want to make sure that the new library (if any) is C API, and apart 
> from parsing the EDID, should be independent of EDID-decode core logic.
> 
> May I propose something which might be able to keep both the 
> expectations maintained upto a certain point, and does solve the purpose 
> as well ? Please consider this and let me know how does it sounds:
> 
> - We add a C wrapper library with following set of functions:
> 	- parse_edid_init()
> 	- query_a_particular_info_from_edid()
> 	- destroy_edid()
> - At init, Client app calls the library parse_edid_init() function with 
> EDID (file node or raw data), this is when The library layer allocates a 
> C struct for this EDID, which has two parts
> 	- base block stuff,
> 	- extension blocks stuff,
> - The library calls the internal edid-decode core function just to parse 
> EDID, and get the edid-state, and then fills this C structure with all 
> the information from edid-state.
> - The library caches the C structure for the EDID, and gives user an 
> identifier for this EDID.
> - At a later stage, when this client tries to extract a particular 
> infomration from EDID (like does this display support YCBCR420), the 
> library identifies the EDID from cached EDID, and extracts the 
> information from cached C struct and responds to the caller API.
> - During the display disconnection, client calls and asks the library to 
> destroy the EDID structures, and it does.
> 
> In this way, this library becomes the CPP->C translation layer, and it 
> takes all the overheads like, and will use the edid-decode core APIs 
> just for parsing the EDID. The edid-decode state remains internal, used 
> immediately, and not being exposed to another process.
> 
> Will that be something you guys would like to see as a prototype code ?

Hi Shashank,

from what I understood from Hans, edid-state structure just does not
contain most of the information the library would need to deliver. So
this won't work. You cannot just "wrap" edid-decode, because it does
not store all the information it parsed, it only prints it.

Try with the monitor make, model and serial number strings first to see
for yourself, e.g. "Display Product Name" entry.

I do not understand why "identifier for EDID" and searching instead of
just a plain opaque pointer. Like fopen() gives you a FILE* and then
it's up to you to fclose() it when you're done. The FILE* is not an
identifier or a handle, it's a pointer to some data structure whose
contents you must not access directly (the pointer is opaque).


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6613 bytes --]

On Wed, 9 Mar 2022 15:45:29 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> On 3/9/22 15:09, Pekka Paalanen wrote:
> > On Tue, 8 Mar 2022 17:36:47 +0100
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >   
> >> Hi Pekka,
> >>
> >> On 3/8/22 15:30, Pekka Paalanen wrote:  
> >>> On Tue, 8 Mar 2022 13:09:37 +0100
> >>> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>>     
> >>>> Hi Shashank,
> >>>>
> >>>> There is no cover letter for this series, so I'll just reply to the
> >>>> first patch, but my comments are high-level and not specific to this
> >>>> patch.
> >>>>
> >>>> To be honest, I am not at all convinced that using edid-decode as a
> >>>> parser library is the right thing to do. It was never written with that
> >>>> in mind.    
> >>>
> >>> Hi Hans,
> >>>
> >>> in https://www.spinics.net/lists/linux-media/msg190064.html you wrote:  
> > 
> > ...
> >   
> >>>> I think edid-decode can function very well as a reference source for
> >>>> a real EDID parser since edid-decode is very complete, but not as a
> >>>> EDID parser library.    
> >>>
> >>> It would be a shame to have to fork edid-decode into something else and
> >>> then play catch-up with the real edid-decode for all times to come. Or
> >>> are you perhaps hoping that the fork would eventually completely
> >>> supersede the original project and developers would migrate to the new
> >>> one?
> >>>
> >>> It would be really nice to be able to involve the community around
> >>> edid-decode to make sure we get the library right, but if the library
> >>> is somewhere else, would that happen? Or are we left with yet another
> >>> half-written ad hoc EDID parsing code base used by maybe two display
> >>> servers?
> >>>
> >>> Maybe we could at least work on this proposal for a while to see what
> >>> it will start to look like before dismissing it?    
> >>
> >> If you are willing to put in the effort, then I think you would have to
> >> first rework the code bit by bit into different layers:  
> > 
> > Hi Hans,
> > 
> > thanks! If Shashank agrees, we can see how this would start to look
> > like. I suppose there would be the occasional patch series sent to this
> > mailing list and publicly discussed between me and Shashank while we
> > iterate. You could mostly ignore it if you want until the two of us
> > need your guidance.  
> 
> I am generally available on irc (channel #linux-media at irc.oftc.net)
> during office hours (CET), so if you want to discuss this a bit more
> interactively, then contact me there.

Cool, I'm on EET.

> Just to make expectations clear: I'm happy to give advice, and of course review
> patches, but I don't have the time to help with the actual coding.

That is what I was hoping for, thanks!

> The main reason C++ is used for edid-decode (originally it was written in plain
> C) are the STL containers. It's a pain to do that in C.
> 
> Creating data structures for the parsed EDID data is definitely going to be
> tricky. And remember that e.g. new CTA/DisplayID Data Block types appear
> regularly, or new fields are added to existing Data Block types. So this
> will need some careful thought.

Right.

> > Btw. how does edid-decode regression testing work? I thought I asked in
> > the past, but I can't find my question or answer. I know what
> > edid-decode README and test/README says about it, but how does one
> > actually run through the tests?  
> 
> I clone https://github.com/linuxhw/EDID.git, then I have a little create.sh script
> located in the checked-out EDID directory:
> 
> $ cat create.sh
> rm -rf data test x.pl update.sh lst
> cp -r ../edid-decode/data .
> cp -r ../edid-decode/test .
> rm test/README
> find Analog Digital data test -type f >lst
> cat <<'EOF' >x.pl
> while (<>) {
>         chomp;
>         $f = $_;
>         printf("( edid-decode --skip-sha -c -p -n \"$f\" >\"$f.new\" ; mv \"$f.new\" \"$f\" ) &\n");
>         if (++$cnt % 5000 == 0) {
>                 printf("sleep 5;\n");
>         }
> }
> EOF
> perl x.pl lst >update.sh
> 
> echo >>update.sh
> echo 'echo Test for non-ASCII characters:' >>update.sh
> echo "git grep '[^ -~]' Analog Digital data" >>update.sh
> chmod +x update.sh
> 
> rm x.pl lst
> ------------------------------------------------------
> 
> It assumes the edid-decode directory is a sibling directory.
> 
> Run this, and it will generate an update.sh script. Then run that in turn
> and it will update all EDIDs using the currently installed edid-decode.
> Then do 'git add data test' to add the data and test directories, and
> 'git commit -an' to commit it all (you probably want to make a branch
> first).
> 
> Then, whenever I make changes to edid-decode I install it and run update.sh
> again and check with 'git diff' that the changes are what I expected.

Thanks for explaining.

Shashank, I think you need to use this testing procedure routinely to
make sure your patches do not change edid-decode behaviour, at least
with a sub-set of the EDID files.

> > 
> > One thing I'm a little wary of is the edid-decode.js target in the
> > Makefile. How do you test that?  
> 
> Not :-)
> 
> Someone else contributed that code, and it worked for him. I really should
> try to set something up so I can check it locally.

Do you mind if we won't be testing that either?

> > 
> > On the other hand, if merging into edid-decode does not work, a new
> > project could have several benefits if I get to decide:
> > 
> > - Meson build system
> > - automated test suite in the project
> > - Gitlab workflow hosted by freedesktop.org
> > - CI
> > 
> > I must admit it is really tempting, but I'm scared of the amount of
> > work needed to establish a new project.
> > 
> > I assume you are not interested in any of that in the current upstream
> > project, are you?  
> 
> It's currently too small of a project for Meson, but if this library thing
> becomes a reality, then that makes sense.
> 
> Better automated testing is always welcome.

Those are great to hear!

> I don't want to move it to
> freedesktop, mostly because as media kernel developer I do all my work
> on linuxtv.org. So as long as I remain maintainer that's unlikely to change.

Of course.

> CI is already done: it's build every day together with the kernel media code
> and v4l-utils in my daily build. Results of that are posted on the linux-media
> mailinglist.

Nice, but that is after merging patches, right? I was thinking
pre-merge.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Hans Verkuil]

Hi Pekka,

On 3/9/22 16:57, Pekka Paalanen wrote:
> On Wed, 9 Mar 2022 15:45:29 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> 
>> On 3/9/22 15:09, Pekka Paalanen wrote:
>>> On Tue, 8 Mar 2022 17:36:47 +0100
>>> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>   
>>>> Hi Pekka,
>>>>
>>>> On 3/8/22 15:30, Pekka Paalanen wrote:  
>>>>> On Tue, 8 Mar 2022 13:09:37 +0100
>>>>> Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>>>     
>>>>>> Hi Shashank,
>>>>>>
>>>>>> There is no cover letter for this series, so I'll just reply to the
>>>>>> first patch, but my comments are high-level and not specific to this
>>>>>> patch.
>>>>>>
>>>>>> To be honest, I am not at all convinced that using edid-decode as a
>>>>>> parser library is the right thing to do. It was never written with that
>>>>>> in mind.    
>>>>>
>>>>> Hi Hans,
>>>>>
>>>>> in https://www.spinics.net/lists/linux-media/msg190064.html you wrote:  
>>>
>>> ...
>>>   
>>>>>> I think edid-decode can function very well as a reference source for
>>>>>> a real EDID parser since edid-decode is very complete, but not as a
>>>>>> EDID parser library.    
>>>>>
>>>>> It would be a shame to have to fork edid-decode into something else and
>>>>> then play catch-up with the real edid-decode for all times to come. Or
>>>>> are you perhaps hoping that the fork would eventually completely
>>>>> supersede the original project and developers would migrate to the new
>>>>> one?
>>>>>
>>>>> It would be really nice to be able to involve the community around
>>>>> edid-decode to make sure we get the library right, but if the library
>>>>> is somewhere else, would that happen? Or are we left with yet another
>>>>> half-written ad hoc EDID parsing code base used by maybe two display
>>>>> servers?
>>>>>
>>>>> Maybe we could at least work on this proposal for a while to see what
>>>>> it will start to look like before dismissing it?    
>>>>
>>>> If you are willing to put in the effort, then I think you would have to
>>>> first rework the code bit by bit into different layers:  
>>>
>>> Hi Hans,
>>>
>>> thanks! If Shashank agrees, we can see how this would start to look
>>> like. I suppose there would be the occasional patch series sent to this
>>> mailing list and publicly discussed between me and Shashank while we
>>> iterate. You could mostly ignore it if you want until the two of us
>>> need your guidance.  
>>
>> I am generally available on irc (channel #linux-media at irc.oftc.net)
>> during office hours (CET), so if you want to discuss this a bit more
>> interactively, then contact me there.
> 
> Cool, I'm on EET.
> 
>> Just to make expectations clear: I'm happy to give advice, and of course review
>> patches, but I don't have the time to help with the actual coding.
> 
> That is what I was hoping for, thanks!
> 
>> The main reason C++ is used for edid-decode (originally it was written in plain
>> C) are the STL containers. It's a pain to do that in C.
>>
>> Creating data structures for the parsed EDID data is definitely going to be
>> tricky. And remember that e.g. new CTA/DisplayID Data Block types appear
>> regularly, or new fields are added to existing Data Block types. So this
>> will need some careful thought.
> 
> Right.
> 
>>> Btw. how does edid-decode regression testing work? I thought I asked in
>>> the past, but I can't find my question or answer. I know what
>>> edid-decode README and test/README says about it, but how does one
>>> actually run through the tests?  
>>
>> I clone https://github.com/linuxhw/EDID.git, then I have a little create.sh script
>> located in the checked-out EDID directory:
>>
>> $ cat create.sh
>> rm -rf data test x.pl update.sh lst
>> cp -r ../edid-decode/data .
>> cp -r ../edid-decode/test .
>> rm test/README
>> find Analog Digital data test -type f >lst
>> cat <<'EOF' >x.pl
>> while (<>) {
>>         chomp;
>>         $f = $_;
>>         printf("( edid-decode --skip-sha -c -p -n \"$f\" >\"$f.new\" ; mv \"$f.new\" \"$f\" ) &\n");
>>         if (++$cnt % 5000 == 0) {
>>                 printf("sleep 5;\n");
>>         }
>> }
>> EOF
>> perl x.pl lst >update.sh
>>
>> echo >>update.sh
>> echo 'echo Test for non-ASCII characters:' >>update.sh
>> echo "git grep '[^ -~]' Analog Digital data" >>update.sh
>> chmod +x update.sh
>>
>> rm x.pl lst
>> ------------------------------------------------------
>>
>> It assumes the edid-decode directory is a sibling directory.
>>
>> Run this, and it will generate an update.sh script. Then run that in turn
>> and it will update all EDIDs using the currently installed edid-decode.
>> Then do 'git add data test' to add the data and test directories, and
>> 'git commit -an' to commit it all (you probably want to make a branch
>> first).
>>
>> Then, whenever I make changes to edid-decode I install it and run update.sh
>> again and check with 'git diff' that the changes are what I expected.
> 
> Thanks for explaining.
> 
> Shashank, I think you need to use this testing procedure routinely to
> make sure your patches do not change edid-decode behaviour, at least
> with a sub-set of the EDID files.
> 
>>>
>>> One thing I'm a little wary of is the edid-decode.js target in the
>>> Makefile. How do you test that?  
>>
>> Not :-)
>>
>> Someone else contributed that code, and it worked for him. I really should
>> try to set something up so I can check it locally.
> 
> Do you mind if we won't be testing that either?

That's fine.

> 
>>>
>>> On the other hand, if merging into edid-decode does not work, a new
>>> project could have several benefits if I get to decide:
>>>
>>> - Meson build system
>>> - automated test suite in the project
>>> - Gitlab workflow hosted by freedesktop.org
>>> - CI
>>>
>>> I must admit it is really tempting, but I'm scared of the amount of
>>> work needed to establish a new project.
>>>
>>> I assume you are not interested in any of that in the current upstream
>>> project, are you?  
>>
>> It's currently too small of a project for Meson, but if this library thing
>> becomes a reality, then that makes sense.
>>
>> Better automated testing is always welcome.
> 
> Those are great to hear!
> 
>> I don't want to move it to
>> freedesktop, mostly because as media kernel developer I do all my work
>> on linuxtv.org. So as long as I remain maintainer that's unlikely to change.
> 
> Of course.
> 
>> CI is already done: it's build every day together with the kernel media code
>> and v4l-utils in my daily build. Results of that are posted on the linux-media
>> mailinglist.
> 
> Nice, but that is after merging patches, right? I was thinking
> pre-merge.

Not sure what useful pre-merge testing can be done other than just running 'make' :-)

Regards,

	Hans

> 
> 
> Thanks,
> pq

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Wed, 9 Mar 2022 17:00:37 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> Hi Pekka,
> 
> On 3/9/22 16:57, Pekka Paalanen wrote:
> > On Wed, 9 Mar 2022 15:45:29 +0100
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >   

...

> >> CI is already done: it's build every day together with the kernel media code
> >> and v4l-utils in my daily build. Results of that are posted on the linux-media
> >> mailinglist.  
> > 
> > Nice, but that is after merging patches, right? I was thinking
> > pre-merge.  
> 
> Not sure what useful pre-merge testing can be done other than just running 'make' :-)

To ensure that patches that are not intended to change the output
indeed do not change the output, or do not introduce crashes. Other
popular tests are checking for compiler warnings against a specific
compiler version, and commit messages e.g. for S-o-b if that's used.

For example wayland-scanner (a code generator) has tests in the Wayland
test suite that verify the output for certain test inputs does not
change. The test inputs and reference outputs are committed into git.
If a patch intentionally changes the output, then that patch also
includes changes to the reference output files. Simply reading a patch
will also show how the output changed.

But your test corpus is huge, and this method does not scale up to that
directly. You could maybe have a few chosen test EDIDs for this, but
running the full corpus needs something quite different. Maybe the full
corpus is best the way you do it now.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

On Tue, 8 Mar 2022 13:09:37 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> Hi Shashank,
> 
> There is no cover letter for this series, so I'll just reply to the
> first patch, but my comments are high-level and not specific to this
> patch.
> 
> To be honest, I am not at all convinced that using edid-decode as a
> parser library is the right thing to do. It was never written with that
> in mind. The two purposes of edid-decode are to:
> 
> 1) Convert the EDID to a human readable text, and
> 2) Verify if the EDID conforms to the various standards and is internally
>    consistent.
> 
> As a result the state information that edid-decode stores is just the
> state that it needs to check conformity across Extension Blocks and/or
> Data Blocks. Most of the parsed data is just printed to stdout and checked
> and then forgotten.
> 
> I have considered if it would make sense to make a library to parse and
> store the EDID data and have edid-decode sit on top of that, but that will
> make the conformity tests much harder. It's kind of interwoven with the
> parsing and a parser library is really not interested in that anyway.
> 
> I think edid-decode can function very well as a reference source for
> a real EDID parser since edid-decode is very complete, but not as a
> EDID parser library.
> 

Hi all,

since this discussion, more people have joined the effort and the
consensus became to start a new project instead of pushing to
edid-decode upstream.

The new project is at
https://gitlab.freedesktop.org/emersion/libdisplay-info
where we are currently discussing what the API should look like in the
issues and merge request comments, so there is essentially no code yet.

If the project gains enough traction, I expect it will be moved into a
non-personal namespace under fd.o to live a life of its own.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]