On Tue, 8 Mar 2022 13:09:37 +0100 Hans Verkuil <hverkuil-cisco@xxxxxxxxx> 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@xxxxxxx> > > > > 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@xxxxxxxxx> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > > > > Signed-off-by: Shashank Sharma <contactshashanksharma@xxxxxxxxx> > > --- > > Makefile | 22 +++++++++++++++++++++- > > edid-decode.cpp | 15 ++++++++++++++- > > 2 files changed, 35 insertions(+), 2 deletions(-)
Attachment:
pgpSbR9IdPWs7.pgp
Description: OpenPGP digital signature
