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@gmail.com>]

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.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.

> +		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.
> +
> +	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.

> +	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_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.

> +
>  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