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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3717 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]