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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com>]

On Thu, 2018-08-16 at 13:41 +0200, Maarten Lankhorst wrote:
> Op 16-08-18 om 02:26 schreef Dhinakaran Pandiyan:
> > On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> > > 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)
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > >  lib/igt_psr.c                    | 90
> > > +++++++++++++++++++++++++++++++-
> > >  lib/igt_psr.h                    |  2 +
> > >  tests/kms_frontbuffer_tracking.c |  6 +--
> > >  3 files changed, 93 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index c979b0b59936..393702037861 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,89 @@ 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 -EINVAL;
> > 
> > We should disable debug here, PSR interrupts aren't used in IGTs.
> > Looks
> >  like you sent this patch before I responded to your question about
> > interrupts.
> > 
> > 
> > > +	else
> > > +		return ret;
> > > +}
> > > +
> > > +static bool psr_modparam_set(int val)
> > > +{
> > > +	static int oldval = -1;
> > > +
> > > +	igt_set_module_param_int("enable_psr", val);
> > > +
> > > +	if (val == oldval)
> > > +		return false;
> > > +
> > > +	oldval = val;
> > > +	return true;
> > > +}
> > > +
> > > +static int psr_restore_debugfs_fd = -1;
> > > +
> > > +static void restore_psr_debugfs(int sig)
> > > +{
> > > +	psr_write(psr_restore_debugfs_fd, "0");
> > > +}
> > > +
> > > +static bool psr_set(int fd, bool enable)
> > > +{
> > > +	int ret;
> > > +
> > > +	igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
> > > +
> > > +	ret = has_psr_debugfs(fd);
> > > +	if (ret == -ENODEV) {
> > > +		igt_skip_on_f(enable, "PSR not available\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Restore original debugfs value on exit, even for
> > > +	 * legacy case, because writing 0xf will set the debug
> > > flag
> > > +	 * regardless.
> > > +	 */
> > > +	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);
> > > +	}
> > > +
> > > +	if (ret == -EINVAL) {
> > > +		ret = psr_modparam_set(enable);
> > > +	} else {
> > > +		ret = psr_write(fd, enable ? "0x13" : "1");
> > 
> > "0x3" to avoid enabling interrupts.
> > 
> > And am not quite sure how this return translates to need_modeset in
> > the
> > next patch. Why is a modeset needed if psr_write() successfully
> > wrote
> > all the bytes?
> 
> Oops, should reset to false in that case.
> 
> Any other comments on the series or is it good with those 2 fixes?

I need a bit more time to review patch 2/2, not very familiar with the
flow in frontbuffer_tracking.c


> 
> ~Maarten
> > > +		igt_assert(ret > 0);
> > > +	}
> > > +
> > > +	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 980f85e04b3e..0ef22c3d1258 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 1dfd7c1cee8d..7ea2f697184d 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
-- 
-DK
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev