Re: [igt-dev] [PATCH i-g-t 1/6] lib/psr: Add support for toggling edp psr through debugfs, v5.

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

On Wed, 2018-08-29 at 14:57 -0700, Dhinakaran Pandiyan wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> It's harmful to write to enable_psr at runtime, and the patch that
> allows
> us to change i915_edp_psr_debug with the panel running will require
> us
> to abandon the module parameter. Hence the userspace change needs to
> be
> put in IGT first before we can change it at kernel time.
> 
> Toggling it to debugfs will mean we can skip a modeset when changing
> our
> feature set.
> 
> Changes since v1:
> - Rebase with the previous patches dropped.
> Changes since v2:
> - Rebase on top of new api in i915_edp_psr_debug.
> Changes since v3:
> - Enable IRQ debugging for extra logging.
> - Force PSR1 mode. (dhnkrn)
> - Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)
> Changes since v4:
> - Redisable irqs right away when debugfs api doesn't work. (dhnkrn)
> - Use hex everywhere. (dhnkrn)
> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_psr.c                    | 86
> +++++++++++++++++++++++++++++++++++++++-
>  lib/igt_psr.h                    |  2 +
>  tests/kms_frontbuffer_tracking.c |  6 +--
>  3 files changed, 89 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index c979b0b5..614caa6f 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -21,7 +21,9 @@
>   * IN THE SOFTWARE.
>   */
>  
> -#include  "igt_psr.h"
> +#include "igt_psr.h"
> +#include "igt_sysfs.h"
> +#include <errno.h>
>  
>  bool psr_active(int fd, bool check_active)
>  {
> @@ -38,3 +40,85 @@ bool psr_wait_entry(int fd)
>  {
>  	return igt_wait(psr_active(fd, true), 500, 1);
>  }
> +
> +static ssize_t psr_write(int fd, const char *buf)
> +{
> +	return igt_sysfs_write(fd, "i915_edp_psr_debug", buf,
> strlen(buf));
> +}
> +
> +static int has_psr_debugfs(int fd)
> +{
> +	int ret;
> +
> +	/*
> +	 * Check if new PSR debugfs api is usable by writing an invalid
> value.
> +	 * Legacy mode will return OK here, debugfs api will return
> -EINVAL.
> +	 * -ENODEV is returned.
> +	 */
> +	ret = psr_write(fd, "0xf");
> +	if (ret == -EINVAL)
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +
> +	/* legacy debugfs api, we enabled irqs by writing, disable
> them. */
> +	psr_write(fd, "0");
> +	return -EINVAL;
> +}
> +
> +static bool psr_modparam_set(int val)
> +{
> +	static int oldval = -1;

This looks wrong, why not read the module parameter? it will guaratee
that the returned value is correct here if something else left the
parameter with another value.


> +
> +	igt_set_module_param_int("enable_psr", val);
> +
> +	if (val == oldval)
> +		return false;
> +
> +	oldval = val;
> +	return true;
> +}
> +
> +static int psr_restore_debugfs_fd = -1;

Maybe move this to the top of the file.

Other than that LGTM.

> +
> +static void restore_psr_debugfs(int sig)
> +{
> +	psr_write(psr_restore_debugfs_fd, "0");
> +}
> +
> +static bool psr_set(int fd, bool enable)
> +{
> +	int ret;
> +
> +	ret = has_psr_debugfs(fd);
> +	if (ret == -ENODEV) {
> +		igt_skip_on_f(enable, "PSR not available\n");
> +		return false;
> +	}
> +
> +	if (ret == -EINVAL) {
> +		ret = psr_modparam_set(enable);
> +	} else {
> +		ret = psr_write(fd, enable ? "0x3" : "0x1");
> +		igt_assert(ret > 0);
> +	}
> +
> +	/* Restore original value on exit */
> +	if (psr_restore_debugfs_fd == -1) {
> +		psr_restore_debugfs_fd = dup(fd);
> +		igt_assert(psr_restore_debugfs_fd >= 0);
> +		igt_install_exit_handler(restore_psr_debugfs);
> +	}
> +
> +	return ret;
> +}
> +
> +bool psr_enable(int fd)
> +{
> +	return psr_set(fd, true);
> +}
> +
> +bool psr_disable(int fd)
> +{
> +	return psr_set(fd, false);
> +}
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 980f85e0..0ef22c3d 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -30,5 +30,7 @@
>  
>  bool psr_wait_entry(int fd);
>  bool psr_active(int fd, bool check_active);
> +bool psr_enable(int fd);
> +bool psr_disable(int fd);
>  
>  #endif
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 1dfd7c1c..7ea2f697 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -941,8 +941,6 @@ static bool
> drrs_wait_until_rr_switch_to_low(void)
>  
>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>  #define drrs_enable()	drrs_set(1)
>  #define drrs_disable()	drrs_set(0)
>  
> @@ -1137,7 +1135,7 @@ static void disable_features(const struct
> test_mode *t)
>  		return;
>  
>  	fbc_disable();
> -	psr_disable();
> +	psr_disable(drm.debugfs);
>  	drrs_disable();
>  }
>  
> @@ -1719,7 +1717,7 @@ static void enable_features_for_test(const
> struct test_mode *t)
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		psr_enable();
> +		psr_enable(drm.debugfs);
>  	if (t->feature & FEATURE_DRRS)
>  		drrs_enable();
>  }
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev