Re: [igt-dev] [PATCH i-g-t 3/3] lib/igt_chamelium: replace EDID IDs with opaque structs

Martin Peres
2019-06-17T07:21:35Z

On 14/06/2019 17:22, Simon Ser wrote:
> There are two reasons for this:
>
> * An opaque struct is more type-safe than an int
> * In the future we'll want to tag EDIDs with the Chamelium port ID, to be able
> to automagically infer the DRM connector <-> Chamelium port mapping. This is
> necessary for DP MST (connector names are not stable).
>
> Signed-off-by: Simon Ser <simon.ser@intel.com>

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>

> ---
> lib/igt_chamelium.c | 25 ++++++++++++-------------
> lib/igt_chamelium.h | 8 ++++++--
> tests/kms_chamelium.c | 2 +-
> 3 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 0c44e56cad86..b83ff395d44b 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -530,13 +530,13 @@ void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
> * Uploads and registers a new EDID with the chamelium. The EDID will be
> * destroyed automatically when #chamelium_deinit is called.
> *
> - * Returns: The ID of the EDID uploaded to the chamelium.
> + * Returns: An opaque pointer to the Chamelium EDID
> */
> -int chamelium_new_edid(struct chamelium *chamelium,
> - const unsigned char *raw_edid)
> +struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
> + const unsigned char *raw_edid)
> {
> xmlrpc_value *res;
> - struct chamelium_edid *allocated_edid;
> + struct chamelium_edid *chamelium_edid;
> int edid_id;
> struct edid *edid = (struct edid *) raw_edid;
> size_t edid_size = sizeof(struct edid) +
> @@ -548,14 +548,12 @@ int chamelium_new_edid(struct chamelium *chamelium,
> xmlrpc_read_int(&chamelium->env, res, &edid_id);
> xmlrpc_DECREF(res);
>
> - allocated_edid = malloc(sizeof(struct chamelium_edid));
> - memset(allocated_edid, 0, sizeof(*allocated_edid));
> + chamelium_edid = calloc(1, sizeof(struct chamelium_edid));
> + chamelium_edid->id = edid_id;
>
> - allocated_edid->id = edid_id;
> + igt_list_add(&chamelium_edid->link, &chamelium->edids);
>
> - igt_list_add(&allocated_edid->link, &chamelium->edids);
> -
> - return edid_id;
> + return chamelium_edid;
> }
>
> static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
> @@ -568,7 +566,7 @@ static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
> * chamelium_port_set_edid:
> * @chamelium: The Chamelium instance to use
> * @port: The port on the Chamelium to set the EDID on
> - * @edid_id: The ID of an EDID on the chamelium created with
> + * @edid: The Chamelium EDID to set
> * #chamelium_new_edid, or 0 to disable the EDID on the port
> *
> * Sets a port on the chamelium to use the specified EDID. This does not fire a
> @@ -578,10 +576,11 @@ static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
> * change.
> */
> void chamelium_port_set_edid(struct chamelium *chamelium,
> - struct chamelium_port *port, int edid_id)
> + struct chamelium_port *port,
> + struct chamelium_edid *edid)
> {
> xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "ApplyEdid", "(ii)",
> - port->id, edid_id));
> + port->id, edid->id));
> }
>
> /**
> diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> index 28f726c91e83..ce9e9ced75d9 100644
> --- a/lib/igt_chamelium.h
> +++ b/lib/igt_chamelium.h
> @@ -59,6 +59,8 @@ struct chamelium_audio_file {
> int channels;
> };
>
> +struct chamelium_edid;
> +
> /**
> * CHAMELIUM_DEFAULT_EDID: provide this ID to #chamelium_port_set_edid to use
> * the default EDID.
> @@ -98,9 +100,11 @@ void chamelium_fire_hpd_pulses(struct chamelium *chamelium,
> void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
> struct chamelium_port *port, int delay_ms,
> bool rising_edge);
> -int chamelium_new_edid(struct chamelium *chamelium, const unsigned char *edid);
> +struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
> + const unsigned char *edid);
> void chamelium_port_set_edid(struct chamelium *chamelium,
> - struct chamelium_port *port, int edid_id);
> + struct chamelium_port *port,
> + struct chamelium_edid *edid);
> bool chamelium_port_get_ddc_state(struct chamelium *chamelium,
> struct chamelium_port *port);
> void chamelium_port_set_ddc_state(struct chamelium *chamelium,
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 03ebbd9c4b20..18f504f3bcad 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -51,7 +51,7 @@ typedef struct {
>
> int drm_fd;
>
> - int edids[TEST_EDID_COUNT];
> + struct chamelium_edid *edids[TEST_EDID_COUNT];
> } data_t;
>
> #define HOTPLUG_TIMEOUT 20 /* seconds */
>