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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- 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 --]