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

Dhinakaran Pandiyan
2019-01-16T05:33:55Z

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