[i-g-t,06/10] lib/psr: Make psr_wait_entry and psr_wait_update aware of the PSR version tested

Submitted by José Roberto de Souza on Jan. 12, 2019, 1:46 a.m.

Details

Message ID 20190112014607.13446-6-jose.souza@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

José Roberto de Souza Jan. 12, 2019, 1:46 a.m.
This way we can test both PSR version separated.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c                    | 26 ++++++++++++++++++--------
 lib/igt_psr.h                    |  9 +++++++--
 tests/kms_frontbuffer_tracking.c |  4 ++--
 tests/kms_psr.c                  |  4 ++--
 4 files changed, 29 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 5edec6a2..c6638c2c 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -25,26 +25,36 @@ 
 #include "igt_sysfs.h"
 #include <errno.h>
 
-static bool psr_active(int debugfs_fd, bool check_active)
+static bool psr_state_check(int debugfs_fd, const char *state)
 {
-	bool active;
 	char buf[PSR_STATUS_MAX_LEN];
 
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
 
-	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
-	return check_active ? active : !active;
+	return strstr(buf, state);
 }
 
-bool psr_wait_entry(int debugfs_fd)
+static inline const char *psr_active_state_get(enum psr_mode mode)
 {
-	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
+	return mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
 }
 
-bool psr_wait_update(int debugfs_fd)
+/*
+ * For PSR1, we wait until PSR is active. We wait until DEEP_SLEEP for PSR2.
+ */
+bool psr_wait_entry(int debugfs_fd, enum psr_mode mode)
 {
-	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
+	const char *state = psr_active_state_get(mode);
+
+	return igt_wait(psr_state_check(debugfs_fd, state), 500, 20);
+}
+
+bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
+{
+	const char *state = psr_active_state_get(mode);
+
+	return igt_wait(!psr_state_check(debugfs_fd, state), 40, 10);
 }
 
 static ssize_t psr_write(int debugfs_fd, const char *buf)
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index 62c680ee..4fff77ec 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -30,8 +30,13 @@ 
 
 #define PSR_STATUS_MAX_LEN 512
 
-bool psr_wait_entry(int debugfs_fd);
-bool psr_wait_update(int debugfs_fd);
+enum psr_mode {
+	PSR_MODE_1,
+	PSR_MODE_2
+};
+
+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_disable(int debugfs_fd);
 bool psr_sink_support(int debugfs_fd);
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index cd7dda91..ed9a039a 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1622,10 +1622,10 @@  static void do_status_assertions(int flags)
 	}
 
 	if (flags & ASSERT_PSR_ENABLED)
-		igt_assert_f(psr_wait_entry(drm.debugfs),
+		igt_assert_f(psr_wait_entry(drm.debugfs, PSR_MODE_1),
 			     "PSR still disabled\n");
 	else if (flags & ASSERT_PSR_DISABLED)
-		igt_assert_f(psr_wait_update(drm.debugfs),
+		igt_assert_f(psr_wait_update(drm.debugfs, PSR_MODE_1),
 			     "PSR still enabled\n");
 }
 
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index fb3bd7a4..23d000bd 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -199,7 +199,7 @@  static bool psr_wait_entry_if_enabled(data_t *data)
 	if (data->with_psr_disabled)
 		return true;
 
-	return psr_wait_entry(data->debugfs_fd);
+	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
 }
 
 static inline void manual(const char *expected)
@@ -289,7 +289,7 @@  static void run_test(data_t *data)
 		expected = "screen GREEN";
 		break;
 	}
-	igt_assert(psr_wait_update(data->debugfs_fd));
+	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
 	manual(expected);
 }
 

Comments

Dhinakaran Pandiyan Jan. 16, 2019, 5:19 a.m.
On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> This way we can test both PSR version separated.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c                    | 26 ++++++++++++++++++--------
>  lib/igt_psr.h                    |  9 +++++++--
>  tests/kms_frontbuffer_tracking.c |  4 ++--
>  tests/kms_psr.c                  |  4 ++--
>  4 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index 5edec6a2..c6638c2c 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -25,26 +25,36 @@
>  #include "igt_sysfs.h"
>  #include <errno.h>
>  
> -static bool psr_active(int debugfs_fd, bool check_active)
> +static bool psr_state_check(int debugfs_fd, const char *state)
nit: Is there a plan to use this function for anything else? Making
this function check only for the active state is more straightforward
IMO

-static bool psr_state_check(int debugfs_fd, const char *state)
+static bool psr_active_check(int debugfs_fd, enum psr_mode mode)
 {
        char buf[PSR_STATUS_MAX_LEN];
+       char *state;
 
+       state = mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
        igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
                                sizeof(buf));
 
        return strstr(buf, state);
 }
>  {
> -	bool active;
>  	char buf[PSR_STATUS_MAX_LEN];
>  
>  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
>  				sizeof(buf));
>  
> -	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
> -	return check_active ? active : !active;
> +	return strstr(buf, state);
>  }
>  
> -bool psr_wait_entry(int debugfs_fd)
> +static inline const char *psr_active_state_get(enum psr_mode mode)
>  {
> -	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
> +	return mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
>  }
>  
> -bool psr_wait_update(int debugfs_fd)
> +/*
> + * For PSR1, we wait until PSR is active. We wait until DEEP_SLEEP
> for PSR2.
> + */
> +bool psr_wait_entry(int debugfs_fd, enum psr_mode mode)
>  {
> -	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
> +	const char *state = psr_active_state_get(mode);
> +
> +	return igt_wait(psr_state_check(debugfs_fd, state), 500, 20);
> +}
> +
> +bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
> +{
> +	const char *state = psr_active_state_get(mode);
> +
> +	return igt_wait(!psr_state_check(debugfs_fd, state), 40, 10);
>  }
>  
>  static ssize_t psr_write(int debugfs_fd, const char *buf)
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 62c680ee..4fff77ec 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -30,8 +30,13 @@
>  
>  #define PSR_STATUS_MAX_LEN 512
>  
> -bool psr_wait_entry(int debugfs_fd);
> -bool psr_wait_update(int debugfs_fd);
> +enum psr_mode {
Add a 
	PSR_MODE_DISABLE, too?
> +	PSR_MODE_1,
> +	PSR_MODE_2
> +};
> +
> +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_disable(int debugfs_fd);
>  bool psr_sink_support(int debugfs_fd);
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index cd7dda91..ed9a039a 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1622,10 +1622,10 @@ static void do_status_assertions(int flags)
>  	}
>  
>  	if (flags & ASSERT_PSR_ENABLED)
> -		igt_assert_f(psr_wait_entry(drm.debugfs),
> +		igt_assert_f(psr_wait_entry(drm.debugfs, PSR_MODE_1),
>  			     "PSR still disabled\n");
>  	else if (flags & ASSERT_PSR_DISABLED)
> -		igt_assert_f(psr_wait_update(drm.debugfs),
> +		igt_assert_f(psr_wait_update(drm.debugfs, PSR_MODE_1),
>  			     "PSR still enabled\n");
>  }
>  
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index fb3bd7a4..23d000bd 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -199,7 +199,7 @@ static bool psr_wait_entry_if_enabled(data_t
> *data)
>  	if (data->with_psr_disabled)
>  		return true;
>  
> -	return psr_wait_entry(data->debugfs_fd);
> +	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
>  }
>  
>  static inline void manual(const char *expected)
> @@ -289,7 +289,7 @@ static void run_test(data_t *data)
>  		expected = "screen GREEN";
>  		break;
>  	}
> -	igt_assert(psr_wait_update(data->debugfs_fd));
> +	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
>  	manual(expected);
>  }
>
José Roberto de Souza Jan. 17, 2019, 1:59 a.m.
On Tue, 2019-01-15 at 21:19 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > This way we can test both PSR version separated.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c                    | 26 ++++++++++++++++++--------
> >  lib/igt_psr.h                    |  9 +++++++--
> >  tests/kms_frontbuffer_tracking.c |  4 ++--
> >  tests/kms_psr.c                  |  4 ++--
> >  4 files changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index 5edec6a2..c6638c2c 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -25,26 +25,36 @@
> >  #include "igt_sysfs.h"
> >  #include <errno.h>
> >  
> > -static bool psr_active(int debugfs_fd, bool check_active)
> > +static bool psr_state_check(int debugfs_fd, const char *state)
> nit: Is there a plan to use this function for anything else? Making
> this function check only for the active state is more straightforward
> IMO
> 
> -static bool psr_state_check(int debugfs_fd, const char *state)
> +static bool psr_active_check(int debugfs_fd, enum psr_mode mode)
>  {
>         char buf[PSR_STATUS_MAX_LEN];
> +       char *state;
>  
> +       state = mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
>         igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status",
> buf,
>                                 sizeof(buf));
>  
>         return strstr(buf, state);
>  }

