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

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 06/11/24 - 15:12, Kamil Konieczny wrote:
> Hi Louis,
> On 2024-10-22 at 12:28:35 +0200, Louis Chauvet wrote:
> > 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@bootlin.com>
> > ---
> >  lib/igt_core.c |  3 +++
> >  lib/igt_kms.c  | 24 ++++++++++++++++++++++++
> >  lib/igt_kms.h  |  9 +++++++++
> >  3 files changed, 36 insertions(+)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 407f7b55187c..5f75141cc42e 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -265,6 +265,9 @@
> >   *	&num; It is not mandatory and allows overriding default values.
> >   *	[DUT]
> >   *	SuspendResumeDelay=10
> > + *	&num; The following option define the timeout for detection feature
> > + *	&num; (waiting for a connector status)
> > + *	DetectTimeout=10.0
> 
> You named it generic, could you keep it in igt_core?
> Or name it DisplayDetectTimeout?

I will change it to v2, I think DisplayDetectTimeout is good.
 
> >   * ]|
> >   *
> >   * 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 bb35d4b82c5a..195868646a14 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
> > @@ -7119,3 +7120,26 @@ void igt_reset_link_params(int drm_fd, igt_output_t *output)
> >  	temp = drmModeGetConnector(drm_fd, output->config.connector->connector_id);
> >  	drmModeFreeConnector(temp);
> >  }
> > +
> > +/**
> > + * 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)
> > +{
> > +	static double timeout = 0.0;
> > +	static bool first_call = true;
> > +
> > +	if (first_call) {
> > +		if (igt_key_file)
> > +			timeout = g_key_file_get_double(igt_key_file, "DUT", "DetectTimeout", NULL);
> 
> There could be errors here, please handle them.

I will do it for v2, in case of errors it will use DEFAULT_DETECT_TIMEOUT.

Thanks for your review,
Louis Chauvet

> Regards,
> Kamil
> 
> > +		else
> > +			timeout = DEFAULT_DETECT_TIMEOUT;
> > +
> > +		first_call = false;
> > +	}
> > +
> > +	return timeout;
> > +}
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 2b26d2bbfff1..4f0030264d9f 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -39,6 +39,13 @@
> >  #include "igt_fb.h"
> >  #include "ioctl_wrappers.h"
> >  
> > +/**
> > + * define DEFAULT_DETECT_TIMEOUT - Default timeout used for some detection functions
> > + *
> > + * It can be overiden by option DetectTimeout in the .igtrc file.
> > + */
> > +#define DEFAULT_DETECT_TIMEOUT 10.0
> > +
> >  /* Low-level helpers with kmstest_ prefix */
> >  
> >  /**
> > @@ -1254,4 +1261,6 @@ int igt_get_dp_pending_lt_failures(int drm_fd, igt_output_t *output);
> >  int igt_get_dp_pending_retrain(int drm_fd, igt_output_t *output);
> >  void igt_reset_link_params(int drm_fd, igt_output_t *output);
> >  
> > +float igt_default_detect_timeout(void);
> > +
> >  #endif /* __IGT_KMS_H__ */
> > 
> > -- 
> > 2.46.2
> >