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

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

On Mon, 2018-08-13 at 09:11 +0200, Maarten Lankhorst wrote:
> Op 11-08-18 om 03:50 schreef Dhinakaran Pandiyan:
> > On Friday, August 10, 2018 3:06:45 AM PDT 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.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > >  tests/kms_frontbuffer_tracking.c | 44
> > > ++++++++++++++++++++++++++++++--
> > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c index
> > > 1dfd7c1cee8d..06ad55709dc6 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -775,6 +775,46 @@ static void drrs_set(unsigned int val)
> > >  		igt_assert_f(ret == (sizeof(buf) - 1),
> > > "debugfs_write failed");
> > >  }
> > > 
> > > +static void restore_psr_debugfs(int sig)
> > > +{
> > > +	debugfs_write("i915_edp_psr_debug", "0");
> > > +}
> > > +
> > > +static bool psr_modparam_set(unsigned int val)
> > > +{
> > > +	static int oldval = -1;
> > > +
> > > +	igt_set_module_param_int("enable_psr", val);
> > > +
> > > +	if (val == oldval)
> > > +		return false;
> > > +
> > > +	oldval = val;
> > > +	return true;
> > > +}
> > > +
> > > +static bool psr_set(bool enable)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Check if new PSR debugfs api is usable. */
> > > +	ret = debugfs_write("i915_edp_psr_debug", "0xf");
> > > +	if (ret == -ENODEV) {
> > > +		/* PSR not enabled, should only be able to set
> > > or unset. */
> > 
> > s/enabled/supported
> > -ENODEV is returned only when source or sink do not support PSR.
> 
> Indeed, typo. :)
> > > +		igt_assert(!enable);
> > > +		return false;
> > > +	}
> > > +
> > > +	if (ret != -EINVAL)
> > 
> > I would add a comment to explain that you are tying -EINVAL return
> > to mean the 
> > driver supports the new API. This needs a comment in the driver
> > too?
> > 
> > > +		return psr_modparam_set(enable);
> > 
> > Don't see this return value being used.
> 
> It's used in 2/2.
> > > +
> > > +	ret = debugfs_write("i915_edp_psr_debug", enable ? "0x2"
> > > : "0x1");
> > 
> > We should be using DEBUG_FORCE_PSR1 here. The only reason we were
> > testing PSR2 
> > is because the module parameter did not have an option to force
> > PSR1.
> 
> Ok, I was under the impression we want to test the newest PSR
> possible,
No, PSR2 isn't ready for testing yet. We should be testing only PSR1
now.

> because that's what the HW will use when enable_psr=1 is passed as
> modparam?
> > > +	igt_assert_lt(0, ret);
> > 
> > Even if the return was -ERESTARTSYS? Isn't one of the benefits of
> > passing the 
> > return value from modeset_lock and
> > wait_for_completion_interruptible that 
> > userspace can retry? Or does  -ERESTARTSYS never reach this point?
> 
> igt_sysfs_write handles signals with the static writeN() function.
> > > +
> > > +	igt_install_exit_handler(restore_psr_debugfs);
> > > +	return false;
> > > +}
> > 
> > It is better to move all of this to  lib/igt_psr so that kms_psr
> > and 
> > kms_fbt_fbcon also use the same code.
> 
> Actually, it might very well be a good idea. :)
> > > +
> > >  static bool is_drrs_high(void)
> > >  {
> > >  	char buf[MAX_DRRS_STATUS_BUF_LEN];
> > > @@ -941,8 +981,8 @@ 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 psr_enable()	psr_set(1)
> > > +#define psr_disable()	psr_set(0)
> > >  #define drrs_enable()	drrs_set(1)
> > >  #define drrs_disable()	drrs_set(0)
> > 
> > 
> > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev