[i-g-t,07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

Submitted by Souza, Jose on Jan. 12, 2019, 1:46 a.m.

Details

Message ID 20190112014607.13446-7-jose.souza@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in IGT

Not browsing as part of any series.

Commit Message

Souza, Jose Jan. 12, 2019, 1:46 a.m.
The nexts patches will add PSR2 tests and for that we need the new
PSR debugfs interface that was released in kernel 4.20 so it will not
break for any updated system and we can drop support to the old
debugsfs and the support to kernels without even a debugfs interface.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c | 35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index c6638c2c..5cc0fbc2 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -73,27 +73,7 @@  static int has_psr_debugfs(int debugfs_fd)
 	 * -ENODEV is returned when PSR is unavailable.
 	 */
 	ret = psr_write(debugfs_fd, "0xf");
-	if (ret == -EINVAL)
-		return 0;
-	else if (ret < 0)
-		return ret;
-
-	/* legacy debugfs api, we enabled irqs by writing, disable them. */
-	psr_write(debugfs_fd, "0");
-	return -EINVAL;
-}
-
-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;
+	return ret == -EINVAL ? 0 : ret;
 }
 
 static int psr_restore_debugfs_fd = -1;
@@ -109,16 +89,15 @@  static bool psr_set(int debugfs_fd, bool enable)
 
 	ret = has_psr_debugfs(debugfs_fd);
 	if (ret == -ENODEV) {
-		igt_skip_on_f(enable, "PSR not available\n");
+		igt_skip("PSR not available\n");
+		return false;
+	} else if (ret) {
+		igt_skip("PSR debugfs interface not available\n");
 		return false;
 	}
 
