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
2019-01-17T01:59:47Z

[-- 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 --]