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

[Maarten Lankhorst]

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 at linux.intel.com>
>> ---
>>  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?

~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();
>>  }