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