Re: [igt-dev] [PATCH i-g-t v2 1/3] Make basic chamelium function accessible to other tests.

Petri Latvala
2020-03-11T08:23:33Z

On Mon, Jan 20, 2020 at 09:50:25AM +0530, Kunal Joshi wrote:
> There are many use case where we can integrate chamelium with other tests,
> Migrating some of basic chamelium functions to igt_chamelium lib to avoid
> Code rewriting.
>
> v2: Moved one more function reset_state from tests/kms_chamelium to
> lib/igt_chamelium.
>
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> ---
> lib/igt_chamelium.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++
> lib/igt_chamelium.h | 28 +++++++++++++++++
> tests/kms_chamelium.c | 81 +-----------------------------------------------
> 3 files changed, 114 insertions(+), 80 deletions(-)
>
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 9971f51..8b910bf 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -132,6 +132,91 @@ static struct chamelium *cleanup_instance;
> static void chamelium_do_calculate_fb_crc(cairo_surface_t *fb_surface,
> igt_crc_t *out);
>
> +void
> +require_displayport_connector_present(struct data_chamelium_t *data,
> + unsigned int type)
> +{
> + int i, count = 0;
> + bool found = false;
> +
> + for (i = 0; i < data->port_count && !found; i++) {
> + if (chamelium_port_get_type(data->ports[i]) == type)
> + count++;
> + if (count == 2)
> + found = true;
> + }
> + igt_require_f(found,
> + "Need atleast 2 ports of type %s connected, found %d\n",
> + kmstest_connector_type_str(type), count);
> +}

For a helper, this could use some generalizing. It's called "require
displayport connector" but requires exactly 2 connectors of a given
type, not displayport. The name also needs to be prefixed with
chamelium_. chamelium_require_connectors(chamelium, type, count)?

Comment below for data_chamelium_t usage.

> +
> +drmModeConnection
> +reprobe_connector(struct data_chamelium_t *data, struct chamelium_port *port)
> +{
> + drmModeConnector *connector;
> + drmModeConnection status;
> +
> + igt_debug("Reprobing %s...\n", chamelium_port_get_name(port));
> + connector = chamelium_port_get_connector(data->chamelium, port, true);
> + igt_assert(connector);
> + status = connector->connection;
> +
> + drmModeFreeConnector(connector);
> + return status;
> +}

As a library function, this functions looks redundant. Just keep
copies of that function in the tests IMHO.

> +
> +const char *connection_str(drmModeConnection c)
> +{
> + switch (c) {
> + case DRM_MODE_CONNECTED:
> + return "connected";
> + case DRM_MODE_DISCONNECTED:
> + return "disconnected";
> + case DRM_MODE_UNKNOWNCONNECTION:
> + return "unknown";
> + }
> + assert(0); /* unreachable */
> +}

Isn't this just kmstest_connector_status_str()?

> +
> +void
> +wait_for_connector(struct data_chamelium_t *data, struct chamelium_port *port,
> + drmModeConnection status)
> +{
> + igt_debug("Waiting for %s to get %s...\n",
> + chamelium_port_get_name(port), connection_str(status));
> +
> + /*
> + * Rely on simple reprobing so we don't fail tests that don't require
> + * that hpd events work in the event that hpd doesn't work on the system
> + */
> + igt_until_timeout(HOTPLUG_TIMEOUT) {
> + if (reprobe_connector(data, port) == status)
> + return;
> +
> + usleep(50000);
> +
> + }
> + igt_assert_f(false, "Timed out waiting for %s to get %s\n",
> + chamelium_port_get_name(port), connection_str(status));
> +}
> +
> +void
> +reset_state(struct data_chamelium_t *data, struct chamelium_port *port)
> +{
> + int p;
> +
> + chamelium_reset(data->chamelium);
> +
> + if (port) {
> + wait_for_connector(data, port, DRM_MODE_DISCONNECTED);
> + } else {
> + for (p = 0; p < data->port_count; p++) {
> + port = data->ports[p];
> + wait_for_connector(data, port, DRM_MODE_DISCONNECTED);
> + }
> + }
> +}
> +

I'm fairly certain we already have helpers for doing these operations,
or if we don't, we don't need them in lib.

> /**
> * chamelium_get_ports:
> * @chamelium: The Chamelium instance to use
> diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> index 08705a9..71080f4 100644
> --- a/lib/igt_chamelium.h
> +++ b/lib/igt_chamelium.h
> @@ -25,6 +25,8 @@
>
> #ifndef IGT_CHAMELIUM_H
> #define IGT_CHAMELIUM_H
> +#define TEST_EDID_COUNT 5
> +#define HOTPLUG_TIMEOUT 20
>
> #include "config.h"
>
> @@ -32,6 +34,7 @@
> #include <xf86drmMode.h>
>
> #include "igt_debugfs.h"
> +#include "igt_kms.h"
>
> struct igt_fb;
> struct edid;
> @@ -81,6 +84,17 @@ struct chamelium_infoframe {
>
> struct chamelium_edid;
>
> +struct data_chamelium_t {
> + struct chamelium *chamelium;
> + struct chamelium_port **ports;
> + igt_display_t display;
> + int port_count;
> +
> + int drm_fd;
> +
> + struct chamelium_edid *edids[TEST_EDID_COUNT];
> +};
> +

This struct is entirely tests/kms_chamelium.c specific. Make the new
lib helpers take a struct chamelium* instead. Why kms_chamelium even
has this intermediary storage for all this data is beyond my expertise
but I'm not seeing it.

From a quick look, the only thing the new lib functions would need is
access to the chamelium ports, and they can be retrieved with
chamelium_get_ports().

CC'ing Arek for comments with his chamelium lib expertise.

--
Petri Latvala