[i-g-t,4/8] lib/psr: Make psr_active() only cares about PSR1

Submitted by Souza, Jose on Dec. 4, 2018, 11:09 p.m.

Details

Message ID 20181204230944.7753-4-jose.souza@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Souza, Jose Dec. 4, 2018, 11:09 p.m.
PSR2 will have it own function to detect if is active so we can drop
the sleep state search and also to make sure that PSR1 is active lets
search for "PSR1 enabled".

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index eecee459..c6d6390f 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -37,7 +37,8 @@  static bool psr_active(int debugfs_fd, bool check_active)
 
 	active = strstr(buf, "HW Enabled & Active bit: yes\n") ||
 		 strstr(buf, "Source PSR ctl: enabled");
-	active &= !!(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
+	active &= !!strstr(buf, "SRDENT");
+	active &= !!strstr(buf, "Status: PSR1 enabled");
 	return check_active ? active : !active;
 }
 

Comments

Dhinakaran Pandiyan Dec. 12, 2018, 12:16 a.m.
On Tue, 2018-12-04 at 15:09 -0800, José Roberto de Souza wrote:
> PSR2 will have it own function to detect if is active so we can drop
> the sleep state search and also to make sure that PSR1 is active lets
> search for "PSR1 enabled".

How do you plan to handle PSR2 in kms_frontbuffer_tracking or
fbt_fbcon? Have new PSR2 sub tests for them?


Would it be better to have some sort of PSR state maintained in the psr
library so that the functions know whether to test for PSR1 or PSR2?

> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index eecee459..c6d6390f 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -37,7 +37,8 @@ static bool psr_active(int debugfs_fd, bool
> check_active)
>  
>  	active = strstr(buf, "HW Enabled & Active bit: yes\n") ||
>  		 strstr(buf, "Source PSR ctl: enabled");
> -	active &= !!(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
> +	active &= !!strstr(buf, "SRDENT");
> +	active &= !!strstr(buf, "Status: PSR1 enabled");
>  	return check_active ? active : !active;
>  }
>
Souza, Jose Dec. 13, 2018, 7:47 p.m.
On Tue, 2018-12-11 at 16:16 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 15:09 -0800, José Roberto de Souza wrote:
> > PSR2 will have it own function to detect if is active so we can
> > drop
> > the sleep state search and also to make sure that PSR1 is active
> > lets
> > search for "PSR1 enabled".
> 
> How do you plan to handle PSR2 in kms_frontbuffer_tracking or
> fbt_fbcon? Have new PSR2 sub tests for them?

My plan is have a PSR and PSR2 function to make functions really simple
and easy to read.

> 
> 
> Would it be better to have some sort of PSR state maintained in the
> psr
> library so that the functions know whether to test for PSR1 or PSR2?
> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index eecee459..c6d6390f 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -37,7 +37,8 @@ static bool psr_active(int debugfs_fd, bool
> > check_active)
> >  
> >  	active = strstr(buf, "HW Enabled & Active bit: yes\n") ||
> >  		 strstr(buf, "Source PSR ctl: enabled");
> > -	active &= !!(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
> > +	active &= !!strstr(buf, "SRDENT");
> > +	active &= !!strstr(buf, "Status: PSR1 enabled");
> >  	return check_active ? active : !active;
> >  }
> >
Dhinakaran Pandiyan Dec. 13, 2018, 8:57 p.m.
On Thu, 2018-12-13 at 11:47 -0800, Souza, Jose wrote:
> On Tue, 2018-12-11 at 16:16 -0800, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-12-04 at 15:09 -0800, José Roberto de Souza wrote:
> > > PSR2 will have it own function to detect if is active so we can
> > > drop
> > > the sleep state search and also to make sure that PSR1 is active
> > > lets
> > > search for "PSR1 enabled".
> > 
> > How do you plan to handle PSR2 in kms_frontbuffer_tracking or
> > fbt_fbcon? Have new PSR2 sub tests for them?
> 
> My plan is have a PSR and PSR2 function to make functions really
> simple
> and easy to read.
> 

We'll end up with 

if (psr2)
	/*test PSR2 exit */
else
	/* test PSR1 exit */

in every single sub-test.


Since, the general format for both PSR1 and PSR2 tests is
1) Wait and check for PSR entry
2) Do some operation
3) Wait and check for PSR exit


Isn't it better to use the psr_wait_entry() and psr_wait_exit()
interfaces in all tests and deal with PSR1 and PSR2 specific way of
testing inside them?

> > 
> > 
> > Would it be better to have some sort of PSR state maintained in the
> > psr
> > library so that the functions know whether to test for PSR1 or
> > PSR2?
> > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  lib/igt_psr.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index eecee459..c6d6390f 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -37,7 +37,8 @@ static bool psr_active(int debugfs_fd, bool
> > > check_active)
> > >  
> > >  	active = strstr(buf, "HW Enabled & Active bit: yes\n") ||
> > >  		 strstr(buf, "Source PSR ctl: enabled");
> > > -	active &= !!(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
> > > +	active &= !!strstr(buf, "SRDENT");
> > > +	active &= !!strstr(buf, "Status: PSR1 enabled");
> > >  	return check_active ? active : !active;
> > >  }
> > >