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

[Pekka Paalanen <ppaalanen@gmail.com>]

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

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.


>  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. 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.

> +}
> +
> +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.

Would it be possible to use bool from stdbool.h?

> +
> +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.

(const void *edid_data, size_t len) would be a more usable signature.

> +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.

> +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?


Thanks,
pq

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