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

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

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