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

[Maarten Lankhorst]

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.intel.com>
>> ---
>>  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,
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)
>
>
>