| Message ID | 20220304125001.1732057-1-contactshashanksharma@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] edid-decode: Introduce libedid-decode wrapper | expand |
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) > {
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) >> {
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
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) > {
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(-)
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(-)
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
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
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
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
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
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
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
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
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) {