[i-g-t,08/10] lib/psr: Add PSR2 support to the remaning psr functions

Submitted by Souza, Jose on Jan. 12, 2019, 1:46 a.m.

Details

Message ID 20190112014607.13446-8-jose.souza@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in IGT

Not browsing as part of any series.

Commit Message

Souza, Jose Jan. 12, 2019, 1:46 a.m.
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(-)

Patch hide | download patch | download mbox

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 */
+	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]");
 }
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);
 
 		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");

Comments

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");
On Tue, 2019-01-15 at 21:33 -0800, Dhinakaran Pandiyan wrote:
> 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.

I have commented in the previous patch, my opinion is that is better
have this comments and handle this way than let user call all other
functions with PSR_MODE_DISABLE.

> 
> > +	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>]?

Okay, I will change kernel and IGT to have this debugfs output.
We should print it just based on the DPCD version? Like version 0x2 is
PSR2 but we don't support PSR2 in that version of panel, same as a
version 0x3 that do not require the Y-coordinate.

> 
> >  }
> > 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");
On Wed, 2019-01-16 at 18:14 -0800, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:33 -0800, Dhinakaran Pandiyan wrote:
> > 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.
> 
> I have commented in the previous patch, my opinion is that is better
> have this comments and handle this way than let user call all other
> functions with PSR_MODE_DISABLE.
Let's go ahead as is and improve this later.

> 
> > 
> > > +	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>]?
> 
> Okay, I will change kernel and IGT to have this debugfs output.
> We should print it just based on the DPCD version? Like version 0x2
> is
> PSR2 but we don't support PSR2 in that version of panel, same as a
> version 0x3 that do not require the Y-coordinate.
Oh yeah, that can get confusing without the debug message. The version
info from the DPCD is clearer even if not descriptive. Feel free to
leave it as is.

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