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

[Shashank Sharma <contactshashanksharma@gmail.com>]

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