Re: [igt-dev] [PATCH i-g-t 08/10] lib/psr: Add PSR2 support to the remaning psr functions

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

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> Add the mode parameter to psr_enable() and psr_sink_support() so PSR1
> and PSR2 can be tested separated.
> For now all PSR tests will run only with PSR1 and the tests for PSR2
> will come in the future.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c                    | 39 +++++++++++++++++++++++++-----
> --
>  lib/igt_psr.h                    |  4 ++--
>  tests/kms_fbcon_fbt.c            |  9 ++++++--
>  tests/kms_frontbuffer_tracking.c |  4 ++--
>  tests/kms_psr.c                  |  5 ++--
>  5 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index 5cc0fbc2..d7028f6c 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -83,9 +83,10 @@ static void restore_psr_debugfs(int sig)
>  	psr_write(psr_restore_debugfs_fd, "0");
>  }
>  
> -static bool psr_set(int debugfs_fd, bool enable)
> +static bool psr_set(int debugfs_fd, enum psr_mode mode)
>  {
>  	int ret;
> +	const char *debug_val;
>  
>  	ret = has_psr_debugfs(debugfs_fd);
>  	if (ret == -ENODEV) {
> @@ -96,7 +97,18 @@ static bool psr_set(int debugfs_fd, bool enable)
>  		return false;
>  	}
>  
> -	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> +	switch (mode) {
> +	case PSR_MODE_1:
> +		debug_val = "0x3";
> +		break;
> +	case PSR_MODE_2:
> +		debug_val = "0x2";
> +		break;
> +	default:
> +		debug_val = "0x1";
> +	}
> +
> +	ret = psr_write(debugfs_fd, debug_val);
>  	igt_assert(ret > 0);
>  
>  	/* Restore original value on exit */
> @@ -109,23 +121,34 @@ static bool psr_set(int debugfs_fd, bool
> enable)
>  	return ret;
>  }
>  
> -bool psr_enable(int debugfs_fd)
> +bool psr_enable(int debugfs_fd, enum psr_mode mode)
>  {
> -	return psr_set(debugfs_fd, true);
> +	return psr_set(debugfs_fd, mode);
>  }
>  
>  bool psr_disable(int debugfs_fd)
>  {
> -	return psr_set(debugfs_fd, false);
> +	/* Any mode different than PSR_MODE_1/2 will disable PSR */
Please consider adding PSR_MODE_DISABLE to get rid of this comment.

> +	return psr_set(debugfs_fd, PSR_MODE_2 + 1);
>  }
>  
> -bool psr_sink_support(int debugfs_fd)
> +bool psr_sink_support(int debugfs_fd, enum psr_mode mode)
>  {
>  	char buf[PSR_STATUS_MAX_LEN];
>  	int ret;
>  
>  	ret = igt_debugfs_simple_read(debugfs_fd,
> "i915_edp_psr_status", buf,
>  				      sizeof(buf));
> -	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
> -			   strstr(buf, "Sink support: yes"));
> +	if (ret < 1)
> +		return false;
> +
> +	if (mode == PSR_MODE_1)
> +		return strstr(buf, "Sink_Support: yes\n") ||
> +		       strstr(buf, "Sink support: yes");
> +	else
> +		/*
> +		 * i915 requires PSR version 0x03 that is PSR2 + SU
> with
> +		 * Y-coordinate to support PSR2
> +		 */
> +		return strstr(buf, "Sink support: yes [0x03]");
For some reason, I thought we also print whether the sink supports PSR1
or PSR2 in debugfs. Hope it's not too late, did we ever consider

Sink support: {No, PSR1, PSR2} [<PSR dpcd version>]?

>  }
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 4fff77ec..7e7017bf 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -37,8 +37,8 @@ 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_enable(int debugfs_fd);
> +bool psr_enable(int debugfs_fd, enum psr_mode);
>  bool psr_disable(int debugfs_fd);
> -bool psr_sink_support(int debugfs_fd);
> +bool psr_sink_support(int debugfs_fd, enum psr_mode);
>  
>  #endif
> diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> index 2823b47a..9d0d5a36 100644
> --- a/tests/kms_fbcon_fbt.c
> +++ b/tests/kms_fbcon_fbt.c
> @@ -199,6 +199,11 @@ static bool psr_wait_until_enabled(int
> debugfs_fd)
>  	return r;
>  }
>  
> +static bool psr_supported_on_chipset(int debugfs_fd)
> +{
> +	return psr_sink_support(debugfs_fd, PSR_MODE_1);
> +}
> +
>  static void disable_features(int debugfs_fd)
>  {
>  	igt_set_module_param_int("enable_fbc", 0);
> @@ -212,7 +217,7 @@ static inline void fbc_modparam_enable(int
> debugfs_fd)
>  
>  static inline void psr_debugfs_enable(int debugfs_fd)
>  {
> -	psr_enable(debugfs_fd);
> +	psr_enable(debugfs_fd, PSR_MODE_1);
>  }
>  
>  struct feature {
> @@ -226,7 +231,7 @@ struct feature {
>  	.connector_possible_fn = connector_can_fbc,
>  	.enable = fbc_modparam_enable,
>  }, psr = {
> -	.supported_on_chipset = psr_sink_support,
> +	.supported_on_chipset = psr_supported_on_chipset,
>  	.wait_until_enabled = psr_wait_until_enabled,
>  	.connector_possible_fn = connector_can_psr,
>  	.enable = psr_debugfs_enable,
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index ed9a039a..609f7b41 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1425,7 +1425,7 @@ static void setup_psr(void)
>  		return;
>  	}
>  
> -	if (!psr_sink_support(drm.debugfs)) {
> +	if (!psr_sink_support(drm.debugfs, PSR_MODE_1)) {
>  		igt_info("Can't test PSR: not supported by sink.\n");
>  		return;
>  	}
> @@ -1722,7 +1722,7 @@ static bool enable_features_for_test(const
> struct test_mode *t)
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		ret = psr_enable(drm.debugfs);
> +		ret = psr_enable(drm.debugfs, PSR_MODE_1);
>  	if (t->feature & FEATURE_DRRS)
>  		drrs_enable();
>  
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 23d000bd..c31dc317 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -191,7 +191,8 @@ static void fill_render(data_t *data, uint32_t
> handle, unsigned char color)
>  
>  static bool sink_support(data_t *data)
>  {
> -	return data->with_psr_disabled || psr_sink_support(data-
> >debugfs_fd);
> +	return data->with_psr_disabled ||
> +	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
>  }
>  
>  static bool psr_wait_entry_if_enabled(data_t *data)
> @@ -410,7 +411,7 @@ int main(int argc, char *argv[])
>  		data.devid = intel_get_drm_devid(data.drm_fd);
>  
Store it in struct data so that the value stays consistent throughout
the test case?
>  		if (!data.with_psr_disabled)
> -			psr_enable(data.debugfs_fd);
> +			psr_enable(data.debugfs_fd, PSR_MODE_1);
>  
>  		igt_require_f(sink_support(&data),
>  			      "Sink does not support PSR\n");

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