[PATCH i-g-t v3 1/5] lib/igt_kms: Add a detect timeout value

[Kamil Konieczny]

Hi Louis,
On 2024-11-22 at 16:19:04 +0100, Louis Chauvet wrote:

sorry for late response.

> Some tests need to wait for a specific connector status. In order to make
> the timeout customisable for each target, add an option in the
> configuration file.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet at bootlin.com>

I changed Bhanu address to: Bhanuprakash Modem <bhanuprakash.modem at gmail.com>
Also +cc Swati and Khartik
Cc: Swati Sharma <swati2.sharma at intel.com>
Cc: Karthik B S <karthik.b.s at intel.com>

> ---
>  lib/igt_core.c |  3 +++
>  lib/igt_kms.c  | 33 +++++++++++++++++++++++++++++++++
>  lib/igt_kms.h  |  9 +++++++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 407f7b55187ceac4cbc0645c629a7fbb17f89575..22d80a1497a9faca85def70d688427cd7d553d60 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -265,6 +265,9 @@
>   *	# It is not mandatory and allows overriding default values.
>   *	[DUT]
>   *	SuspendResumeDelay=10

Add newline here to separate this:

    *

> + *	# The following option define the timeout for detection feature
> + *	# (waiting for a connector status)
> + *	DisplayDetectTimeout=10.0
>   * ]|
>   *
>   * Some specific configuration options may be used by specific parts of IGT,
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 5d8096a17596ca592a274800140804edec6d7422..0b76a47eb2135cca21881d97c2fd78e724d9a30e 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -58,6 +58,7 @@
>  #include "intel_chipset.h"
>  #include "igt_debugfs.h"
>  #include "igt_device.h"
> +#include "igt_rc.h"
>  #include "igt_sysfs.h"
>  #include "sw_sync.h"
>  #ifdef HAVE_CHAMELIUM
> @@ -7185,3 +7186,35 @@ int igt_backlight_write(int value, const char *fname, igt_backlight_context_t *c
>  
>  	return 0;
>  }
> +
> +/**
> + * igt_default_detect_timeout:
> + *
> + * Get the default timeout value for detection feature
> + *
> + * Some tests requires to wait for a specific connector status. This value will determine the
> + * timeout value for this waiting.
> + */
> +float igt_default_detect_timeout(void)

Please rename to igt_default_display_detect_timeout(void)

Why not returning double?

> +{
> +	static double timeout = 0.0;
> +	static bool first_call = true;
> +	GError *error = NULL;
> +
> +	if (first_call) {
> +		if (igt_key_file) {
> +			timeout = g_key_file_get_double(igt_key_file, "DUT", "DisplayDetectTimeout", &error);
> +			if (error) {
> +				igt_warn("Failed to read DisplayDetectTimeout, defaulting to %f\n", DEFAULT_DETECT_TIMEOUT);

Why warn? Is this mandatory?

> +				g_clear_error(&error);
> +				timeout = DEFAULT_DETECT_TIMEOUT;
> +			}
> +		} else {
> +			timeout = DEFAULT_DETECT_TIMEOUT;
> +		}
> +
> +		first_call = false;
> +	}
> +
> +	return timeout;
> +}
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index bd154d1c19e9f36ec61f29d953df149b0642465d..3509283b6da5b3fdbd70ebdc9bdd241326bc5073 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -40,6 +40,13 @@
>  #include "igt_fb.h"
>  #include "ioctl_wrappers.h"
>  
> +/**
> + * define DEFAULT_DETECT_TIMEOUT - Default timeout used for some detection functions

Please add also quontities used, I guess it is seconds but write
it down.

> + *
> + * It can be overiden by option DetectTimeout in the .igtrc file.
> + */
> +#define DEFAULT_DETECT_TIMEOUT 10.0

Is it ok to have 10 seconds as a default?
Overall looks good.

Regards,
Kamil

> +
>  /* Low-level helpers with kmstest_ prefix */
>  
>  /**
> @@ -1266,4 +1273,6 @@ void igt_reset_link_params(int drm_fd, igt_output_t *output);
>  int igt_backlight_read(int *result, const char *fname, igt_backlight_context_t *context);
>  int igt_backlight_write(int value, const char *fname, igt_backlight_context_t *context);
>  
> +float igt_default_detect_timeout(void);
> +
>  #endif /* __IGT_KMS_H__ */
> 
> -- 
> 2.47.0
>