-	if (ret == -EINVAL) {
-		ret = psr_modparam_set(enable);
-	} else {
-		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
-		igt_assert(ret > 0);
-	}
+	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
+	igt_assert(ret > 0);
 
 	/* Restore original value on exit */
 	if (psr_restore_debugfs_fd == -1) {

Comments

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> The nexts patches will add PSR2 tests and for that we need the new
> PSR debugfs interface that was released in kernel 4.20 so it will not
> break for any updated system and we can drop support to the old
> debugsfs and the support to kernels without even a debugfs interface.

Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does not
support the new interface?


> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c | 35 +++++++----------------------------
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index c6638c2c..5cc0fbc2 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
>  	 * -ENODEV is returned when PSR is unavailable.
>  	 */
>  	ret = psr_write(debugfs_fd, "0xf");
> -	if (ret == -EINVAL)
> -		return 0;
> -	else if (ret < 0)
> -		return ret;
> -
> -	/* legacy debugfs api, we enabled irqs by writing, disable
> them. */
> -	psr_write(debugfs_fd, "0");
> -	return -EINVAL;
> -}
> -
> -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;
> +	return ret == -EINVAL ? 0 : ret;
>  }
>  
>  static int psr_restore_debugfs_fd = -1;
> @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool enable)
>  
>  	ret = has_psr_debugfs(debugfs_fd);
>  	if (ret == -ENODEV) {
> -		igt_skip_on_f(enable, "PSR not available\n");
> +		igt_skip("PSR not available\n");
> +		return false;
> +	} else if (ret) {
> +		igt_skip("PSR debugfs interface not available\n");
>  		return false;
>  	}
>  
> -	if (ret == -EINVAL) {
> -		ret = psr_modparam_set(enable);
> -	} else {
> -		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> -		igt_assert(ret > 0);
> -	}
> +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> +	igt_assert(ret > 0);
>  
>  	/* Restore original value on exit */
>  	if (psr_restore_debugfs_fd == -1) {
On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > The nexts patches will add PSR2 tests and for that we need the new
> > PSR debugfs interface that was released in kernel 4.20 so it will
> > not
> > break for any updated system and we can drop support to the old
> > debugsfs and the support to kernels without even a debugfs
> > interface.
> 
> Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does not
> support the new interface?

We could but why keep supporting old kernels? My experience of running
a newer IGT over a old kernel or on the contrary was always
problematic.
	

> 
> 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c | 35 +++++++----------------------------
> >  1 file changed, 7 insertions(+), 28 deletions(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index c6638c2c..5cc0fbc2 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
> >  	 * -ENODEV is returned when PSR is unavailable.
> >  	 */
> >  	ret = psr_write(debugfs_fd, "0xf");
> > -	if (ret == -EINVAL)
> > -		return 0;
> > -	else if (ret < 0)
> > -		return ret;
> > -
> > -	/* legacy debugfs api, we enabled irqs by writing, disable
> > them. */
> > -	psr_write(debugfs_fd, "0");
> > -	return -EINVAL;
> > -}
> > -
> > -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;
> > +	return ret == -EINVAL ? 0 : ret;
> >  }
> >  
> >  static int psr_restore_debugfs_fd = -1;
> > @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool
> > enable)
> >  
> >  	ret = has_psr_debugfs(debugfs_fd);
> >  	if (ret == -ENODEV) {
> > -		igt_skip_on_f(enable, "PSR not available\n");
> > +		igt_skip("PSR not available\n");
> > +		return false;
> > +	} else if (ret) {
> > +		igt_skip("PSR debugfs interface not available\n");
> >  		return false;
> >  	}
> >  
> > -	if (ret == -EINVAL) {
> > -		ret = psr_modparam_set(enable);
> > -	} else {
> > -		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > -		igt_assert(ret > 0);
> > -	}
> > +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > +	igt_assert(ret > 0);
> >  
> >  	/* Restore original value on exit */
> >  	if (psr_restore_debugfs_fd == -1) {
On Wed, 2019-01-16 at 18:07 -0800, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > The nexts patches will add PSR2 tests and for that we need the
> > > new
> > > PSR debugfs interface that was released in kernel 4.20 so it will
> > > not
> > > break for any updated system and we can drop support to the old
> > > debugsfs and the support to kernels without even a debugfs
> > > interface.
> > 
> > Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does
> > not
> > support the new interface?
> 
> We could but why keep supporting old kernels? My experience of
> running
> a newer IGT over a old kernel or on the contrary was always
> problematic.

Downstream OS'es can package a newer IGT with an older kernel. In this
case, it's not much of an effort to continue support for PSR1 tests
from what I can tell. 
> 	
> 
> > 
> > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  lib/igt_psr.c | 35 +++++++----------------------------
> > >  1 file changed, 7 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index c6638c2c..5cc0fbc2 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
> > >  	 * -ENODEV is returned when PSR is unavailable.
> > >  	 */
> > >  	ret = psr_write(debugfs_fd, "0xf");
> > > -	if (ret == -EINVAL)
> > > -		return 0;
> > > -	else if (ret < 0)
> > > -		return ret;
> > > -
> > > -	/* legacy debugfs api, we enabled irqs by writing, disable
> > > them. */
> > > -	psr_write(debugfs_fd, "0");
> > > -	return -EINVAL;
> > > -}
> > > -
> > > -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;
> > > +	return ret == -EINVAL ? 0 : ret;
> > >  }
> > >  
> > >  static int psr_restore_debugfs_fd = -1;
> > > @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool
> > > enable)
> > >  
> > >  	ret = has_psr_debugfs(debugfs_fd);
> > >  	if (ret == -ENODEV) {
> > > -		igt_skip_on_f(enable, "PSR not available\n");
> > > +		igt_skip("PSR not available\n");
> > > +		return false;
> > > +	} else if (ret) {
> > > +		igt_skip("PSR debugfs interface not available\n");
> > >  		return false;
> > >  	}
> > >  
> > > -	if (ret == -EINVAL) {
> > > -		ret = psr_modparam_set(enable);
> > > -	} else {
> > > -		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > -		igt_assert(ret > 0);
> > > -	}
> > > +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > +	igt_assert(ret > 0);
> > >  
> > >  	/* Restore original value on exit */
> > >  	if (psr_restore_debugfs_fd == -1) {
On Thu, Jan 17, 2019 at 02:07:27AM +0000, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > The nexts patches will add PSR2 tests and for that we need the new
> > > PSR debugfs interface that was released in kernel 4.20 so it will
> > > not
> > > break for any updated system and we can drop support to the old
> > > debugsfs and the support to kernels without even a debugfs
> > > interface.
> > 
> > Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does not
> > support the new interface?
> 
> We could but why keep supporting old kernels? My experience of running
> a newer IGT over a old kernel or on the contrary was always
> problematic.


Any problematic experiences you might have encountered should be filed
as bugs. We do support old kernels (to a length) and shouldn't
knowingly break support for them without an informed decision.
On Wed, 2019-01-16 at 18:16 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-01-16 at 18:07 -0800, Souza, Jose wrote:
> > On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> > > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > > The nexts patches will add PSR2 tests and for that we need the
> > > > new
> > > > PSR debugfs interface that was released in kernel 4.20 so it
> > > > will
> > > > not
> > > > break for any updated system and we can drop support to the old
> > > > debugsfs and the support to kernels without even a debugfs
> > > > interface.
> > > 
> > > Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does
> > > not
> > > support the new interface?
> > 
> > We could but why keep supporting old kernels? My experience of
> > running
> > a newer IGT over a old kernel or on the contrary was always
> > problematic.
> 
> Downstream OS'es can package a newer IGT with an older kernel. In
> this
> case, it's not much of an effort to continue support for PSR1 tests
> from what I can tell. 

Other problem here it that with the enable_psr parameter we don't
control what PSR version is going to be enabled, if possible PSR2 will
be enabled causing tests to fail in the first psr_wait_entry().

But okay, I will drop this patch.



> > 	
> > 
> > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  lib/igt_psr.c | 35 +++++++----------------------------
> > > >  1 file changed, 7 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > > index c6638c2c..5cc0fbc2 100644
> > > > --- a/lib/igt_psr.c
> > > > +++ b/lib/igt_psr.c
> > > > @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
> > > >  	 * -ENODEV is returned when PSR is unavailable.
> > > >  	 */
> > > >  	ret = psr_write(debugfs_fd, "0xf");
> > > > -	if (ret == -EINVAL)
> > > > -		return 0;
> > > > -	else if (ret < 0)
> > > > -		return ret;
> > > > -
> > > > -	/* legacy debugfs api, we enabled irqs by writing,
> > > > disable
> > > > them. */
> > > > -	psr_write(debugfs_fd, "0");
> > > > -	return -EINVAL;
> > > > -}
> > > > -
> > > > -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;
> > > > +	return ret == -EINVAL ? 0 : ret;
> > > >  }
> > > >  
> > > >  static int psr_restore_debugfs_fd = -1;
> > > > @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool
> > > > enable)
> > > >  
> > > >  	ret = has_psr_debugfs(debugfs_fd);
> > > >  	if (ret == -ENODEV) {
> > > > -		igt_skip_on_f(enable, "PSR not available\n");
> > > > +		igt_skip("PSR not available\n");
> > > > +		return false;
> > > > +	} else if (ret) {
> > > > +		igt_skip("PSR debugfs interface not
> > > > available\n");
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (ret == -EINVAL) {
> > > > -		ret = psr_modparam_set(enable);
> > > > -	} else {
> > > > -		ret = psr_write(debugfs_fd, enable ? "0x3" :
> > > > "0x1");
> > > > -		igt_assert(ret > 0);
> > > > -	}
> > > > +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > > +	igt_assert(ret > 0);
> > > >  
> > > >  	/* Restore original value on exit */
> > > >  	if (psr_restore_debugfs_fd == -1) {