Re: [igt-dev] [PATCH i-g-t v3 3/6] tests/fbcon_fbt: Add and user psr_long_wait_update()

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Wed, 2019-04-10 at 15:07 -0700, José Roberto de Souza wrote:
> When fbcon is enabled, PSR will be active between cursor blinks so
> what it should really use to test PSR is psr_wait_entry(), so a new
> feature callback was added.
We could add a PSR only blinking subtest that verifies both entry and exit
happen as the next step.

> But the fbcon cursor blinks at 5hz what give us 200ms between each
> screen update what make psr_wait_update() prone to fail the test
> because it timed out before a blink could happen, so here adding and
> using psr_long_wait_update() that have a longer timeout.

Patch 4 that sets up the test conditions should come before this patch that
validates the kernel's response.

Also, is there a way to read the blink rate?

> 
> v3:
> - 3 previous patches squashed in this one (Maarten)
> - Back to !feature->wait_until_enabled() to test feature state when
> all CRTCS are disabled(Dhinakaran)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c         |  5 +++++
>  lib/igt_psr.h         |  1 +
>  tests/kms_fbcon_fbt.c | 17 +++++++++++++++--
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index b5847bfd..b92ea73f 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -54,6 +54,11 @@ bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
>  	return igt_wait(!psr_active_check(debugfs_fd, mode), 40, 10);
>  }
>  
> +bool psr_long_wait_update(int debugfs_fd, enum psr_mode mode)
> +{
> +	return igt_wait(!psr_active_check(debugfs_fd, mode), 500, 10);
nit: We could reduce the debugfs read frequency as the timeout is also larger,
haven't really given enough though into what's a sensible upper limit. But,
something like 50 should be okay.

> +}
> +
>  static ssize_t psr_write(int debugfs_fd, const char *buf)
>  {
>  	return igt_sysfs_write(debugfs_fd, "i915_edp_psr_debug", buf,
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 49599cf8..ca385736 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -37,6 +37,7 @@ enum psr_mode {
>  
>  bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
>  bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
> +bool psr_long_wait_update(int debugfs_fd, enum psr_mode mode);
>  bool psr_enable(int debugfs_fd, enum psr_mode);
>  bool psr_disable(int debugfs_fd);
>  bool psr_sink_support(int debugfs_fd, enum psr_mode);
> diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> index a9d91839..a5340d85 100644
> --- a/tests/kms_fbcon_fbt.c
> +++ b/tests/kms_fbcon_fbt.c
> @@ -130,6 +130,11 @@ static bool fbc_wait_until_enabled(int debugfs_fd)
>  	return r;
>  }
>  
> +static bool fbc_wait_until_update(int debugfs)
> +{
This warrants an explanation as to why fbc_wait_until_update ==
!fbc_wait_until_enabled.

My understanding is that we do not expect fbc to become active at all with
fbcon. Can you confirm that?

With an explanation included and patches re-ordered, 

Reviewed-by: Dhinkaran Pandiyan <dhinakaran.pandiyan@intel.com>

> +	return !fbc_wait_until_enabled(debugfs);
> +}
> +
>  typedef bool (*connector_possible_fn)(drmModeConnectorPtr connector);
>  
>  static void set_mode_for_one_screen(struct drm_info *drm, struct igt_fb *fb,
> @@ -196,6 +201,11 @@ static bool psr_supported_on_chipset(int debugfs_fd)
>  	return psr_sink_support(debugfs_fd, PSR_MODE_1);
>  }
>  
> +static bool psr_wait_until_update(int debugfs_fd)
> +{
> +	return psr_long_wait_update(debugfs_fd, PSR_MODE_1);
> +}
> +
>  static void disable_features(int debugfs_fd)
>  {
>  	igt_set_module_param_int("enable_fbc", 0);
> @@ -215,16 +225,19 @@ static inline void psr_debugfs_enable(int debugfs_fd)
>  struct feature {
>  	bool (*supported_on_chipset)(int debugfs_fd);
>  	bool (*wait_until_enabled)(int debugfs_fd);
> +	bool (*wait_until_update)(int debugfs_fd);
>  	bool (*connector_possible_fn)(drmModeConnectorPtr connector);
>  	void (*enable)(int debugfs_fd);
>  } fbc = {
>  	.supported_on_chipset = fbc_supported_on_chipset,
>  	.wait_until_enabled = fbc_wait_until_enabled,
> +	.wait_until_update = fbc_wait_until_update,
>  	.connector_possible_fn = connector_can_fbc,
>  	.enable = fbc_modparam_enable,
>  }, psr = {
>  	.supported_on_chipset = psr_supported_on_chipset,
>  	.wait_until_enabled = psr_wait_until_enabled,
> +	.wait_until_update = psr_wait_until_update,
>  	.connector_possible_fn = connector_can_psr,
>  	.enable = psr_debugfs_enable,
>  };
> @@ -263,13 +276,13 @@ static void subtest(struct feature *feature, bool
> suspend)
>  	sleep(3);
>  
>  	wait_user("Back to fbcon.");
> -	igt_assert(!feature->wait_until_enabled(drm.debugfs_fd));
> +	igt_assert(feature->wait_until_update(drm.debugfs_fd));
>  
>  	if (suspend) {
>  		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>  					      SUSPEND_TEST_NONE);
>  		sleep(5);
> -		igt_assert(!feature->wait_until_enabled(drm.debugfs_fd));
> +		igt_assert(feature->wait_until_update(drm.debugfs_fd));
>  	}
>  }
>  

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev