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

[Pandiyan, Dhinakaran]

On Tue, 2018-08-14 at 13:08 +0200, Maarten Lankhorst wrote:
> Op 13-08-18 om 20:34 schreef Dhinakaran Pandiyan:
> > 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 at linux.int
> > > > > el.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.
> 
> Ok that's fine, should we also set the I915_PSR_DEBUG_IRQ flag?
Not yet. It is useful for interactive debugging but there were issues
when I tried to make IGT tests use the interrupts.


-DK

> 
> ~Maarten