Yeah, I don't see other use than check for active state.
I will apply this change, thanks.

> >  {
> > -	bool active;
> >  	char buf[PSR_STATUS_MAX_LEN];
> >  
> >  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
> >  				sizeof(buf));
> >  
> > -	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
> > -	return check_active ? active : !active;
> > +	return strstr(buf, state);
> >  }
> >  
> > -bool psr_wait_entry(int debugfs_fd)
> > +static inline const char *psr_active_state_get(enum psr_mode mode)
> >  {
> > -	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
> > +	return mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
> >  }
> >  
> > -bool psr_wait_update(int debugfs_fd)
> > +/*
> > + * For PSR1, we wait until PSR is active. We wait until DEEP_SLEEP
> > for PSR2.
> > + */
> > +bool psr_wait_entry(int debugfs_fd, enum psr_mode mode)
> >  {
> > -	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
> > +	const char *state = psr_active_state_get(mode);
> > +
> > +	return igt_wait(psr_state_check(debugfs_fd, state), 500, 20);
> > +}
> > +
> > +bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
> > +{
> > +	const char *state = psr_active_state_get(mode);
> > +
> > +	return igt_wait(!psr_state_check(debugfs_fd, state), 40, 10);
> >  }
> >  
> >  static ssize_t psr_write(int debugfs_fd, const char *buf)
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > index 62c680ee..4fff77ec 100644
> > --- a/lib/igt_psr.h
> > +++ b/lib/igt_psr.h
> > @@ -30,8 +30,13 @@
> >  
> >  #define PSR_STATUS_MAX_LEN 512
> >  
> > -bool psr_wait_entry(int debugfs_fd);
> > -bool psr_wait_update(int debugfs_fd);
> > +enum psr_mode {
> Add a 
> 	PSR_MODE_DISABLE, too?

To keep all functions simple we should not add this, otherwise we would
need to earlier return in psr_wait_entry(), psr_wait_update() and
psr_sink_support() when PSR_MODE_DISABLE is set? Or calling those
funciton with PSR_MODE_DISABLE mean that we should call psr_disable()?!

I guess is better just keep PSR_MODE_1 and PSR_MODE_2.

> > +	PSR_MODE_1,
> > +	PSR_MODE_2
> > +};
> > +
> > +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_disable(int debugfs_fd);
> >  bool psr_sink_support(int debugfs_fd);
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index cd7dda91..ed9a039a 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1622,10 +1622,10 @@ static void do_status_assertions(int flags)
> >  	}
> >  
> >  	if (flags & ASSERT_PSR_ENABLED)
> > -		igt_assert_f(psr_wait_entry(drm.debugfs),
> > +		igt_assert_f(psr_wait_entry(drm.debugfs, PSR_MODE_1),
> >  			     "PSR still disabled\n");
> >  	else if (flags & ASSERT_PSR_DISABLED)
> > -		igt_assert_f(psr_wait_update(drm.debugfs),
> > +		igt_assert_f(psr_wait_update(drm.debugfs, PSR_MODE_1),
> >  			     "PSR still enabled\n");
> >  }
> >  
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index fb3bd7a4..23d000bd 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -199,7 +199,7 @@ static bool psr_wait_entry_if_enabled(data_t
> > *data)
> >  	if (data->with_psr_disabled)
> >  		return true;
> >  
> > -	return psr_wait_entry(data->debugfs_fd);
> > +	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> >  }
> >  
> >  static inline void manual(const char *expected)
> > @@ -289,7 +289,7 @@ static void run_test(data_t *data)
> >  		expected = "screen GREEN";
> >  		break;
> >  	}
> > -	igt_assert(psr_wait_update(data->debugfs_fd));
> > +	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
> >  	manual(expected);
> >  }
> >