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

[Pandiyan, Dhinakaran]

On Tue, 2018-09-04 at 14:59 -0700, Souza, Jose wrote:
> On Wed, 2018-08-29 at 14:57 -0700, Dhinakaran Pandiyan wrote:
> > From: Maarten Lankhorst <maarten.lankhorst at 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 at intel.com>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at 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.
> 
This patch was merged as part of a different series, send a fix and Cc
Maarten?
> 
> > +
> > +	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();
> >  }