Re: [PATCH 2/3] edid-decode: Introduce libedid-decode APIs

[Shashank Sharma <contactshashanksharma@gmail.com>]

On 07.03.22 17:11, Pekka Paalanen wrote:
> On Fri,  4 Mar 2022 13:50:00 +0100
> Shashank Sharma <contactshashanksharma@gmail.com> wrote:
>
>> From: Shashank Sharma <shashank.sharma@amd.com>
>>
>> This patch adds a shared library wrapper for edid-decode
>> tool. With this library acting as an interface, other Linux
>> processes would also be able to analyze their EDIDs using the
>> core logic of edid-decode tools.
>>
>> This would be particularly useful for applications like a Compositor
>> who wants to extract the information from an EDID, but doesn't
>> want to add tons of code to do that.
>>
>> The initial version of the library APIs are basic and fundamental to
>> understand the response of the community. The long term plan is to
>> introduce more capable APIs which can:
>> - extract color correction and colorspace capabilities of the display
>>    from their respective CTA-861 blocks.
>> - extract advance information like static and dynamic HDR capabilities,
>>    YCBCR 4:2:0 support, color depth and bpc, max pixel clocks for
>>    HDMI 2.0, 2.1 etc.
>>
>> This infomration will help a display manager or compositor to take
>> several decisions related to display states and modeset.
>>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
>> ---
>>   libedid-decode-api.cpp | 174 +++++++++++++++++++++++++++++++++++++++++
>>   libedid-decode-api.h   |  27 +++++++
> You forgot to add the header in the Makefile as a dependency.
Noted.
>
>
>>   2 files changed, 201 insertions(+)
>>   create mode 100644 libedid-decode-api.cpp
>>   create mode 100644 libedid-decode-api.h
>>
>> diff --git a/libedid-decode-api.cpp b/libedid-decode-api.cpp
>> new file mode 100644
>> index 0000000..ce06ba6
>> --- /dev/null
>> +++ b/libedid-decode-api.cpp
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Author: Shashank Sharma <contactshashanksharma@gmail.com>
>> + */
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +#include "libedid-decode-api.h"
>> +
>> +extern struct edid_state *extract_edid_state(int fd, FILE *error);
>> +
>> +/*
>> + * This is the init function for the API, a user must call
>> + * this function with the EDID file node, to extract the edid
>> + * into a state, and then call the rest of the APIs with that state
>> + * to extract information about EDID.
>> + */
>> +struct edid_state *libedid_parse_edid(const char *edid_path)
>> +{
>> +    int edid_fd, ret;
>> +    struct edid_state *estate;
>> +
>> +    if (!edid_path) {
>> +        printf("No EDID path provided\n");
>> +        return NULL;
>> +    }
>> +
>> +    /* Expecting path to a connector's EDID file like /sys/class/drm/..../edid */
>> +    edid_fd = open(edid_path, O_RDONLY);
>> +    if (edid_fd < 0) {
>> +        printf("Failed to open fd at path %s\n", edid_path);
>> +        return NULL;
>> +    }
>> +
>> +    /* Extract the infomrmation from edid node and prepare it's state */
>> +    estate = extract_edid_state(edid_fd, stderr);
>> +    if (!estate) {
>> +        printf("Failed to extract EDID\n");
>> +        return NULL;
>> +    }
>> +    printf("EDID extracted\n");
>> +
>> +    /* Now parse edid blocks */
>> +    ret = estate->parse_edid();
>> +    if (ret < 0) {
>> +        printf("Error parsing edid, err=%d \n", ret);
>> +        estate = NULL;
>> +    }
>> +
>> +    close(edid_fd);
>> +    return estate;
>> +}
>> +
>> +int libedid_num_blks(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->num_blocks;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
> Can you not simply document and require that 'estate' must be a valid
> pointer created with libedid_parse_edid()?
>
> A library probably should not printf() anything.

Yes, eventually when we have a mature API, I will hide this under a 
compile time debug option, to a file (or may be entirely remove it). Was 
a bit lazy to do it in the initial version :).

