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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@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.
Ok that's fine, should we also set the I915_PSR_DEBUG_IRQ flag?

~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev