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

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5435 bytes --]

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);
> >  }
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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