> I was just discussing
> how libinput has a context object that you have to pass to every
> function, but in libedid-decode case that context object might as well
> be 'estate'. If there is a reason to log messages, the context object
> carries callback hooks that the user can set.
>
> A context object is also the place to put all the variables that would
> otherwise need to be global. Global variables are not good for
> libraries. Then again, I would not expect a EDID parsing library to
> need any global data, and very few context data. Logging functions at
> most.
Agree, if we move to the new state caching API, this should be managed 
by library layer internally, so no global state sharing.
>
>> +}
>> +
>> +int libedid_has_cta_blks(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->has_cta;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->max_hor_freq_hz;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return 0;
>> +}
>> +
>> +unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->max_vert_freq_hz;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return 0;
>> +}
>> +
>> +unsigned int libedid_get_max_pclk_khz(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->max_pixclk_khz;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return 0;
>> +}
>> +
>> +int libedid_get_edid_version_minor(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->base.edid_minor;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_get_edid_get_num_dtd(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->base.dtd_cnt;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_if_preferred_mode_native(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->base.preferred_is_also_native;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm)
>> +{
>> +    if (estate && hmm && wmm) {
>> +        *hmm = estate->base.max_display_height_mm;
>> +        *wmm = estate->base.max_display_width_mm;
>> +        return 0;
>> +    }
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_ctablk_has_hdmi(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.has_hdmi;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_ctablk_has_vcdb(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.has_vcdb;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +int libedid_ctablk_has_hfvsdb(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.have_hf_vsdb;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> +
>> +unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate)
>> +{
>> +    if (estate)
>> +        return estate->cta.supported_hdmi_vic_codes;
>> +
>> +    printf("EDID state not initialized\n");
>> +    return -1;
>> +}
>> \ No newline at end of file
>> diff --git a/libedid-decode-api.h b/libedid-decode-api.h
>> new file mode 100644
>> index 0000000..742b4a4
>> --- /dev/null
>> +++ b/libedid-decode-api.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: MIT
>> + *
>> + * Author: Shashank Sharma <contactshashanksharma@gmail.com>
>> + */
>> +
>> +#ifndef __LIBEDID_DECODE_API_H_
>> +#define __LIBEDID_DECODE_API_H_
>> +
>> +#include "edid-decode.h"
> You cannot include edid-decode.h in this intended-for-public header for
> a couple of reasons:
>
> - It is C++, while the library API/ABI needs to be C.
> - You probably do not want to expose all those details as library ABI.
>
> All the below should be declared inside an extern "C" block when
> compiling as C++.
>
> The most important bit is to document all the public functions so we
> can understand how you expect the API to be used.
Noted, will do these changes.
> Would it be possible to use bool from stdbool.h?
Sure.
>> +
>> +struct edid_state *libedid_parse_edid(const char *edid_path);
> KMS clients get the raw bytes array from the kernel. Wayland
> compositors would have no use for loading a file path, nor do they have
> an fd pointing to the EDID data.
This is interesting dependency, in my understanding core edid-decode is 
written to always read from a file (either stdin or file path), so it 
appeared to be the least-deviation route for me to pass the path of edid 
node itself. If not, I will have to convert the raw-edid-data into a 
file, and pass to edid-decode.
>
> (const void *edid_data, size_t len) would be a more usable signature.
As mentioned above.
>> +int libedid_num_blks(struct edid_state *estate);
>> +int libedid_has_cta_blks(struct edid_state *estate);
> How are these supposed to be used?
>
> Do you expect the user to poke into struct edid_state internals?
> If you don't, then 'struct edid_state' should be an opaque type. Right
> now it's not opaque.

No, of course not. The idea was that:

- a user first parses the edid and gets an edid-state

- user uses this edid-state as a handle for all subsequent calls, to 
extract information using APIs.

It will change with a library API with cachable EDID state, and handle.

>
>> +int libedid_get_edid_version_minor(struct edid_state *estate);
>> +int libedid_get_edid_get_num_dtd(struct edid_state *estate);
>> +int libedid_if_preferred_mode_native(struct edid_state *estate);
>> +int libedid_get_max_display_w_h_mm(struct edid_state *estate, int *wmm, int *hmm);
>> +int libedid_ctablk_has_hdmi(struct edid_state *estate);
>> +int libedid_ctablk_has_vcdb(struct edid_state *estate);
>> +int libedid_ctablk_has_hfvsdb(struct edid_state *estate);
>> +
>> +unsigned int libedid_get_max_pclk_khz(struct edid_state *estate);
>> +unsigned int libedid_get_max_hfreq_hz(struct edid_state *estate);
>> +unsigned int libedid_get_max_vfreq_hz(struct edid_state *estate);
>> +unsigned int libedid_ctablk_supported_hdmi_vics(struct edid_state *estate);
>> +
>> +#endif
> Getters are fine, but maybe some of them could be grouped like the max
> display width&height mm are. However, for each getter you have to ask:
> how would the caller use the returned value? If there is no clear idea
> of that, then the getter is not yet needed.
>
> Some immediately useful getters would be for monitor make, model and
> serial. If some of those fields could have multiple different source
> blocks in EDID, then this would also be a nice demonstration of the
> low-level vs. high-level API. In low-level API one might ask if a
> certain block exists, and if so, access its fields. High-level API just
> checks all the places and returns the most accurate information
> available when the caller does not care where it comes from.
>
> How do I destroy the struct edid_state I created?

In this version, there was no dynamic allocation, so it gets overwritten 
with every new EDID node, and last one will destroy with the process.

Now, I will add an exclusive destroy.

- Shashank

>
>
> Thanks,
> pq